-
-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
base: main
Are you sure you want to change the base?
Conversation
0e87938
to
654bb3c
Compare
The setting could also be |
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? |
This is the last JPEG setting I'm currently planning. I've been submitting them sequentially to avoid merge conflicts in 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 |
Would it make sense to have one setting that takes a list of segment names to enable/disable? |
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. |
cd3bc91
to
afb2b93
Compare
afb2b93
to
b29b67e
Compare
Whoops, didn't notice the branch update before force-pushing. I've now rebased onto |
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.
b29b67e
to
8053d5e
Compare
Done. Before we get too far into detailed review, could I get some maintainer feedback about the high-level approach? Should I continue with |
Could you provide an example how to use |
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", |
There was a problem hiding this comment.
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)
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.