Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support serving images under s3 subdirectories, Fix to make /fit-in/ work; Fix for VipsJpeg: Invalid SOS error plus several other critical fixes. #130

Closed
wants to merge 13 commits into from

Conversation

rpong
Copy link

@rpong rpong commented Jul 24, 2019

This PR includes fixes for the following v4 issues:

  • S3 subdirectories not working.
  • /fit-in/ url directive not working.
  • Exif orientation not honoured/followed.
  • VipsJpeg: Invalid SOS parameters for sequential JPEG errors. This is observable with images taken with Samsung cameras, causing the api/cloudfront endpoint to output a blank json {} instead of the image.
  • Wrong resizing if the path/filename contains an 'x' with surrounding digits (eg: /image8x8.jpg)

Sample paths in which this was tested on aside from the unit tests:

/100x100/directory1/directory2/image.jpg
/fit-in/100x100/directory1/directory2/image.jpg
/fit-in/100x100/filters:someFilters/directory1/directory2/image.jpg
/filters:someFilters/directory1/directory2/image.jpg
/100x100/filters:someFilters/directory1/directory2/image_8x8x8x12.jpg

You may start using this PR/Fork with this Exported Function zip: https://github.com/innovationlove/serverless-image-handler/releases/tag/4.0.1-ilove

Just replace the code that your Serverless Image Handler Function is using with the package above. USE AT YOUR OWN RISK.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@beyerz
Copy link

beyerz commented Jul 25, 2019

I have manually added this solution to our lambda-function and can confirm that it performs as defined.
@rpong You are a life saver!

@rpong rpong changed the title Support serving images under s3 subdirectories Support serving images under s3 subdirectories and fix to make /fit-in/ work Jul 25, 2019
@denys1yermakov
Copy link

denys1yermakov commented Jul 25, 2019

@rpong, images without resizing still doesn't work, example:
/directory1/directory2/image.jpg
If you can fix it in that pull request will be super helpfull, thanks!

@codyhawke
Copy link

Been trying to get this going - everything seems to work fine, except it's not resizing the image when in a subdirectory - example url:
https://xxxxxxx.cloudfront.net/400x0/products/img/product01.jpg

@rpong
Copy link
Author

rpong commented Jul 27, 2019

@rpong, images without resizing still doesn't work, example:
/directory1/directory2/image.jpg
If you can fix it in that pull request will be super helpfull, thanks!

sorry, my bad. this should be fixed now.

@rpong
Copy link
Author

rpong commented Jul 27, 2019

Been trying to get this going - everything seems to work fine, except it's not resizing the image when in a subdirectory - example url:
https://xxxxxxx.cloudfront.net/400x0/products/img/product01.jpg

can you attach the actual photo and actual filename? we discovered an anomaly with filenames containing an 'x' and digits around it causing improper resizing, we will be applying the fix for the PR too in a bit, just want to make sure about your use case and perhaps cover it with our fix later today. thanks man..

@rpong rpong changed the title Support serving images under s3 subdirectories and fix to make /fit-in/ work Support serving images under s3 subdirectories, Fix to make /fit-in/ work and Fix for VipsJpeg: Invalid SOS error Jul 27, 2019
@gendigbadig
Copy link

gendigbadig commented Jul 27, 2019

Been trying to get this going - everything seems to work fine, except it's not resizing the image when in a subdirectory - example url:
https://xxxxxxx.cloudfront.net/400x0/products/img/product01.jpg

can you attach the actual photo and actual filename? we discovered an anomaly with filenames containing an 'x' and digits around it causing improper resizing, we will be applying the fix for the PR too in a bit, just want to make sure about your use case and perhaps cover it with our fix later today. thanks man..

in my use case, some of key path contains 'x' characters, and i discovered dimension path checking on thumbor-mapping.js is using only String.prototype.includes method, so i change those line to using regex match

Line 39

           else if (edit.match(/^\d+x\d+$/)) { // change this line
                this.edits.resize = {};
                const dims = edit.split('x');
                this.edits.resize.width = Number(dims[0]);
                this.edits.resize.height = Number(dims[1]);
            } 

Please add it to your PR, thank you :)

@denys1yermakov
Copy link

Been trying to get this going - everything seems to work fine, except it's not resizing the image when in a subdirectory - example url:
https://xxxxxxx.cloudfront.net/400x0/products/img/product01.jpg

can you attach the actual photo and actual filename? we discovered an anomaly with filenames containing an 'x' and digits around it causing improper resizing, we will be applying the fix for the PR too in a bit, just want to make sure about your use case and perhaps cover it with our fix later today. thanks man..

in my use case, some of key path contains 'x' characters, and i discovered dimension path checking on thumbor-mapping.js is using only String.prototype.includes method, so i change those line to using regex match

Line 39

           else if (edit.match(/^\d+x\d+$/)) { // change this line
                this.edits.resize = {};
                const dims = edit.split('x');
                this.edits.resize.width = Number(dims[0]);
                this.edits.resize.height = Number(dims[1]);
            } 

Please add it to your PR, thank you :)

that work, thanks !

@gendigbadig
Copy link

gendigbadig commented Jul 27, 2019

also, in previous version (thumbor), i have some use case that unfortunately unsupported on current version:

  1. using key path without file extension
    Example: https://xxxx.cloudfront.net/64x64/images/svara-radio-logo/download/58e5ecc7cab82dc1700c0043
    My workaround is remove path extension checking on parseRequestType method on image-request.js
parseRequestType(event) {
    const path = event["path"];
    // ----
    const matchDefault = new RegExp(
      /^(\/?)([0-9a-zA-Z+\/]{4})*(([0-9a-zA-Z+\/]{2}==)|([0-9a-zA-Z+\/]{3}=))?$/
    );
    const matchThumbor = new RegExp(
      /^(\/?)((fit-in)?|(filters:.+\(.?\))?|(unsafe)?).*$/  // Remove extension on this regex
    ); 
    const matchCustom = new RegExp(/(\/?)(.*)(jpg|png|webp|tiff|jpeg)/);
    const definedEnvironmentVariables =
      process.env.REWRITE_MATCH_PATTERN !== "" &&
      process.env.REWRITE_SUBSTITUTION !== "" &&
      process.env.REWRITE_MATCH_PATTERN !== undefined &&
      process.env.REWRITE_SUBSTITUTION !== undefined;
    // ----
    if (matchDefault.test(path)) {
      // use sharp
      return "Default";
    } else if (matchCustom.test(path) && definedEnvironmentVariables) {
      // use rewrite function then thumbor mappings
      return "Custom";
    } else if (matchThumbor.test(path)) {
      // use thumbor mappings
      return "Thumbor";
    } else {
      throw {
        status: 400,
        code: "RequestTypeError",
        message:
          "The type of request you are making could not be processed. Please ensure that your original image is of a supported file type (jpg, png, tiff, webp) and that your image request is provided in the correct syntax. Refer to the documentation for additional guidance on forming image requests."
      };
    }
  }
  1. using smart path
    For now, my workaround is only omit the smart keyword on parseImageKey method at image-request.js
parseImageKey(event, requestType) {
    if (requestType === "Default") {
      // Decode the image request and return the image key
      const decoded = this.decodeRequest(event);
      return decoded.key;
    } else if (requestType === "Thumbor" || requestType === "Custom") {
      // Parse the key from the end of the path
      const pathParts = event["path"].split("/");

      for (let i = 0; i < pathParts.length; i++) {
        if (
          pathParts[i] !== "" &&
          pathParts[i] !== "fit-in" &&
          pathParts[i].match(/^\d+x\d+$/) == null &&
          pathParts[i].match(/^filters:/) == null &&
          pathParts[i].match(/^smart$/) == null  // add this line
        ) {
          const parts = pathParts.slice(i);
          return parts.join("/");
        }
      }
    }

    // Return an error for all other conditions
    throw {
      status: 400,
      code: "ImageEdits::CannotFindImage",
      message:
        "The image you specified could not be found. Please check your request syntax as well as the bucket you specified to ensure it exists."
    };
  }
  1. using filters fill with same usage as thumbor, ex:filters:fill(white,1)
    Currently my workaround on this problem is adding some hardcoded line to convert filter value to color object that accepted by sharpjs, ex: 'white,1' to {r: 255, g:255, b: 255, alpha: 0}
    I added it on method mapFilter() at thumbor-mapping.js

maybe you have more clean workaround for this problem
thank you

@rpong
Copy link
Author

rpong commented Jul 27, 2019

Been trying to get this going - everything seems to work fine, except it's not resizing the image when in a subdirectory - example url:
https://xxxxxxx.cloudfront.net/400x0/products/img/product01.jpg

can you attach the actual photo and actual filename? we discovered an anomaly with filenames containing an 'x' and digits around it causing improper resizing, we will be applying the fix for the PR too in a bit, just want to make sure about your use case and perhaps cover it with our fix later today. thanks man..

in my use case, some of key path contains 'x' characters, and i discovered dimension path checking on thumbor-mapping.js is using only String.prototype.includes method, so i change those line to using regex match

Line 39

           else if (edit.match(/^\d+x\d+$/)) { // change this line
                this.edits.resize = {};
                const dims = edit.split('x');
                this.edits.resize.width = Number(dims[0]);
                this.edits.resize.height = Number(dims[1]);
            } 

Please add it to your PR, thank you :)

Nice! thank you for this, I added this to the PR but moved it outside the foreach loop so it is processed/looked up only once. Just updated the PR as well so the tests pass and several other critical things that required us to fix before we launched our beta app this morning.

@rpong rpong changed the title Support serving images under s3 subdirectories, Fix to make /fit-in/ work and Fix for VipsJpeg: Invalid SOS error Support serving images under s3 subdirectories, Fix to make /fit-in/ work and Fix for VipsJpeg: Invalid SOS error plus several other fixes. Jul 27, 2019
@rpong rpong changed the title Support serving images under s3 subdirectories, Fix to make /fit-in/ work and Fix for VipsJpeg: Invalid SOS error plus several other fixes. Support serving images under s3 subdirectories, Fix to make /fit-in/ work; Fix for VipsJpeg: Invalid SOS error plus several other critical fixes. Jul 27, 2019
@codyhawke
Copy link

Still not working for some of my URLS such as the following:
https://xxxxxxxxx.cloudfront.net/400x400/products/img/ricky-powell-beastie-boys-18x13-1xrun-01.jpg

@rpong
Copy link
Author

rpong commented Jul 28, 2019

Still not working for some of my URLS such as the following:
https://xxxxxxxxx.cloudfront.net/400x400/products/img/ricky-powell-beastie-boys-18x13-1xrun-01.jpg

https://i.ibb.co/v4RRc5h/Screen-Shot-2019-07-28-at-4-35-55-PM.png

thanks for that use case, just pushed a change to the PR which should take care of it, strange that the unit-tests missed this.

@adam91holt
Copy link

Hi @rpong

Thanks for all your hard work. Another issue I just spotted is when you form a URL like so...

https://d1h4lz25tmeef3.cloudfront.net/fit-in/400x/clients/demo/images/flip/ichts176-navaksha-original-imaeceu7sqmcekt8.jpeg

Specifying "400x" with the old Thumbor used to give you the best fit for 400 and figure it out itself. However, doing that now will give you "NoSuchKey". If you use 400x400 for example then it works fine.

https://d1h4lz25tmeef3.cloudfront.net/fit-in/400x400/clients/demo/images/flip/ichts176-navaksha-original-imaeceu7sqmcekt8.jpeg

@rpong
Copy link
Author

rpong commented Jul 29, 2019

https://d1h4lz25tmeef3.cloudfront.net/fit-in/400x400/clients/demo/images/flip/ichts176-navaksha-original-imaeceu7sqmcekt8.jpeg

Hi @adam91holt , noted on this, for now, if this is an actual production use case for you, what we do to circumvent around this is to specify zero and a fit-in.

https://d1h4lz25tmeef3.cloudfront.net/fit-in/400x0/clients/demo/images/flip/ichts176-navaksha-original-imaeceu7sqmcekt8.jpeg

I did not realize they support 400x before, such dimension convention also wasn't in the unit tests so we did not cover that.

@beyerz
Copy link

beyerz commented Sep 1, 2019

This PR resolves some major BC issues for me. Is there an expected time that we can see this merged back into the main repo?

@carlmathisen
Copy link

Any news on this PR making it in?

@andyfurniss4
Copy link

I've tried implementing this manually by exporting the existing Lambda function code, making the changes to the files and then re-uploading a newly zipped version of the function but I'm getting the following error:

module initialization error: Error
at Object.hasVendoredLibvips (/var/task/node_modules/sharp/lib/libvips.js:61:13)
at Object.<anonymous> (/var/task/node_modules/sharp/lib/constructor.js:9:22)
at Module._compile (module.js:652:30)
at Object.Module._extensions..js (module.js:663:10)
at Module.load (module.js:565:32)
at tryModuleLoad (module.js:505:12)
at Function.Module._load (module.js:497:3)
at Module.require (module.js:596:17)
at require (internal/module.js:11:18)
at Object.<anonymous> (/var/task/node_modules/sharp/lib/index.js:3:15)

Any ideas?

@rpong
Copy link
Author

rpong commented Sep 18, 2019

I've tried implementing this manually by exporting the existing Lambda function code, making the changes to the files and then re-uploading a newly zipped version of the function but I'm getting the following error:

module initialization error: Error
at Object.hasVendoredLibvips (/var/task/node_modules/sharp/lib/libvips.js:61:13)
at Object.<anonymous> (/var/task/node_modules/sharp/lib/constructor.js:9:22)
at Module._compile (module.js:652:30)
at Object.Module._extensions..js (module.js:663:10)
at Module.load (module.js:565:32)
at tryModuleLoad (module.js:505:12)
at Function.Module._load (module.js:497:3)
at Module.require (module.js:596:17)
at require (internal/module.js:11:18)
at Object.<anonymous> (/var/task/node_modules/sharp/lib/index.js:3:15)

Any ideas?

easier to just use this pre-built one: #130 (comment) goodluck and let us know.

@GeorgeBellTMH
Copy link

Any updates?

@komandar
Copy link

komandar commented Nov 7, 2019

I've tried implementing this manually by exporting the existing Lambda function code, making the changes to the files and then re-uploading a newly zipped version of the function but I'm getting the following error:

module initialization error: Error
at Object.hasVendoredLibvips (/var/task/node_modules/sharp/lib/libvips.js:61:13)
at Object.<anonymous> (/var/task/node_modules/sharp/lib/constructor.js:9:22)
at Module._compile (module.js:652:30)
at Object.Module._extensions..js (module.js:663:10)
at Module.load (module.js:565:32)
at tryModuleLoad (module.js:505:12)
at Function.Module._load (module.js:497:3)
at Module.require (module.js:596:17)
at require (internal/module.js:11:18)
at Object.<anonymous> (/var/task/node_modules/sharp/lib/index.js:3:15)

Any ideas?

easier to just use this pre-built one: #130 (comment) goodluck and let us know.

Worked very well for me in combination with #157

@padre
Copy link

padre commented Dec 3, 2019

@rpong Thanks for your contribution.

But there is a problem: if the filename contains square brackets, the regular expression does not work.

Example: [test].jpg does not work (throw error 400)

@rpong
Copy link
Author

rpong commented Dec 18, 2019

I've tried implementing this manually by exporting the existing Lambda function code, making the changes to the files and then re-uploading a newly zipped version of the function but I'm getting the following error:

module initialization error: Error
at Object.hasVendoredLibvips (/var/task/node_modules/sharp/lib/libvips.js:61:13)
at Object.<anonymous> (/var/task/node_modules/sharp/lib/constructor.js:9:22)
at Module._compile (module.js:652:30)
at Object.Module._extensions..js (module.js:663:10)
at Module.load (module.js:565:32)
at tryModuleLoad (module.js:505:12)
at Function.Module._load (module.js:497:3)
at Module.require (module.js:596:17)
at require (internal/module.js:11:18)
at Object.<anonymous> (/var/task/node_modules/sharp/lib/index.js:3:15)

Any ideas?

easier to just use this pre-built one: #130 (comment) goodluck and let us know.

Worked very well for me in combination with #157

Glad to help, btw just released the same package we use with Node 12.x -> https://github.com/innovationlove/serverless-image-handler/releases/tag/4.0.3

Goodluck.

@rpong
Copy link
Author

rpong commented Dec 18, 2019

@rpong Thanks for your contribution.

But there is a problem: if the filename contains square brackets, the regular expression does not work.

Example: [test].jpg does not work (throw error 400)

Will look into this within the week.

@padre
Copy link

padre commented Dec 19, 2019

@rpong Thank you very much, again! But I don't think it's working properly. How do you crop an image to exactly a specified size?

Before upgrading, if I didn't use the fit-in argument, the image auto-cropped and auto-resized to be EXACTLY the specified size. Now domain.com/fit-in/50x50/image.jpg and domain.com/50x50/image.jpg produces the same result :-(

@Vita1ik
Copy link

Vita1ik commented Dec 26, 2019

Why in this case https://cloudfront-fake-endpoint.net/fit-in/400x0/image_id.jpg image is not covered, I receive an empty object(

It doesn't work with lambda which runs on node.js 12.x, but I have the older Serverless Image Handler that runs the lambda on node.js 8.10 and this one works properly

@OBrown92
Copy link

OBrown92 commented Jan 9, 2020

Got also some problems with the newest version 4.1. Is it still possible to deploy version 4 and then upgrade the node version with the release from innovationlove?

@Vita1ik
Copy link

Vita1ik commented Jan 15, 2020

Why in this case https://cloudfront-fake-endpoint.net/fit-in/400x0/image_id.jpg image is not covered, I receive an empty object(

It doesn't work with lambda which runs on node.js 12.x, but I have the older Serverless Image Handler that runs the lambda on node.js 8.10 and this one works properly

filters:upscale() solved my issue =)

@beomseoklee
Copy link
Member

Thanks for your effort here, @rpong
We have updated our solution, and your pull request has been merged into our master branch. However, as we've merged many pull requests at this time, the source code might not exactly same as what you've done. In addition, due to our internal process, we've merged source code manually, so please understand how your effort has been merged.

You can refer to the recent changes here

@ijhar8
Copy link

ijhar8 commented Mar 9, 2020

@rpong this solution is not working for image size more thane 4MB .
getting error .
{"message": "Internal server error"}

@OBrown92
Copy link

OBrown92 commented Mar 9, 2020

I think this is more a Aws Lambda problem because the request and response size is limited:
Request and response body payload size are maximized to 6 MB. (aws)
Maybe some overhead or something consuming the 2mb left.

@giovannipds
Copy link

Is there a way to use the old fit-in behavior url without having to overwrite the code?

@cxrod
Copy link

cxrod commented Apr 6, 2020

@giovannipds the old fit-it is working on the last release. Just update your stack with the latest template.

@giovannipds
Copy link

@cxrod seriously? I probably have edited the code before putting the stack up the last time I've updated here (29 days ago).

@giovannipds
Copy link

@cxrod I won't re-do this right now since it's already working as I need on my stack 😆 but thanks for letting me know.

@cxrod
Copy link

cxrod commented Apr 15, 2020

@giovannipds sorry for the late reply! I was observing for a long time the PR with the fix, until it was released on February

@giovannipds
Copy link

@cxrod no worries! o/

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.