Repository navigation
Avoid conflicting JFIF and Adobe markers for RGB JPEGs - #10345
Muguett-DBY wants to merge 7 commits into
Conversation
|
I find the conditions for using RGB very complex. What if instead, when keep_rgb and DPI are used together, we abandon the JFIF header, and store the DPI in EXIF? Muguett-DBY#2 |
|
Thanks, avoiding the conflicting JFIF marker when writing is a much simpler approach. I tried the proposed commit and found a DPI case the 72-DPI test seems to miss: from io import BytesIO
from PIL import Image
out = BytesIO()
Image.new("RGB", (20, 20), "red").save(out, "JPEG", keep_rgb=True, dpi=(96, 96))
out.seek(0)
with Image.open(out) as im:
print(im.getpixel((0, 0)), im.info["dpi"]) # (255, 0, 0), (72, 72)
One trade-off to consider: a writer-only change would prevent new files from having both markers, but files already saved by older Pillow versions would still decode with the wrong colors. What do you think about handling those existing files? Checked with Codex. |
I've created #10353
I think this would be better addressed in #10334
I've pushed a commit for this.
If Pillow saved those images incorrectly, then they have incorrect colours. |
4c942d2 to
94de18b
Compare
|
Thanks, the writer-only approach makes sense. Avoiding this marker combination when saving is preferable to changing how other software's files are decoded. The current branch now reflects your replacement, and I've updated the PR description accordingly. I checked I'll leave non-square DPI to #10334 as you suggest. |
|
Thanks for merging #10353 and updating the branch. While checking the new head, I found a case the square-DPI test misses: from io import BytesIO
from PIL import Image
exif = Image.Exif()
exif[0x011A] = 300
exif[0x011B] = 200
exif[0x0128] = 3 # resolution in centimetres
out = BytesIO()
Image.new("RGB", (12, 12)).save(out, "JPEG", keep_rgb=True, dpi=(96, 96), exif=exif)
with Image.open(BytesIO(out.getvalue())) as im:
print(im.info["dpi"]) # (243.84, 243.84), not (96, 96)The new XResolution is in inches, but the copied ResolutionUnit remains centimetres. Assigning the entire DPI tuple to XResolution also emits a “too many entries” warning and discards its second value. For a new file, that leaves no YResolution for #10334 to read; with supplied EXIF it can leave an older YResolution in place. There is a related change for non-RGB modes: with I checked the exact writer and reader at |
Fixes #10344. The reader change in #10353 has been merged and included in this branch.
Saving RGB JPEGs with
keep_rgb=Trueand DPI currently emits both JFIF and Adobe markers, which makes libjpeg decode the saved RGB components as YCbCr and changes the colors. This was found through a Codex-assisted source investigation.The current implementation builds on @radarhere's replacement: for RGB images with
keep_rgb=True, store both DPI axes and their inch unit in EXIF instead of adding JFIF density. It copies supplied EXIF data before updating it. Other image modes keep their existing JFIF handling. The reader change in #10353 is included in the branch.Validation:
9a20ff03, 12 focused regression tests passed against the exact changed Python writer and reader using the existing Pillow 13.0 codec and an_Tilecompatibility adapter. They cover supplied EXIF in centimetres, both stored axes, other image modes, zero density and invalid DPI lengths. The same tests failed on the preceding PR head. This is not a full current-source build or test suite.The writer stores non-square EXIF DPI correctly, but Pillow's current reader reports the X value for both axes. The reader fix in #10334 needs to land first to avoid a non-square DPI readback regression.
Investigation, implementation and checks assisted by Codex.