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

Add support for parsing cICP chunks. #529

Merged
merged 1 commit into from
Nov 1, 2024

Conversation

anforowicz
Copy link
Contributor

This commit is needed for parity with the existing PNG decoder in Blink / Chromium - see https://crbug.com/376758571.

I think that we should opaquely return u8 for color_primaries and transfer_function. Being opaque will help with extensibility (in case ITU-T-H.273 gets augmented in the future with other table entries / configurations).

I am less sure about the other 2 fields:

  • I think that we should also represent matrix_coefficients as u8 and return it even though the parser rejects anything other than 0 (same as blink::PNGImageDecoder in Chromium - see here).
  • I think it is okay to represent video_full_range_flag as an enum.
    • AFAICT only 2 values are supported (see the spec snippet here). And in that case having named values is nice.
    • The spec says that "each of the fields of the cICP chunk corresponds to the parameter of the same name in ITU-T-H.273", but I wasn't able to find "Video Full Range Flag" in there (or in an earlier revision from 2017) when searching for various keywords, including "range" and "narrow".

@fintelia
Copy link
Contributor

fintelia commented Nov 1, 2024

I think that we should also represent matrix_coefficients as u8 and return it even though the parser rejects anything other than 0 (same as blink::PNGImageDecoder in Chromium - see here).

Seems reasonable.

The spec says that "each of the fields of the cICP chunk corresponds to the parameter of the same name in ITU-T-H.273", but I wasn't able to find "Video Full Range Flag" in there (or in an earlier revision from 2017) when searching for various keywords, including "range" and "narrow".

It is described in section 8.3 of the 7/24 version of ITU-T-H.273 under the name "VideoFullRangeFlag". The values don't seem to have names and are just listed as 0 and 1.

I think it might be worth encoding it just as a bool rather than an enum. Honestly, given that narrow range images are extremely rare we might even want to add a note that users probably shouldn't bother handling them.

This commit is needed for parity with the existing PNG decoder
in Blink / Chromium - see https://crbug.com/376758571.
@anforowicz
Copy link
Contributor Author

The spec says that "each of the fields of the cICP chunk corresponds to the parameter of the same name in ITU-T-H.273", but I wasn't able to find "Video Full Range Flag" in there (or in an earlier revision from 2017) when searching for various keywords, including "range" and "narrow".

It is described in section 8.3 of the 7/24 version of ITU-T-H.273 under the name "VideoFullRangeFlag". The values don't seem to have names and are just listed as 0 and 1.

Ack.

I think it might be worth encoding it just as a bool rather than an enum. Honestly, given that narrow range images are extremely rare we might even want to add a note that users probably shouldn't bother handling them.

Thanks for the feedback. Done (changed to bool + tweaked the name of the last field; and also tweaked the doc comments of the last 2 fields).

@fintelia fintelia merged commit e912074 into image-rs:master Nov 1, 2024
19 checks passed
@anforowicz anforowicz deleted the cicp-chunk branch November 6, 2024 17:34
aarongable pushed a commit to chromium/chromium that referenced this pull request Nov 19, 2024
This CL patches the `png` crate in a way that corresponds to the
following upstream PRs:

* image-rs/image-png#526 - baseline to make
  subsequent patches easier to apply
* image-rs/image-png#528 - `mDCv` and `cLLI`
* image-rs/image-png#529 - `cICP`

This CL is a prerequisite for rolling Skia beyond
http://review.skia.org/917216

Bug: chromium:376758571
Bug: chromium:376550658
Change-Id: I5a70fccd71ae4295b62f7291a5fd3809f0d53860
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5999738
Commit-Queue: Łukasz Anforowicz <lukasza@chromium.org>
Reviewed-by: Adrian Taylor <adetaylor@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1385066}
@karip karip mentioned this pull request Nov 20, 2024
hubot pushed a commit to google/skia that referenced this pull request Nov 21, 2024
This CL propagates the data from the `cICP` chunk into
`SkEncodedInfo::ICCProfile`.

Dependencies:

* This CL unblocks enabling the `PNGTests.cicp` test in the Chromium CL
  at https://crrev.com/c/6013356.
* This CL depends on the new `SkColorSpace::MakeCICP` API that was
  recently added in https://crrev.com/c/5999738.

This CL patches the `png` crate in a way that corresponds to the
following upstream PRs (rolling this CL into Chromium depends on
https://crrev.com/c/5999738 which included the same patches):

* image-rs/image-png#526 - baseline to make
  subsequent patches easier to apply
* image-rs/image-png#528 - `mDCv` and `cLLI`
* image-rs/image-png#529 - `cICP`

Bug: chromium:376758571
Change-Id: I1e550b1eba4d629ff4337d85daa5656ac03f3a5a
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/917216
Commit-Queue: Łukasz Anforowicz <lukasza@google.com>
Reviewed-by: Daniel Dilan <danieldilan@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants