Fix premature output of non-interleaved multi-scan JPEGs [chromium/src : main]
Florin Malita (Gerrit)
Florin Malita added 5 comments![]( "Open in Gerrit")
This change requires rebaselines for three jpeg images:
[12-55.jpg](https://chromium.googlesource.com/chromium/src.git/+/refs/heads/main/third_party/blink/web_tests/images/resources/jpegfuzz/12-55.jpg)
[182.jpg](https://chromium.googlesource.com/chromium/src.git/+/refs/heads/main/third_party/blink/web_tests/images/resources/jpegfuzz/182.jpg)
[23-55.jpg](https://chromium.googlesource.com/chromium/src.git/+/refs/heads/main/third_party/blink/web_tests/images/resources/jpegfuzz/23-55.jpg)
These are part of jpegfuzz and contain corrupted/truncated data (mainly missing some component scans). We are now rejecting them (jpeg_input_complete correctly returns false) instead of attempting to decode with missing components and producing junk pixels.
kJpegDecompressSequential, // Output sequential pixelsFlorin MalitaShould we change this comment to "Output pixels in sequential mode." for consistency?
Done
// If this is a progressive JPEG ...Florin MalitaDo we need to change the word "progressive" in this comment?
Done
[](const jpeg_decompress_struct& info) -> bool {Florin MalitaShould we remove the `const` here so that we don't need the `const_cast` at line 605?
Done
// images, all components are only seen when the input is complete.Florin MalitaJust for my own understanding: At this point, `info.coef_bits` is a null pointer. Could you explain why that implies the image is non-progressive?
Per `coef_bits` [docs](https://github.com/libjpeg-turbo/libjpeg-turbo/blob/c082e8697dfc4b6f8e2eacf0a23e41d98004ba65/src/jpeglib.h#L625): *This pointer is NULL when reading a non-progressive file.*
Related details
- Wan-Teh Chang
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Wan-Teh Chang (Gerrit)
Wan-Teh Chang added 1 comment![]( "Open in Gerrit")
return jpeg_input_complete(&info);IMPORTANT: I wonder if we should also test `info.input_scan_number == info.output_scan_number` here. See lines 670-672, reproduced here:
```
if (jpeg_input_complete(&info_) &&
(info_.input_scan_number == info_.output_scan_number)) {
break;
```
and the test file provided by DRC in https://crbug.com/504670493#comment10. Note that the condition DRC uses is NOT the negation of the condition above:
```
while (!jpeg_input_complete(&cinfo) &&
cinfo.input_scan_number != cinfo.output_scan_number) {
```
It would be good to get DRC's input on this.
Related details
- Florin Malita
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Wan-Teh Chang (Gerrit)
Wan-Teh Chang voted and added 2 comments![]( "Open in Gerrit")
Votes added by Wan-Teh Chang
| Code-Review | +1 |
2 comments
LGTM. Thanks!
// If this is a JPEG requiring buffered-image mode?Why was a question mark added?
Related details
- Florin Malita
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Wan-Teh Chang (Gerrit)
Wan-Teh Chang added 1 comment![]( "Open in Gerrit")
// images, all components are only seen when the input is complete.Florin MalitaJust for my own understanding: At this point, `info.coef_bits` is a null pointer. Could you explain why that implies the image is non-progressive?
Per `coef_bits` [docs](https://github.com/libjpeg-turbo/libjpeg-turbo/blob/c082e8697dfc4b6f8e2eacf0a23e41d98004ba65/src/jpeglib.h#L625): *This pointer is NULL when reading a non-progressive file.*
Thanks. Code Search shows that the `coef_bits` table is allocated in `jinit_phuff_decoder()`. `jinit_phuff_decoder()` is called under the following condition:
```
!cinfo->master->lossless && !cinfo->arith_code && cinfo->progressive_mode
```
Does this match your understanding?
Wan-Teh Chang (Gerrit)
Wan-Teh Chang added 1 comment![]( "Open in Gerrit")
all_components_seen(info_) ? info_.input_scan_number : 0;Moritz: You added the `first_scan_to_display` and `all_components_seen` code ([CL1](https://chromium-review.googlesource.com/c/chromium/src/+/2515401), [CL2](https://chromium-review.googlesource.com/c/chromium/src/+/2577498)). It would be good if you could take a look at this CL.
If we ignore the renaming changes, the only change to the logic in this CL is line 605 -- what `all_components_seen()` should return when `info.coef_bits` is a null pointer.
Since `all_components_seen()` is only used to set the `first_scan_to_display` variable, we can also take a step back and consider what value `first_scan_to_display` should be set to when `info.coef_bits` is a null pointer.
Related details
- Florin Malita
- Moritz Firsching
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |