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

Use Capsule for WEBP saving #8386

Open
wants to merge 11 commits into
base: main
Choose a base branch
from
Open

Conversation

homm
Copy link
Member

@homm homm commented Sep 16, 2024

Adoption of #8341 ideas

Improvements

  • Doesn't create extra image copy (.tobytes("raw", rawmode))
  • Uses the same more appropriate API .has_transparency_data for save and save_all.
  • Supports saving RGBX mode without conversion
  • Adds ImagingSectionEnter/Leave for WebPAnimEncoder

Also:
remove _VALID_WEBP_MODES and _VALID_WEBP_LEGACY_MODES consts
RGBX is not faster RGB since demands more bandwidth
Do not convert to str paths in tests
@homm homm added the WebP label Sep 16, 2024
Tests/test_file_webp.py Outdated Show resolved Hide resolved
@Yay295
Copy link
Contributor

Yay295 commented Sep 16, 2024

You're essentially using _VALID_WEBP_MODES in two locations, but you removed that constant. I assume you inlined them when _VALID_WEBP_MODES and _VALID_WEBP_LEGACY_MODES were each only used in one place, but it seems like _VALID_WEBP_MODES could be kept. Also it could just be a Set instead of a Dict with True values.

@homm
Copy link
Member Author

homm commented Sep 17, 2024

@Yay295 I've removed this constants primary since it's mistyping. Now I'm added separate _convert_frame routine for this.

@radarhere
Copy link
Member

I expected these changes to improve speed, but I'm not finding a definitive difference when I test. Does that mirror your experience?

Are the improvements here more about theory and tidying up?

@Yay295
Copy link
Contributor

Yay295 commented Sep 21, 2024

I don't know how much of an effect it has, but there is some overhead when using Capsules because they use strings as identifiers. So there's some added string comparisons behind the scenes.

@homm
Copy link
Member Author

homm commented Sep 22, 2024

@radarhere

Are the improvements here more about theory and tidying up?

All improvements are listed in the PR description. While elimination of .tobytes() doesn't affect execution time much, it helps to avid extra image copy, which reduces memory footprint. See "Maximum resident set size" below. Also, ImagingSectionEnter/Leave in WebPAnimEncoder unlocks GIL in the current thread, which improves IO in other threads.

from io import BytesIO
from PIL import Image
for i in range(4):
    with BytesIO() as f:
        Image.new('RGB', (8192, 8192)).save(f, 'webp', method=0, quality=1)
# AMD Cpu

## Main branch
$ /usr/bin/time -v python test.py
  Command being timed: "python test.py"
  User time (seconds): 19.39
  System time (seconds): 1.16
  Percent of CPU this job got: 99%
  Elapsed (wall clock) time (h:mm:ss or m:ss): 0:20.63
  Maximum resident set size (kbytes): 1296972
  Major (requiring I/O) page faults: 18
  Minor (reclaiming a frame) page faults: 1484790
  Voluntary context switches: 19
  Involuntary context switches: 719

## webp-capsule branch
$ /usr/bin/time -v python test.py 
  Command being timed: "python test.py"
  User time (seconds): 19.46
  System time (seconds): 0.78
  Percent of CPU this job got: 99%
  Elapsed (wall clock) time (h:mm:ss or m:ss): 0:20.27
  Maximum resident set size (kbytes): 1100660
  Major (requiring I/O) page faults: 0
  Minor (reclaiming a frame) page faults: 1090899
  Voluntary context switches: 1
  Involuntary context switches: 1180


# Local Docker (M1 Pro)

## Main branch
$ /usr/bin/time -v python test.py
    Command being timed: "python test.py"
    User time (seconds): 4.51
    System time (seconds): 0.35
    Percent of CPU this job got: 99%
    Elapsed (wall clock) time (h:mm:ss or m:ss): 0:04.89
    Maximum resident set size (kbytes): 851160
    Major (requiring I/O) page faults: 9
    Minor (reclaiming a frame) page faults: 674262
    Voluntary context switches: 495
    Involuntary context switches: 88

## webp-capsule branch
$ /usr/bin/time -v python test.py
    Command being timed: "python test.py"
    User time (seconds): 4.18
    System time (seconds): 0.24
    Percent of CPU this job got: 99%
    Elapsed (wall clock) time (h:mm:ss or m:ss): 0:04.44
    Maximum resident set size (kbytes): 648432
    Major (requiring I/O) page faults: 9
    Minor (reclaiming a frame) page faults: 286806
    Voluntary context switches: 390
    Involuntary context switches: 10

@homm
Copy link
Member Author

homm commented Sep 22, 2024

@Yay295

some overhead when using Capsules because they use strings as identifiers

It's literally nothing comparing to the copying megabytes of image data to the byte string. But as you can see above, even the copying doesn't affect encoding time much.

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

Successfully merging this pull request may close these issues.

3 participants