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 configuring JPEG restart marker interval on save #7488

Merged
merged 1 commit into from
Nov 14, 2023

Conversation

bgilbert
Copy link
Contributor

If a JPEG includes restart markers, the decoder doesn't have to give up if it finds data corruption, but can scan forward to the next restart marker and resume decoding from there. Restart markers add overhead, but are useful in applications that want to support a limited form of error recovery or random access within a JPEG.

libjpeg allows specifying the marker interval either in MCU blocks or in MCU rows. Support both, via separate parameters, rather than requiring callers to do the math.

@bgilbert bgilbert force-pushed the jpeg-restart branch 2 times, most recently from 1474f56 to e56fbf4 Compare October 23, 2023 06:47
src/libImaging/Jpeg.h Outdated Show resolved Hide resolved
@radarhere
Copy link
Member

Just linking to some more information, https://github.com/libjpeg-turbo/libjpeg-turbo/blob/main/libjpeg.txt describes these features as

unsigned int restart_interval
int restart_in_rows
To emit restart markers in the JPEG file, set one of these nonzero. Set restart_interval to specify the exact interval in MCU blocks (samples in lossless mode). Set restart_in_rows to specify the interval in MCU rows. (If restart_in_rows is not 0, then restart_interval is set after the image width in MCUs is computed.) Defaults are zero (no restarts). One restart marker per MCU row is often a good choice. NOTE: the overhead of restart markers is higher in grayscale JPEG files than in color files, and MUCH higher in progressive JPEGs. If you use restarts, you may want to use larger intervals in those cases.

@bgilbert
Copy link
Contributor Author

Updated with feedback from bgilbert#2.

Tests/test_file_jpeg.py Outdated Show resolved Hide resolved
@radarhere
Copy link
Member

radarhere commented Oct 31, 2023

Just in a comment here, could you provide a short explanation for why you picked the names "restart_marker_blocks" and "restart_marker_rows" instead of continuing with libjpeg's names of "restart_interval" and "restart_in_rows"?

@bgilbert
Copy link
Contributor Author

I find the libjpeg names inconsistent and confusing, and tried to pick better ones. I'm open to other ideas though.

libjpeg allows specifying the marker interval either in MCU blocks or in
MCU rows.  Support both, via separate parameters, rather than requiring
callers to do the math.

Co-authored-by: Andrew Murray <radarhere@users.noreply.github.com>
@bgilbert
Copy link
Contributor Author

bgilbert commented Oct 31, 2023

Sorry, I missed the previous merge of main into this PR when force-pushing. The only changes to the PR are the test_file_jpeg.py ones in the net diff.

@bgilbert
Copy link
Contributor Author

Is this ready to go in?

@radarhere
Copy link
Member

I've approved it, so I raise no objections if it were to be merged in.

I'm holding off on actually merging for two reasons

  1. I'm not completely certain about renaming "restart_interval" and "restart_in_rows", as that is subjective, and maybe others have thoughts
  2. In case @homm has any further thoughts in general, as he has offered some thoughts already

@radarhere radarhere merged commit 4b308dc into python-pillow:main Nov 14, 2023
56 checks passed
@bgilbert bgilbert deleted the jpeg-restart branch November 14, 2023 12:58
radarhere added a commit to radarhere/Pillow that referenced this pull request Dec 8, 2023
radarhere added a commit to radarhere/Pillow that referenced this pull request Dec 8, 2023
@radarhere radarhere mentioned this pull request Dec 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants