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

Support saving JPEG comments #6774

Merged
merged 12 commits into from Dec 7, 2022
Merged

Conversation

smason
Copy link
Contributor

@smason smason commented Dec 2, 2022

Fixes #6773

Changes proposed in this pull request:

  • a JPEG COM segment can now be written during Image.save
  • opening a JPEG file and saving to another file will now copy any existing comment across
  • user can now do image.save(filename, format='JPEG', comment='comment to be written') and the comment will be written to a COM segment

@radarhere radarhere changed the title support round-tripping JPEG comments Support round-tripping JPEG comments Dec 2, 2022
@radarhere radarhere added the JPEG label Dec 2, 2022
@smason
Copy link
Contributor Author

smason commented Dec 3, 2022

not sure what's going on with that MSYS2 MinGW 32-bit test failure, but it looks unrelated

@radarhere
Copy link
Member

There are some intermittent failures in our CIs. I've rerun the job, and it has passed now.

@radarhere radarhere changed the title Support round-tripping JPEG comments Support saving JPEG comments Dec 4, 2022
Use jpeg_write_marker to write comment
* means `comment=None` can be passed directly
* no need to conditionally run `str.encode()`
* clean up checking of whether a comment is passed
src/encode.c Show resolved Hide resolved
@@ -278,7 +278,7 @@ ImagingJpegEncode(Imaging im, ImagingCodecState state, UINT8 *buf, int bytes) {

case 4:

if (context->comment_size > 0) {
if (context->comment) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

the rest of the code is conditional on comment != NULL rather than comment_size > 0

they should be the same, but keeping things consistent feels nicer to me

Copy link
Member

Choose a reason for hiding this comment

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

When you say "the rest of the code", what are you referring to?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sorry, that wasn't explained well

PyImaging_JpegEncoderNew seems to imply that comment_size is only valid when comment is non-NULL. e.g. comment is initialised to NULL, but comment_size isn't initialised to zero, and later in function (line 1109) it only sets comment = NULL

Free comment when returning early
@radarhere radarhere merged commit 378adeb into python-pillow:main Dec 7, 2022
mergify bot added a commit that referenced this pull request Dec 7, 2022
@smason
Copy link
Contributor Author

smason commented Dec 8, 2022

@radarhere thanks for all the help getting that merged!

sorry for the silence the past couple of days. I've been a bit ill, but recovering now

@smason smason deleted the write-jpeg-com branch December 12, 2022 13:28
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.

PIL doesn't write JPEG comment
2 participants