Skip to content

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
@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.

@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 <[email protected]>
@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
@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.

3 participants