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

Allow disabling default emission of JPEG APP0 and APP14 segments #7679

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

Conversation

bgilbert
Copy link
Contributor

@bgilbert bgilbert commented Jan 2, 2024

When embedding JPEGs into a container file format, it may be desirable to minimize JPEG metadata, since the container will include the pertinent details. By default, libjpeg emits a JFIF APP0 segment for JFIF-compatible colorspaces (grayscale or YCbCr) and Adobe APP14 otherwise. Add a no_default_app_segments option to disable these.

#4639 added code to force emission of the JFIF segment if the DPI is specified, even for JFIF-incompatible colorspaces. This seems inconsistent with the JFIF spec, but apparently other software does it too. no_default_app_segments does not disable this behavior, since it only happens when the application explicitly specifies the DPI.

@radarhere radarhere added the JPEG label Jan 2, 2024
@bgilbert bgilbert force-pushed the jpeg-app-segments branch 2 times, most recently from 0e87938 to 654bb3c Compare January 2, 2024 09:55
@bgilbert
Copy link
Contributor Author

bgilbert commented Jan 2, 2024

The setting could also be default_app_segments, defaulting to True.

@radarhere
Copy link
Member

Recently, #7488 added "restart_marker_blocks" and "restart_marker_rows", and #7553 added "keep_rgb". Those were functional changes, whereas this is about saving a few bytes and grouping two libjpeg settings into one, which feels a bit more arbitrary (I'm afraid that a user will come along and say that they would like to set "write_JFIF_header" to false but not "write_Adobe_marker").

To try and prevent waking up one day and finding an overabundance of settings when saving JPEGs, and being unable to rearrange them without breaking backwards compatibility, can I ask - do you have a planned list of JPEG settings that you're thinking of adding in future PRs?

@bgilbert
Copy link
Contributor Author

bgilbert commented Jan 3, 2024

This is the last JPEG setting I'm currently planning. I've been submitting them sequentially to avoid merge conflicts in PyImaging_JpegEncoderNew(), partly because I didn't understand how long review would take. (Not complaining. It's best to get API changes correct.)

This change indeed seems a bit more obscure than the other two, and I'm okay skipping it if desired. It's possible to remove the APP segments outside of Pillow by postprocessing the byte stream. (Assuming performance isn't a major concern, which is true for my use case.)

Alternatively, if the setting is too broad, I could split it into two tri-states such as write_jfif_segment and write_adobe_segment so the caller can force either segment on or off.

@Yay295
Copy link
Contributor

Yay295 commented Jan 3, 2024

Would it make sense to have one setting that takes a list of segment names to enable/disable?
exclude_app_segments = ('APP0','APP14')

@bgilbert
Copy link
Contributor Author

bgilbert commented Jan 3, 2024

I believe JFIF and Adobe are the only APP segments supported natively by libjpeg, so those would be the only two supported values unless Pillow adds its own segments.

src/libImaging/Jpeg.h Outdated Show resolved Hide resolved
@bgilbert
Copy link
Contributor Author

bgilbert commented Jan 7, 2024

Whoops, didn't notice the branch update before force-pushing. I've now rebased onto main. The net diff of the force-pushes is here.

Tests/test_file_jpeg.py Outdated Show resolved Hide resolved
When embedding JPEGs into a container file format, it may be desirable
to minimize JPEG metadata, since the container will include the pertinent
details.  By default, libjpeg emits a JFIF APP0 segment for JFIF-
compatible colorspaces (grayscale or YCbCr) and Adobe APP14 otherwise.
Add a no_default_app_segments option to disable these.

660894c added code to force emission of the JFIF segment if the DPI is
specified, even for JFIF-incompatible colorspaces.  This seems
inconsistent with the JFIF spec, but apparently other software does it
too.  no_default_app_segments does not disable this behavior, since it
only happens when the application explicitly specifies the DPI.
@bgilbert
Copy link
Contributor Author

bgilbert commented Jan 8, 2024

Done.

Before we get too far into detailed review, could I get some maintainer feedback about the high-level approach? Should I continue with no_default_app_segments, invert to default_app_segments, split into two options, drop the PR, something else?

@homm
Copy link
Member

homm commented Jan 9, 2024

When embedding JPEGs into a container file format

Could you provide an example how to use no_default_app_segments to embed in container and which containers this could be?

@bgilbert
Copy link
Contributor Author

Something like:

start = fp.tell()
img.save(fp, format='JPEG', quality='web_medium', streamtype=2, no_default_app_segments=True)
end = fp.tell()

Container metadata, including JPEG tables, can then be written afterward as appropriate. My immediate use case is for a TIFF builder (I need to be able to write non-standards-conforming TIFFs), and DICOM datasets are also on my radar. Any file format designed to embed multiple JPEGs can potentially use this.

@@ -88,6 +88,31 @@ def test_app(self):
assert im.info["comment"] == b"File written by Adobe Photoshop\xa8 4.0\x00"
assert im.app["COM"] == im.info["comment"]

@pytest.mark.parametrize(
"keep_rgb, no_default_app_segments, expect_app0, expect_app14",
Copy link
Member

Choose a reason for hiding this comment

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

Could you also add a case with dpi? (JFIF should present as documentation said)

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.

None yet

5 participants