From d822d85af6d9b1c108650832d56b75cd86b2f5a9 Mon Sep 17 00:00:00 2001 From: Sam Mason Date: Fri, 2 Dec 2022 17:57:19 +0000 Subject: [PATCH 1/9] support round-tripping JPEG comments --- Tests/test_file_jpeg.py | 12 ++++++++++++ src/PIL/JpegImagePlugin.py | 10 ++++++++++ 2 files changed, 22 insertions(+) diff --git a/Tests/test_file_jpeg.py b/Tests/test_file_jpeg.py index fa96e425b8c..94ef595650b 100644 --- a/Tests/test_file_jpeg.py +++ b/Tests/test_file_jpeg.py @@ -87,6 +87,18 @@ def test_app(self): assert im.info["comment"] == b"File written by Adobe Photoshop\xa8 4.0\x00" + def test_com_write(self): + dummy_text = "this is a test comment" + with Image.open(TEST_FILE) as im: + with BytesIO() as buf: + im.save(buf, format="JPEG") + with Image.open(buf) as im2: + assert im.app['COM'] == im2.app['COM'] + with BytesIO() as buf: + im.save(buf, format="JPEG", comment=dummy_text) + with Image.open(buf) as im2: + assert im2.app['COM'].decode() == dummy_text + def test_cmyk(self): # Test CMYK handling. Thanks to Tim and Charlie for test data, # Michael for getting me to look one more time. diff --git a/src/PIL/JpegImagePlugin.py b/src/PIL/JpegImagePlugin.py index a6ed223bc6f..a6abe8b9f6c 100644 --- a/src/PIL/JpegImagePlugin.py +++ b/src/PIL/JpegImagePlugin.py @@ -44,6 +44,7 @@ from . import Image, ImageFile, TiffImagePlugin from ._binary import i16be as i16 from ._binary import i32be as i32 +from ._binary import o16be as o16 from ._binary import o8 from ._deprecate import deprecate from .JpegPresets import presets @@ -713,6 +714,15 @@ def validate_qtables(qtables): extra = info.get("extra", b"") + comment = info.get("comment") + if comment is None and isinstance(im, JpegImageFile): + comment = im.app.get('COM') + if comment: + if isinstance(comment, str): + comment = comment.encode() + size = o16(2 + len(comment)) + extra += b'\xFF\xFE%s%s' % (size, comment) + icc_profile = info.get("icc_profile") if icc_profile: ICC_OVERHEAD_LEN = 14 From e9f485849157100ddf75f289db6fcb509927706c Mon Sep 17 00:00:00 2001 From: "pre-commit-ci[bot]" <66853113+pre-commit-ci[bot]@users.noreply.github.com> Date: Fri, 2 Dec 2022 18:07:07 +0000 Subject: [PATCH 2/9] [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --- Tests/test_file_jpeg.py | 4 ++-- src/PIL/JpegImagePlugin.py | 6 +++--- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/Tests/test_file_jpeg.py b/Tests/test_file_jpeg.py index 94ef595650b..ffaf2cabaf4 100644 --- a/Tests/test_file_jpeg.py +++ b/Tests/test_file_jpeg.py @@ -93,11 +93,11 @@ def test_com_write(self): with BytesIO() as buf: im.save(buf, format="JPEG") with Image.open(buf) as im2: - assert im.app['COM'] == im2.app['COM'] + assert im.app["COM"] == im2.app["COM"] with BytesIO() as buf: im.save(buf, format="JPEG", comment=dummy_text) with Image.open(buf) as im2: - assert im2.app['COM'].decode() == dummy_text + assert im2.app["COM"].decode() == dummy_text def test_cmyk(self): # Test CMYK handling. Thanks to Tim and Charlie for test data, diff --git a/src/PIL/JpegImagePlugin.py b/src/PIL/JpegImagePlugin.py index a6abe8b9f6c..cb8a4e57fd8 100644 --- a/src/PIL/JpegImagePlugin.py +++ b/src/PIL/JpegImagePlugin.py @@ -44,8 +44,8 @@ from . import Image, ImageFile, TiffImagePlugin from ._binary import i16be as i16 from ._binary import i32be as i32 -from ._binary import o16be as o16 from ._binary import o8 +from ._binary import o16be as o16 from ._deprecate import deprecate from .JpegPresets import presets @@ -716,12 +716,12 @@ def validate_qtables(qtables): comment = info.get("comment") if comment is None and isinstance(im, JpegImageFile): - comment = im.app.get('COM') + comment = im.app.get("COM") if comment: if isinstance(comment, str): comment = comment.encode() size = o16(2 + len(comment)) - extra += b'\xFF\xFE%s%s' % (size, comment) + extra += b"\xFF\xFE%s%s" % (size, comment) icc_profile = info.get("icc_profile") if icc_profile: From 976ad5746a0155135337efa75648c550705215c0 Mon Sep 17 00:00:00 2001 From: Andrew Murray Date: Sat, 3 Dec 2022 09:29:02 +1100 Subject: [PATCH 3/9] Save comments from any image format by default --- src/PIL/JpegImagePlugin.py | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/PIL/JpegImagePlugin.py b/src/PIL/JpegImagePlugin.py index cb8a4e57fd8..c9de714d895 100644 --- a/src/PIL/JpegImagePlugin.py +++ b/src/PIL/JpegImagePlugin.py @@ -714,9 +714,7 @@ def validate_qtables(qtables): extra = info.get("extra", b"") - comment = info.get("comment") - if comment is None and isinstance(im, JpegImageFile): - comment = im.app.get("COM") + comment = info.get("comment", im.info.get("comment")) if comment: if isinstance(comment, str): comment = comment.encode() From c1d0a00943ee6fcc993f47047b798e2b6f9bac6f Mon Sep 17 00:00:00 2001 From: Andrew Murray Date: Sat, 3 Dec 2022 09:31:05 +1100 Subject: [PATCH 4/9] Use _binary instead of struct --- src/PIL/JpegImagePlugin.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/PIL/JpegImagePlugin.py b/src/PIL/JpegImagePlugin.py index c9de714d895..92dbb3193a0 100644 --- a/src/PIL/JpegImagePlugin.py +++ b/src/PIL/JpegImagePlugin.py @@ -732,7 +732,7 @@ def validate_qtables(qtables): icc_profile = icc_profile[MAX_DATA_BYTES_IN_MARKER:] i = 1 for marker in markers: - size = struct.pack(">H", 2 + ICC_OVERHEAD_LEN + len(marker)) + size = o16(2 + ICC_OVERHEAD_LEN + len(marker)) extra += ( b"\xFF\xE2" + size From 525c01143a8a4e0133908826577ccb54ed829a1b Mon Sep 17 00:00:00 2001 From: Andrew Murray Date: Sat, 3 Dec 2022 09:59:22 +1100 Subject: [PATCH 5/9] Test that comment is reread --- Tests/test_file_jpeg.py | 28 ++++++++++++++++++---------- 1 file changed, 18 insertions(+), 10 deletions(-) diff --git a/Tests/test_file_jpeg.py b/Tests/test_file_jpeg.py index ffaf2cabaf4..bb4ebb686f8 100644 --- a/Tests/test_file_jpeg.py +++ b/Tests/test_file_jpeg.py @@ -86,18 +86,26 @@ def test_app(self): assert len(im.applist) == 2 assert im.info["comment"] == b"File written by Adobe Photoshop\xa8 4.0\x00" + assert im.app["COM"] == im.info["comment"] - def test_com_write(self): - dummy_text = "this is a test comment" + def test_comment_write(self): with Image.open(TEST_FILE) as im: - with BytesIO() as buf: - im.save(buf, format="JPEG") - with Image.open(buf) as im2: - assert im.app["COM"] == im2.app["COM"] - with BytesIO() as buf: - im.save(buf, format="JPEG", comment=dummy_text) - with Image.open(buf) as im2: - assert im2.app["COM"].decode() == dummy_text + assert im.info["comment"] == b"File written by Adobe Photoshop\xa8 4.0\x00" + + # Test that existing comment is saved by default + out = BytesIO() + im.save(out, format="JPEG") + with Image.open(out) as reloaded: + assert im.info["comment"] == reloaded.info["comment"] + + # Test that a comment argument overrides the default comment + for comment in ("Test comment text", b"Text comment text"): + out = BytesIO() + im.save(out, format="JPEG", comment=comment) + with Image.open(out) as reloaded: + if not isinstance(comment, bytes): + comment = comment.encode() + assert reloaded.info["comment"] == comment def test_cmyk(self): # Test CMYK handling. Thanks to Tim and Charlie for test data, From 1ed1a3a971e127b55353bde39cb7ea6bedb45c04 Mon Sep 17 00:00:00 2001 From: Sam Mason Date: Sat, 3 Dec 2022 15:07:37 +0000 Subject: [PATCH 6/9] make sure passing a blank comment removes existing comment --- Tests/test_file_jpeg.py | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/Tests/test_file_jpeg.py b/Tests/test_file_jpeg.py index bb4ebb686f8..7a958c7da84 100644 --- a/Tests/test_file_jpeg.py +++ b/Tests/test_file_jpeg.py @@ -98,6 +98,13 @@ def test_comment_write(self): with Image.open(out) as reloaded: assert im.info["comment"] == reloaded.info["comment"] + # Ensure that a blank comment causes any existing comment to be removed + for comment in ("", b"", None): + out = BytesIO() + im.save(out, format="JPEG", comment=comment) + with Image.open(out) as reloaded: + assert "comment" not in reloaded.info + # Test that a comment argument overrides the default comment for comment in ("Test comment text", b"Text comment text"): out = BytesIO() From e50ae85ea406d86073ca88ffdec469e1e18d7527 Mon Sep 17 00:00:00 2001 From: Andrew Murray Date: Mon, 5 Dec 2022 13:57:26 +1100 Subject: [PATCH 7/9] Use jpeg_write_marker to write comment --- src/PIL/JpegImagePlugin.py | 12 +++++------- src/encode.c | 26 ++++++++++++++++++++++---- src/libImaging/Jpeg.h | 4 ++++ src/libImaging/JpegEncode.c | 13 ++++++++++++- 4 files changed, 43 insertions(+), 12 deletions(-) diff --git a/src/PIL/JpegImagePlugin.py b/src/PIL/JpegImagePlugin.py index 92dbb3193a0..7b5b32be0a2 100644 --- a/src/PIL/JpegImagePlugin.py +++ b/src/PIL/JpegImagePlugin.py @@ -714,13 +714,6 @@ def validate_qtables(qtables): extra = info.get("extra", b"") - comment = info.get("comment", im.info.get("comment")) - if comment: - if isinstance(comment, str): - comment = comment.encode() - size = o16(2 + len(comment)) - extra += b"\xFF\xFE%s%s" % (size, comment) - icc_profile = info.get("icc_profile") if icc_profile: ICC_OVERHEAD_LEN = 14 @@ -743,6 +736,10 @@ def validate_qtables(qtables): ) i += 1 + comment = info.get("comment", im.info.get("comment")) or b"" + if isinstance(comment, str): + comment = comment.encode() + # "progressive" is the official name, but older documentation # says "progression" # FIXME: issue a warning if the wrong form is used (post-1.1.7) @@ -765,6 +762,7 @@ def validate_qtables(qtables): dpi[1], subsampling, qtables, + comment, extra, exif, ) diff --git a/src/encode.c b/src/encode.c index 72c7f64d0a3..a2eae81fdae 100644 --- a/src/encode.c +++ b/src/encode.c @@ -1048,6 +1048,8 @@ PyImaging_JpegEncoderNew(PyObject *self, PyObject *args) { PyObject *qtables = NULL; unsigned int *qarrays = NULL; int qtablesLen = 0; + char *comment = NULL; + Py_ssize_t comment_size; char *extra = NULL; Py_ssize_t extra_size; char *rawExif = NULL; @@ -1055,7 +1057,7 @@ PyImaging_JpegEncoderNew(PyObject *self, PyObject *args) { if (!PyArg_ParseTuple( args, - "ss|nnnnnnnnOy#y#", + "ss|nnnnnnnnOy#y#y#", &mode, &rawmode, &quality, @@ -1067,6 +1069,8 @@ PyImaging_JpegEncoderNew(PyObject *self, PyObject *args) { &ydpi, &subsampling, &qtables, + &comment, + &comment_size, &extra, &extra_size, &rawExif, @@ -1090,12 +1094,24 @@ PyImaging_JpegEncoderNew(PyObject *self, PyObject *args) { return NULL; } - // Freed in JpegEncode, Case 5 + // Freed in JpegEncode, Case 6 qarrays = get_qtables_arrays(qtables, &qtablesLen); + if (comment && comment_size > 0) { + /* malloc check ok, length is from python parsearg */ + char *p = malloc(comment_size); // Freed in JpegEncode, Case 6 + if (!p) { + return ImagingError_MemoryError(); + } + memcpy(p, comment, comment_size); + comment = p; + } else { + comment = NULL; + } + if (extra && extra_size > 0) { /* malloc check ok, length is from python parsearg */ - char *p = malloc(extra_size); // Freed in JpegEncode, Case 5 + char *p = malloc(extra_size); // Freed in JpegEncode, Case 6 if (!p) { return ImagingError_MemoryError(); } @@ -1107,7 +1123,7 @@ PyImaging_JpegEncoderNew(PyObject *self, PyObject *args) { if (rawExif && rawExifLen > 0) { /* malloc check ok, length is from python parsearg */ - char *pp = malloc(rawExifLen); // Freed in JpegEncode, Case 5 + char *pp = malloc(rawExifLen); // Freed in JpegEncode, Case 6 if (!pp) { if (extra) { free(extra); @@ -1134,6 +1150,8 @@ PyImaging_JpegEncoderNew(PyObject *self, PyObject *args) { ((JPEGENCODERSTATE *)encoder->state.context)->streamtype = streamtype; ((JPEGENCODERSTATE *)encoder->state.context)->xdpi = xdpi; ((JPEGENCODERSTATE *)encoder->state.context)->ydpi = ydpi; + ((JPEGENCODERSTATE *)encoder->state.context)->comment = comment; + ((JPEGENCODERSTATE *)encoder->state.context)->comment_size = comment_size; ((JPEGENCODERSTATE *)encoder->state.context)->extra = extra; ((JPEGENCODERSTATE *)encoder->state.context)->extra_size = extra_size; ((JPEGENCODERSTATE *)encoder->state.context)->rawExif = rawExif; diff --git a/src/libImaging/Jpeg.h b/src/libImaging/Jpeg.h index a876d3bb6d9..1d755081871 100644 --- a/src/libImaging/Jpeg.h +++ b/src/libImaging/Jpeg.h @@ -92,6 +92,10 @@ typedef struct { /* in factors of DCTSIZE2 */ int qtablesLen; + /* Comment */ + char *comment; + size_t comment_size; + /* Extra data (to be injected after header) */ char *extra; int extra_size; diff --git a/src/libImaging/JpegEncode.c b/src/libImaging/JpegEncode.c index a44debcafe8..b6e3acc951c 100644 --- a/src/libImaging/JpegEncode.c +++ b/src/libImaging/JpegEncode.c @@ -277,6 +277,13 @@ ImagingJpegEncode(Imaging im, ImagingCodecState state, UINT8 *buf, int bytes) { } case 4: + + if (context->comment_size > 0) { + jpeg_write_marker(&context->cinfo, JPEG_COM, (unsigned char *)context->comment, context->comment_size); + } + state->state++; + + case 5: if (1024 > context->destination.pub.free_in_buffer) { break; } @@ -301,7 +308,7 @@ ImagingJpegEncode(Imaging im, ImagingCodecState state, UINT8 *buf, int bytes) { state->state++; /* fall through */ - case 5: + case 6: /* Finish compression */ if (context->destination.pub.free_in_buffer < 100) { @@ -310,6 +317,10 @@ ImagingJpegEncode(Imaging im, ImagingCodecState state, UINT8 *buf, int bytes) { jpeg_finish_compress(&context->cinfo); /* Clean up */ + if (context->comment) { + free(context->comment); + context->comment = NULL; + } if (context->extra) { free(context->extra); context->extra = NULL; From eddc9bdcecb6c5add58f284ab7497fd22dc99adb Mon Sep 17 00:00:00 2001 From: Sam Mason Date: Mon, 5 Dec 2022 17:46:54 +0000 Subject: [PATCH 8/9] switch to #z for comment parameter * means `comment=None` can be passed directly * no need to conditionally run `str.encode()` * clean up checking of whether a comment is passed --- src/PIL/JpegImagePlugin.py | 4 +--- src/encode.c | 2 +- src/libImaging/JpegEncode.c | 2 +- 3 files changed, 3 insertions(+), 5 deletions(-) diff --git a/src/PIL/JpegImagePlugin.py b/src/PIL/JpegImagePlugin.py index 7b5b32be0a2..ef0be669954 100644 --- a/src/PIL/JpegImagePlugin.py +++ b/src/PIL/JpegImagePlugin.py @@ -736,9 +736,7 @@ def validate_qtables(qtables): ) i += 1 - comment = info.get("comment", im.info.get("comment")) or b"" - if isinstance(comment, str): - comment = comment.encode() + comment = info.get("comment", im.info.get("comment")) # "progressive" is the official name, but older documentation # says "progression" diff --git a/src/encode.c b/src/encode.c index a2eae81fdae..d37cbfbcf71 100644 --- a/src/encode.c +++ b/src/encode.c @@ -1057,7 +1057,7 @@ PyImaging_JpegEncoderNew(PyObject *self, PyObject *args) { if (!PyArg_ParseTuple( args, - "ss|nnnnnnnnOy#y#y#", + "ss|nnnnnnnnOz#y#y#", &mode, &rawmode, &quality, diff --git a/src/libImaging/JpegEncode.c b/src/libImaging/JpegEncode.c index b6e3acc951c..2a24eff39ca 100644 --- a/src/libImaging/JpegEncode.c +++ b/src/libImaging/JpegEncode.c @@ -278,7 +278,7 @@ ImagingJpegEncode(Imaging im, ImagingCodecState state, UINT8 *buf, int bytes) { case 4: - if (context->comment_size > 0) { + if (context->comment) { jpeg_write_marker(&context->cinfo, JPEG_COM, (unsigned char *)context->comment, context->comment_size); } state->state++; From 1d780081a620b00007a8fe93db469e5759c86868 Mon Sep 17 00:00:00 2001 From: Andrew Murray Date: Tue, 6 Dec 2022 20:22:12 +1100 Subject: [PATCH 9/9] Free comment when returning early --- src/encode.c | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/encode.c b/src/encode.c index d37cbfbcf71..e6352cbfe1a 100644 --- a/src/encode.c +++ b/src/encode.c @@ -1113,6 +1113,9 @@ PyImaging_JpegEncoderNew(PyObject *self, PyObject *args) { /* malloc check ok, length is from python parsearg */ char *p = malloc(extra_size); // Freed in JpegEncode, Case 6 if (!p) { + if (comment) { + free(comment); + } return ImagingError_MemoryError(); } memcpy(p, extra, extra_size); @@ -1125,6 +1128,9 @@ PyImaging_JpegEncoderNew(PyObject *self, PyObject *args) { /* malloc check ok, length is from python parsearg */ char *pp = malloc(rawExifLen); // Freed in JpegEncode, Case 6 if (!pp) { + if (comment) { + free(comment); + } if (extra) { free(extra); }