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

[FIX] base: fix autoresize and exclude gif #96307

Closed
wants to merge 1 commit into from

Conversation

nle-odoo
Copy link
Contributor

@nle-odoo nle-odoo commented Jul 19, 2022

Odoo may resize attachment image with side larger than 1920 pixels.

But for animated gifs, this resizement seems to in general increase size
file which is not what we want (in some case making it grow from 3MB to
60 MB).

So with this change, we only resize and optimize images that are not
gifs.

Reasoning: pillow doesn't seem to resize GIF (and seems to only increase
their size, especially animated GIF, because each frame is not
optimized) so we should just not touch them.

Note:

  • currently tiff were not resized (because of a mimetype typo)
  • currently image dimensions were not resized (from our test, resizing
    on the dimension does not change the size much, quality is most
    important).

both of these issue have been solved in this commit.

opw-2897291

Note: In the ticket case, a 4MB gif would grow to 60 MB. Here is an example of a 300 KB gif that is getting saved as 3 MB:

bignyan

@robodoo
Copy link
Contributor

robodoo commented Jul 19, 2022

@C3POdoo C3POdoo added the OE the report is linked to a support ticket (opw-...) label Jul 19, 2022
@C3POdoo C3POdoo requested review from a team July 19, 2022 11:43
@xmo-odoo
Copy link
Collaborator

xmo-odoo commented Jul 19, 2022

Maybe we should disable this entire thing for gifs? Before python-pillow/Pillow#5291 (apparently released in 8.2.0) pillow doesn't compress the frames themselves, it stores the frame data uncompressed.

Updating to the latest (9.2) the image size still increases when round-tripping but only to 388.9kB. Which I guess is less bad.

I also found https://stackoverflow.com/questions/41718892/pillow-resizing-a-gif in which the author halves the image dimensions which multiplies the image size by 4, so it looks like it'd be necessary to go from a very large image to a very small one in order for there to be a gain due to the loss of intra-frame compression.

@nle-odoo
Copy link
Contributor Author

Maybe we should disable this entire thing for gifs?

I think it might be better, I've changed the PR so we don't do it for animated GIF. In any case I think animated gif higher than 1920 must not happen too often.

@nle-odoo nle-odoo force-pushed the 14.0-base-opw-2897291-nle branch 2 times, most recently from ebcf44f to 5ff903b Compare July 19, 2022 14:14
Copy link
Collaborator

@xmo-odoo xmo-odoo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems good to me, I'd just like the commit message updated with the additional decisions and the reasoning.

@nle-odoo nle-odoo changed the title [FIX] base: skip resize if animated gif gets bigger [FIX] base: fix autoresize and exclude gif Jul 20, 2022
@nle-odoo
Copy link
Contributor Author

Thanks, I did some more test and I removed the resize of all GIF completely. It always increased the size from my test even on non-animated ones, eg. 9 to 14 MB on pillow 5, improved to 9 to 9.1 MB on pillow 9.2 version.

I fixed the resize to not leave dead code.

Odoo may resize attachment image with side larger than 1920 pixels.

But for animated gifs, this resizement seems to in general increase size
file which is not what we want (in some case making it grow from 3MB to
60 MB).

So with this change, we only resize and optimize images that are not
gifs.

Reasoning: pillow doesn't seem to resize GIF (and seems to only increase
their size, especially animated GIF, because each frame is not
optimized) so we should just not touch them.

Note:

- currently tiff were not resized (because of a mimetype typo)
- currently image dimensions were not resized (from our test, resizing
  on the dimension does not change the size much, quality is most
  important).

both of these issue have been solved in this commit.

opw-2897291
@xmo-odoo
Copy link
Collaborator

👍 anything more to change or is this GTG?

@nle-odoo
Copy link
Contributor Author

good to go, thanks

robodoo r+

robodoo pushed a commit that referenced this pull request Jul 27, 2022
Odoo may resize attachment image with side larger than 1920 pixels.

But for animated gifs, this resizement seems to in general increase size
file which is not what we want (in some case making it grow from 3MB to
60 MB).

So with this change, we only resize and optimize images that are not
gifs.

Reasoning: pillow doesn't seem to resize GIF (and seems to only increase
their size, especially animated GIF, because each frame is not
optimized) so we should just not touch them.

Note:

- currently tiff were not resized (because of a mimetype typo)
- currently image dimensions were not resized (from our test, resizing
  on the dimension does not change the size much, quality is most
  important).

both of these issue have been solved in this commit.

opw-2897291

closes #96307

Signed-off-by: Nicolas Lempereur (nle) <nle@odoo.com>
@robodoo robodoo closed this Jul 27, 2022
@robodoo robodoo temporarily deployed to merge July 27, 2022 10:48 Inactive
@fw-bot fw-bot deleted the 14.0-base-opw-2897291-nle branch August 10, 2022 11:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
OE the report is linked to a support ticket (opw-...)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants