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

Thumbor URL "filters" changes from 3.1.1 to 4.2 #187

Closed
Furytron opened this issue Feb 17, 2020 · 6 comments
Closed

Thumbor URL "filters" changes from 3.1.1 to 4.2 #187

Furytron opened this issue Feb 17, 2020 · 6 comments

Comments

@Furytron
Copy link

Hi all,

I upgraded from 3.1.1 to 4.2 to begin some local testing since I found out that S3 paths could be followed in 4.2 for Thumbor URLs. I noticed that the supported URL format seems to have changed between the versions. Just wondering if this was something that was planned or a possible oversight or a bug maybe?

These URLs would have worked on 3.1.1. Notice that the filter rules are chained with a ":" colon delimiter.

  1. https://cloudfront.net/filters:quality(80):blur(11)/path/to/file/cat.jpg
  2. https://cloudfront.net/fit-in/300x300/filters:rotate(90):convolution(1;2;1;2;4;2;1;2;1,3,false)/path/to/file/cat.jpg

After upgrading to 4.2 using the Cloudformation template the above 2 URLs no longer work. The first URL needs to have an extra "filters:" keyword before the "blur". The documentation does reflect this change. however, if you had a lot of filters in the URL, it would be unnecessarily long since it needs to include the keyword "filters:" for each one. It seems kind of redundant to specify the "filters" keyword for each filter in the URL. The Thumbor documentation also has examples of filter chaining, which leads me to think its a bug or something.

The fixed first URL:

  1. https://cloudfront.net/filters:quality(80)/filters:blur(11)/path/to/file/cat.jpg

For the second link, the "convolution" filter doesn't appear to work from what I have tried. Not sure if I have the filter wrong, but I am using the same one from the documentation. I can see that its mapped in the Thumbor mapping code, so not sure whats wrong. I can open up a new issue for that if you think its necessary.

  1. https://cloudfront.net/fit-in/300x300/filters:rotate(90)/filters:convolution(1;2;1;2;4;2;1;2;1,3,false)/path/to/file/cat.jpg

Any help would be appreciated! Thanks

@beomseoklee
Copy link
Member

Thanks for contacting us, @Furytron
I'm not sure the design decision of the chain filters in v4, but according to the source code, you need to have "filters" keyword for each filters, but it can be a good discussion with others regarding the chain filters.

For the second question, I can see that the below one causes the issue.
https://github.com/awslabs/serverless-image-handler/blob/19cbc3ce759d7c8d8ddc35081972d7ac1daf0c71/source/image-handler/image-request.js#L188-L190.

If you change the code to the below, I believe it can fix the issue, and we will definitely fix this one in the next update.

if (requestType === "Thumbor" || requestType === "Custom") {
    return decodeURIComponent(event["path"].replace(/\d+x\d+\/|filters[:-][^/]+|\/fit-in\/+|^\/+/g,'').replace(/^\/+/,''));
}

@Furytron
Copy link
Author

@beomseoklee Thanks for the quick reply!

Great to see that youve already figured out and fixed convolution issue. I thought I was going mad! I ran a couple of local tests with your fix and can see that removing the semi-colon will fix the issue. It will actually work in both circumstances with multiple "filters" keywords and with filter chaining. So that is great news!.

I would like to hear more about filter chaining though from others. It would be great to support it in a future release, especially since Thumbor docs reference it. If I get time I will take a look and see if I can spot where the issue is in the code. Maybe there is a quick fix.

@beomseoklee
Copy link
Member

@Furytron
Thanks for waiting. convolution filter is fixed in v5.0.0.
We will consider how we can handle filter chaining.

@github-actions
Copy link

This issue has not received a response in a while. If you want to keep this issue open, please leave a comment below and auto-close will be canceled.

@github-actions
Copy link

This issue was closed because it has been inactive for 7 days since being marked as closing-soon.

@simonkrol
Copy link
Member

Hi @Furytron,
My apologies for the ping. We're looking to gain some insight into how our most active Github customers are using the Solution and how we can better support those use cases. Would you be interested in having a half hour call with me some time over the next few weeks? Let me know here if you're interested and I'll get the process underway.

Thank you,
Simon

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

No branches or pull requests

3 participants