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

Normalize images loaded from storage #1548

Merged
merged 1 commit into from Jul 14, 2023

Conversation

thomas-brx
Copy link
Contributor

This PR is a workaround for #1547.

It will run engine.normalization() after the engine has loaded the buffer from storage.

Happy to try to add some tests for this if the workaround feels acceptable.

@sonarcloud
Copy link

sonarcloud bot commented Jan 27, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@github-actions
Copy link

This PR is stale because it has been open 60 days with no activity. Remove the stale label or add a comment, or this PR will be closed in 30 days. You can always re-open if you feel this is something we should still keep working on. Tag @heynemann for more information.

@github-actions github-actions bot added the Stale label Mar 29, 2023
@github-actions
Copy link

This PR was closed because it has been stale for 30 days with no activity.

@RaphaelVRossi
Copy link
Member

Hey @thomas-brx, first of all, thanks for your contribution!

What do you think about normalize be called after retrieve from storage?

Otherwise, all other calls to method _fetch will be normalized too.

e.g.

[...]
                if (
                    mime == "image/gif"
                    and self.context.config.USE_GIFSICLE_ENGINE
                ):
                    self.context.request.engine = (
                        self.context.modules.gif_engine
                    )
                else:
                    self.context.request.engine = self.context.modules.engine

                fetch_result.normalized = self.context.request.engine.normalize()
                return fetch_result
[...]

@RaphaelVRossi RaphaelVRossi reopened this Jun 29, 2023
@coveralls
Copy link

coveralls commented Jun 29, 2023

Pull Request Test Coverage Report for Build 5521461086

  • 2 of 2 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.3%) to 86.672%

Totals Coverage Status
Change from base Build 5486971773: 0.3%
Covered Lines: 4006
Relevant Lines: 4622

💛 - Coveralls

@devppjr devppjr force-pushed the normalize-images-from-storage branch from 170c33d to de302c1 Compare July 7, 2023 13:54
@scorphus
Copy link
Member

scorphus commented Jul 8, 2023

#1568 didn't see any test, I hope this one tells a different story 😊 Please LMK if I can help with anything.

@devppjr
Copy link
Contributor

devppjr commented Jul 10, 2023

Hey @thomas-brx, first of all, thanks for your contribution!

What do you think about normalize be called after retrieve from storage?

Otherwise, all other calls to method _fetch will be normalized too.

e.g.

[...]
                if (
                    mime == "image/gif"
                    and self.context.config.USE_GIFSICLE_ENGINE
                ):
                    self.context.request.engine = (
                        self.context.modules.gif_engine
                    )
                else:
                    self.context.request.engine = self.context.modules.engine

                fetch_result.normalized = self.context.request.engine.normalize()
                return fetch_result
[...]

@RaphaelVRossi thanks, I was testing your idea but I didn't succeed 😢. When we normalize the image we use the image sizes (here), and if we normalized the image at this point we would not have self.size initialized, as the image hasn't yet been loaded with self.context.request.engine.load, like here. IMHO we should keep with @thomas-brx contribution, what do you think?

ps: @scorphus feel free to give your opinion (always) ❤️

@devppjr devppjr force-pushed the normalize-images-from-storage branch 3 times, most recently from 83c3317 to e4e0cb3 Compare July 10, 2023 18:55
@thomas-brx
Copy link
Contributor Author

@RaphaelVRossi, @devppjr. Thank you so much for taking a look at this, sorry for not being able to respond sooner. Had some issues getting the test suite to run yesterday, but I see a test has been added already 😍

@devppjr devppjr force-pushed the normalize-images-from-storage branch 5 times, most recently from 7915742 to 02db082 Compare July 11, 2023 13:42
@sonarcloud
Copy link

sonarcloud bot commented Jul 11, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@devppjr devppjr merged commit 3e35fc0 into thumbor:master Jul 14, 2023
24 checks passed
@scorphus
Copy link
Member

Awesome to see one more bug being fixed! Thanks all of you for your contributions! ❤️

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.

Images from storage are not normalized (not respecting MAX_WIDTH / MAX_HEIGHT)
5 participants