Add support for parsing the sBIT chunk in the decoder

[thirumurugan-git]

Implemented support for sBIT chunk parsing in the decoder.

Issue - #519

[thirumurugan-git]

@anforowicz Kindly review this PR.

[thirumurugan-git]

The following images fail in the image_source_chromaticities, roundtrip, and roundtrip_stream tests. This is because these images contain an sBIT chunk where the value exceeds the bit depth. Could you clarify whether I handled the decoding incorrectly, or if the images contain an invalid sBIT chunk?

tests/pngsuite/s01i3p01.png
tests/pngsuite/s01n3p01.png
tests/pngsuite/s02i3p01.png
tests/pngsuite/s02n3p01.png
tests/pngsuite/s03i3p01.png
tests/pngsuite/s03n3p01.png
tests/pngsuite/s04i3p01.png
tests/pngsuite/s04n3p01.png
tests/pngsuite/s05i3p02.png
tests/pngsuite/s05n3p02.png
tests/pngsuite/s06i3p02.png
tests/pngsuite/s06n3p02.png
tests/pngsuite/s07i3p02.png
tests/pngsuite/s07n3p02.png
tests/pngsuite/s08i3p02.png
tests/pngsuite/s08n3p02.png
tests/pngsuite/s09i3p02.png
tests/pngsuite/s09n3p02.png

[thirumurugan-git]

cargo fmt formats all the existing code, but it doesn't seem to match the current formatting style. Additionally, I don't see a rustfmt.toml file in the project. Could you let me know how I can format just the changed files and align them with the existing style?"

[anforowicz]

The following images fail in the image_source_chromaticities, roundtrip, and roundtrip_stream tests. This is because these images contain an sBIT chunk where the value exceeds the bit depth. Could you clarify whether I handled the decoding incorrectly, or if the images contain an invalid sBIT chunk?

...
tests/pngsuite/s08n3p02.png
...

https://www.w3.org/TR/2003/REC-PNG-20031110/#11IHDR says:

The sample depth is the same as the bit depth except in the case of indexed-colour PNG images (colour type 3), in which the sample depth is always 8 bits

Looking at s08n3p02.png as an example (see http://www.schaik.com/pngsuite/pngsuite_siz_png.html):

• IHDR.bits is 2, and IHDR.color_type is 3 (indexed) which means that IDAT uses 2 bits per pixel
• All pLTE entries always use 8 bits per R / G / B
• So sBIT chunk values should be capped at 8 (not 2)

I think the failures will go away if fn parse_sbit takes the above into account when calculating the bit_depth.

[anforowicz]

cargo fmt formats all the existing code, but it doesn't seem to match the current formatting style. Additionally, I don't see a rustfmt.toml file in the project. Could you let me know how I can format just the changed files and align them with the existing style?"

I am not sure what is going on :-(. FWIW, I tried running cargo fmt on top of this PR and got a reasonably looking result here: master...anforowicz:image-png:impl-sbit-chunk.

[thirumurugan-git]

@anforowicz PTAL, I've made the necessary changes. Just to inform you, I created the file //test/sbit/indexed.png by adding an sBIT chunk before the PLTE chunk in the paletted-zune.png image. While this isn't a valid PNG with respect to the sBIT standard, the goal of this PR is to expose sBIT data to the info struct, and the test PNG was created with that in mind. Is this acceptable? I think we should also incorporate sBIT handling in the encoder and add sample depth rescaling based on the sBIT chunk. This would allow us to generate example images using our encoder. Do you think we need this? @kornelski mentioned that sBIT isn't widely used on the web. What are your thoughts on adding it to the encoder? If so, I can create a new issue to address this.

[fintelia]

It isn't specific to this PR, but we really shouldn't be generating fatal errors in response to corrupt auxiliary chunks. Opened #525 to track.

Just to inform you, I created the file //test/sbit/indexed.png by adding an sBIT chunk before the PLTE chunk in the paletted-zune.png image. While this isn't a valid PNG with respect to the sBIT standard, the goal of this PR is to expose sBIT data to the info struct, and the test PNG was created with that in mind. Is this acceptable?

Why isn't it valid? The spec allows sBIT chunks with indexed color images.

I think we should also incorporate sBIT handling in the encoder and add sample depth rescaling based on the sBIT chunk. This would allow us to generate example images using our encoder. Do you think we need this? @kornelski mentioned that #519 (comment) on the web. What are your thoughts on adding it to the encoder?

Encoding can be a separate PR. Though I'm not sure sample depth rescaling makes sense in this crate. Might be better to require the user to pass already rescaled data.

[thirumurugan-git]

Why isn't it valid? The spec allows sBIT chunks with indexed color images.

I added a random sBIT chunk value to the original image, specifically 5, 6, and 5 for the R, G, and B channels. The original image wasn’t encoded with rescaled values, so I was wondering if it’s acceptable to have this modified PNG.

I’ve been thinking about how other decoders handle this and how the image is rendered by a rendering engine when the sBIT chunk is present. Would a consumer of this decoder rescale the sample depth based on the sBIT value and display the PNG with the rescaled values? If that’s the case, the displayed image could differ from the original pixel values, which led me to believe that the image might be invalid. If I’m mistaken, please correct me.

Though I'm not sure sample depth rescaling makes sense in this crate. Might be better to require the user to pass already rescaled data.

I looked through the libpng code and noticed it uses png_set_shift in its decoder for rescaling sample depth based on the sBIT values. From this, I thought it was necessary to include sample depth rescaling in both the encoder and decoder based on the sBIT value. However, if rescaling is optional, we could leave it for the consumer to handle instead.

I also referred to documentation about including this in our decoder. The doc mentions that Decoders may wish to scale PNG which seems optional.

[kornelski]

sBIT doesn't change how the image looks like at all. sBIT is purely informational hint.
There is never any shifting of pixel data done in response to it. That libpng function is for a custom API of their encoder, not a PNG feature.

[thirumurugan-git]

@kornelski, thanks for the explanation. Regarding the merging of this PR, are there any blocking changes we need to address?

[fintelia]

The indexed test image is 1 MB. We've already got some large images checked into the repository, but it would be better to avoid checking in more.

Haven't had a chance to look through the code changes in detail

[thirumurugan-git]

@fintelia / @anforowicz PTAL, I've uploaded the updated image for the indexed PNG.

[anforowicz]

@fintelia / @anforowicz PTAL, I've uploaded the updated image for the indexed PNG.

Thanks @thirumurugan-git!

I agree with @fintelia's comment at #524 (comment) that we shouldn't be generating fatal errors in response to corrupt auxiliary chunks. In particular, before this PR sBIT errors would be ignored, but after the current version of the PR they may result in DecodingErrors - this risks breaking users/scenarios that used to succeed with past versions of the crate. See #525 for additional discussion.

Can the PR be tweaked to make parse_sbit infallible (e.g. returning Decoded rather than Result<Decoded, DecodingError>)? This can be done by gracefully handling errors within the parse_sbit function (treating them as-if the chunk was missing altogether). FWIW let me share an example of what I've done when adding support for other chunk kinds in #529, although I am not sure if this would directly translate for sBIT chunk (since parsing sBIT doesn't seem to result in std::io::Errors... still, maybe the inner parse function can return Option<Cow<'a, [u8]>>).

Other than the error handling, the PR LGTM.