Implement cICP chunk for PNG as per version 3 of the spec. [chromium/src : main]
Dale Curtis (Gerrit)
Attention is currently required from: Luca Versari, Peter Kasting, Wan-Teh Chang.
3 comments:
Patchset:
+pkasting for OWNERs review, +wtc who is most recently familiar with what we've done for AVIF.
File third_party/blink/renderer/platform/image-decoders/png/png_image_decoder.cc:
Patch Set #2, Line 205:
color_space.ToGfxColorSpace().GetAsFullRangeRGB().ToSkColorSpace();You support limited above, but this will always expand to full range. Did you mean to do that? Did you look over the limitations in ToSkColorSpace()?
File third_party/blink/renderer/platform/image-decoders/png/png_image_reader.cc:
Patch Set #2, Line 102:
png_set_keep_unknown_chunks(png_, 3,Does this have any memory concerns? I.e., are there lots of PNGs out there with a ton of metadata that would cause us to waste memory on?
To view, visit change 3705739. To unsubscribe, or for help writing mail filters, visit settings.
Luca Versari (Gerrit)
Attention is currently required from: Dale Curtis, Peter Kasting, Wan-Teh Chang.
2 comments:
File third_party/blink/renderer/platform/image-decoders/png/png_image_decoder.cc:
Patch Set #2, Line 205:
color_space.ToGfxColorSpace().GetAsFullRangeRGB().ToSkColorSpace();
You support limited above, but this will always expand to full range. […]
I did mean to do it, in the sense that I know AVIF does the same.
Is it the correct thing to do, that I'm less sure about ;)
File third_party/blink/renderer/platform/image-decoders/png/png_image_reader.cc:
Patch Set #2, Line 102:
png_set_keep_unknown_chunks(png_, 3,
Does this have any memory concerns? I.e. […]
This should only keep cICP unknown chunks, which should be 4 bytes, so I don't think this is a memory concern.
non-cICP unknown chunks are still discarded as before.
To view, visit change 3705739. To unsubscribe, or for help writing mail filters, visit settings.
Peter Kasting (Gerrit)
Attention is currently required from: Dale Curtis, Luca Versari, Wan-Teh Chang.
Patch set 2:Code-Review +1
Dale Curtis (Gerrit)
Attention is currently required from: Luca Versari, Wan-Teh Chang.
1 comment:
File third_party/blink/renderer/platform/image-decoders/png/png_image_decoder.cc:
Patch Set #2, Line 205:
color_space.ToGfxColorSpace().GetAsFullRangeRGB().ToSkColorSpace();
I did mean to do it, in the sense that I know AVIF does the same. […]
That works because we later expand everything to full range when we can't use the YUV decoding path. Here I think you'll need to do a limited to full range expansion on the actual pixel data like we do for avif.
To view, visit change 3705739. To unsubscribe, or for help writing mail filters, visit settings.
Dale Curtis (Gerrit)
File third_party/blink/renderer/platform/image-decoders/png/png_image_reader.cc:
Patch Set #2, Line 102:
png_set_keep_unknown_chunks(png_, 3,
This should only keep cICP unknown chunks, which should be 4 bytes, so I don't think this is a memor […]
Ah, sorry I misread this as enabling all unknown chunks.
To view, visit change 3705739. To unsubscribe, or for help writing mail filters, visit settings.
Luca Versari (Gerrit)
Attention is currently required from: Dale Curtis, Wan-Teh Chang.
1 comment:
File third_party/blink/renderer/platform/image-decoders/png/png_image_decoder.cc:
Patch Set #2, Line 205:
color_space.ToGfxColorSpace().GetAsFullRangeRGB().ToSkColorSpace();
That works because we later expand everything to full range when we can't use the YUV decoding path. […]
Ah, good to know, we didn't spot that.
I'd be somewhat tempted by just disallowing limited range RGB, at least for now -- I honestly find it hard to imagine such files would be ever produced.
Otherwise, is there some range conversion code that one could use?
To view, visit change 3705739. To unsubscribe, or for help writing mail filters, visit settings.
Eugene Kliuchnikov (Gerrit)
Attention is currently required from: Dale Curtis, Luca Versari, Wan-Teh Chang.
4 comments:
Patchset:
Thanks!
File third_party/blink/renderer/platform/image-decoders/png/png_image_decoder.cc:
Patch Set #2, Line 169:
if (strcmp(reinterpret_cast<const char*>(unknown_chunks[i].name), "cICP") !=Is it possible to "const auto& chunk = unknown_chunks[i];" here?
Patch Set #2, Line 184:
Per PNG specWould be nice to have a reference to the spec (before loop) for the context.
Patch Set #2, Line 185:
unknown_chunks[i].data[2]Along with "primaries" and "trc" would be great to have "m" and "f" variables for better readability.
To view, visit change 3705739. To unsubscribe, or for help writing mail filters, visit settings.
Chris Blume (Gerrit)
Attention is currently required from: Dale Curtis, Eugene Kliuchnikov, Luca Versari, Wan-Teh Chang.
5 comments:
File third_party/blink/renderer/platform/image-decoders/png/png_image_decoder.cc:
Patch Set #2, Line 184:
Per PNG spec
Would be nice to have a reference to the spec (before loop) for the context.
The current spec is only in editor's draft phase. Things are likely to change.
https://w3c.github.io/PNG-spec/#11cICP
Interestingly, that fragment "#11cICP" is probably one of the things that will change. So that link will break.As a side note, it might be good to mention why the PNG says m must be 0: It is for converting YUV to RGB and PNG doesn't currently support YUV.
Patch Set #2, Line 185:
unknown_chunks[i].data[2]
Along with "primaries" and "trc" would be great to have "m" and "f" variables for better readability […]
Agreed, but perhaps better names like |matrix_coefficients|.
You'll see "f" is actually named |range| below.Change "f" to |range|?
Patch Set #2, Line 205:
color_space.ToGfxColorSpace().GetAsFullRangeRGB().ToSkColorSpace();
Ah, good to know, we didn't spot that. […]
This might be less rare than you think.
I learned about this via BBC test/reference images. As you might imagine, the web is unlikely to use limited range but TV is quite likely to use it. And once you then throw in Chromium-based smart TVs, it becomes fairly common and important.
My understanding of the limited range is so an analog TV signal can hold a deep black for long enough that the TV says "ah, this is the back/front porch and I should start a new frame from the top". That means the broadcast image itself shouldn't hold that deep black for long enough (a few scan lines, IIRC).
I haven't worked much with TVs but I could imagine a scenario where we could cause a TV to malfunction based on incorrect handling of limited range.
Patch Set #2, Line 221:
// TODO(veluca): add support for iCCN.We'll likely not be adding a new iCCN chunk.
Rather, we're repurposing the existing iCCP chunk to honor ICC v4 profiles.The official PNG spec says iCCP should only be ICC v2 profiles. But many places (including Chromium) treat it as v4 profiles already, anyway.
One thing that may be changing is a UTF-8 BOM at the beginning of the profile name to indicate that the profile name is UTF-8 instead of Latin-1. (If we do this, we'll extend that behavior to other Latin-1-only chunks such as tEXt.
To view, visit change 3705739. To unsubscribe, or for help writing mail filters, visit settings.
Luca Versari (Gerrit)
Attention is currently required from: Chris Blume, Dale Curtis, Eugene Kliuchnikov, Wan-Teh Chang.
6 comments:
File third_party/blink/renderer/platform/image-decoders/png/png_image_decoder.cc:
Patch Set #2, Line 169:
if (strcmp(reinterpret_cast<const char*>(unknown_chunks[i].name), "cICP") !=
Is it possible to "const auto& chunk = unknown_chunks[i];" here?
Done
Patch Set #2, Line 184:
Per PNG spec
The current spec is only in editor's draft phase. Things are likely to change. […]
Done
Patch Set #2, Line 185:
unknown_chunks[i].data[2]
Agreed, but perhaps better names like |matrix_coefficients|. […]
Done
Change "f" to |range|?
Done
Patch Set #2, Line 205:
color_space.ToGfxColorSpace().GetAsFullRangeRGB().ToSkColorSpace();
This might be less rare than you think. […]
I would argue, however, that such handling of limited range would be independent of _image_ formats (because, for example, web pages themselves assume full range), and that such situations should be handled rather via a final conversion to limited range. However I am not very familiar with the composition pipeline of Chrome and as such probably dalecurtis@ is better positioned to express an opinion here.
My personal preference would be to try as much as possible to avoid new limited-range content to be created, ideally by disallowing it in the PNG spec (but that's a discussion for another place). I really struggle to see a meaningful usecase for limited-range PNG in a web browser (even if part of a smart TV).
If that is not an option, I'd argue for either delaying the implementation of limited range RGB to a future CL, or even a future libpng update; however if there is a simple way to handle this in Chrome itself, it could also be an option to do it in this CL.
Patch Set #2, Line 221:
// TODO(veluca): add support for iCCN.
We'll likely not be adding a new iCCN chunk. […]
I removed the comment for now. I don't *think* Chrome would need to do anything to support UTF-8 profile names, as I don't think it uses names for anything.
To view, visit change 3705739. To unsubscribe, or for help writing mail filters, visit settings.
Dale Curtis (Gerrit)
Attention is currently required from: Chris Blume, Eugene Kliuchnikov, Luca Versari, Wan-Teh Chang.
1 comment:
File third_party/blink/renderer/platform/image-decoders/png/png_image_decoder.cc:
Patch Set #2, Line 205:
color_space.ToGfxColorSpace().GetAsFullRangeRGB().ToSkColorSpace();I would argue, however, that such handling of limited range would be independent of _image_ formats […]
Are you able to construct a limited range RGB PNG using any tooling today?
IIUC limited range is fairly uncommon in non-YUV contexts, since PNG doesn't support YUV at this time, disabling support for it for now sgtm. You may want to print a DLOG(WARNING) so it's obvious it's not working properly to a future chrome dev.
You can use the same gfx::ColorTransform that we use in AVIF, but it's very slow. You may be able to just use Skia and specify a limited -> full YUV space of the same color space (i.e. BT709_Limited -> BT709_Full), but we'd need to test/check with skia folk since that may incur precision loss.
To view, visit change 3705739. To unsubscribe, or for help writing mail filters, visit settings.
Chris Blume
Chris Blume (Gerrit)
Attention is currently required from: Chris Blume, Eugene Kliuchnikov, Luca Versari, Wan-Teh Chang.
My power is out (likely until Friday) so my ability to respond is limited. Sorry about that. (Look up AEP in Columbus, Ohio for details of the outage.)
Am *I* able to create a limited range PNG? No.
But the BBC has tooling where they do exactly that for their reference images.
Maybe a good path forward is to put this behind a flag. That way no TVs could suddenly misbehave. Plus this is not yet a complete PNG spec anyway so it probably shouldn't be default behavior yet.
1 comment:
Patchset:
My power is out (likely until Friday) so my ability to respond is limited. Sorry about that. (Look up AEP in Columbus, Ohio for details of the outage.)
Am *I* able to create a limited range PNG? No.
But the BBC has tooling where they do exactly that for their reference images.
Maybe a good path forward is to put this behind a flag. That way no TVs could suddenly misbehave. Plus this is not yet a complete PNG spec anyway so it probably shouldn't be default behavior yet.
To view, visit change 3705739. To unsubscribe, or for help writing mail filters, visit settings.
Wan-Teh Chang (Gerrit)
Attention is currently required from: Chris Blume, Eugene Kliuchnikov, Luca Versari.
Patch set 3:Code-Review -1
The change is no longer submittable: Code-Review is unsatisfied now.
10 comments:
Patchset:
Please add a test.
File third_party/blink/renderer/platform/image-decoders/png/png_image_decoder.cc:
Patch Set #3, Line 175:
// First, validate the cICP chunk.Optional: Convert lines 175-212 to a function.
Patch Set #3, Line 176:
// cICP must be 4 bytes.IMPORTANT: We should link to the spec. It's okay to link to a draft of the spec.
The link that Chris Blume provided, https://w3c.github.io/PNG-spec/#11cICP, says:
The cICP chunk contains:
Colour Primaries 2 bytes
Transfer Function 2 bytes
Matrix Coefficients 2 bytes
Full Range Flag 1 byteThat means the cICP chunk is 7 bytes, not 4 bytes. Is the spec wrong?
UPDATE: I see you mentioned this in the commit message. https://github.com/w3c/PNG-spec/issues/129 should be added to the comment in the source code.
Patch Set #3, Line 178:
continue;I suggest we return nullptr if the cICP chunk is invalid.
Patch Set #3, Line 181:
primariesNit: primaries => colour primaries
Patch Set #3, Line 182:
transfer curveNit: In CICP, this field is called "transfer characteristics".
Patch Set #3, Line 187:
uint8_t range_u8 = chunk.data[2];IMPORTANT: This should be `chunk.data[3]`.
Note: You have the right index in line 198.
Patch Set #3, Line 198:
chunk.data[3] == 1Nit: change `chunk.data[3] == 1` to `range_u8`.
Nit: Did you mean to add this blank line?
File third_party/blink/renderer/platform/image-decoders/png/png_image_reader.cc:
Change 3 to the symbolic constant PNG_HANDLE_CHUNK_ALWAYS for better readability.
To view, visit change 3705739. To unsubscribe, or for help writing mail filters, visit settings.
Dale Curtis (Gerrit)
Attention is currently required from: Chris Blume, Eugene Kliuchnikov, Luca Versari.
1 comment:
Patchset:
My power is out (likely until Friday) so my ability to respond is limited. Sorry about that. […]
Yes limited range is extremely common in video contexts. I think it's important to differentiate if one can produce a limited range PNG in RGB though.
If tooling can't produce them (i.e. perhaps as an artifact of how the RGB encoding is done) a TODO is fine for now. If the spec allows them to be created they should eventually be handled.
To view, visit change 3705739. To unsubscribe, or for help writing mail filters, visit settings.
Chris Blume
Chris Blume (Gerrit)
Attention is currently required from: Chris Blume, Eugene Kliuchnikov, Luca Versari.
I think the link between YUV and limited range is causal. It is just whether or not dark darks and bright brights are allowed in the data vs. used as signaling (like a new frame starting), right?
As far as CICP size (4 vs. 7 bytes), I will continue to read H.273 once my power is restored. I don't have a print out.
1 comment:
Patchset:
I think the link between YUV and limited range is causal. It is just whether or not dark darks and bright brights are allowed in the data vs. used as signaling (like a new frame starting), right?
As far as CICP size (4 vs. 7 bytes), I will continue to read H.273 once my power is restored. I don't have a print out.
To view, visit change 3705739. To unsubscribe, or for help writing mail filters, visit settings.
Chris Blume
Chris Blume (Gerrit)
Attention is currently required from: Chris Blume, Eugene Kliuchnikov, Luca Versari.
Sorry, I meant to say the link is coincidence, not causal.
1 comment:
Patchset:
Sorry, I meant to say the link is coincidence, not causal.
To view, visit change 3705739. To unsubscribe, or for help writing mail filters, visit settings.
Dale Curtis (Gerrit)
Attention is currently required from: Chris Blume, Eugene Kliuchnikov, Luca Versari.
1 comment:
Patchset:
I think the link between YUV and limited range is causal. […]
Kind of, limited means the signal values are [16, 235] and full means [0, 255] -- we expand limited to full for display, so they will look the same. There's nothing that stops a container from lying about the range though as the color values are not generally checked.
Certainly limited range RGB can be constructed, it's uncommon though. If it's intended to be part of the spec the code should eventually support it.
However, if they can't be created by any current tooling the priority of doing so in this CL versus a followup leans more towards the followup side (since it's complicated). (Which is the only point I'm trying to make)
To view, visit change 3705739. To unsubscribe, or for help writing mail filters, visit settings.
Chris Blume
Chris Blume (Gerrit)
Attention is currently required from: Chris Blume, Eugene Kliuchnikov, Luca Versari.
Got ya.
If treating the image as full means it does not get expanded, that would mean no signal values are accidentally generated. So I think it is fine for now to always treat it as full and not expand.
I do know BBC has tooling and PNGs to test limited range. But
1.) This isn't normal. Normal web users won't see that for a long, long time. And
2.) If we aren't expanding values yet anyway, it should be fine.
1 comment:
Patchset:
Got ya.
If treating the image as full means it does not get expanded, that would mean no signal values are accidentally generated. So I think it is fine for now to always treat it as full and not expand.
I do know BBC has tooling and PNGs to test limited range. But
1.) This isn't normal. Normal web users won't see that for a long, long time. And
2.) If we aren't expanding values yet anyway, it should be fine.
To view, visit change 3705739. To unsubscribe, or for help writing mail filters, visit settings.
Luca Versari (Gerrit)
Attention is currently required from: Chris Blume, Dale Curtis, Eugene Kliuchnikov, Wan-Teh Chang.
12 comments:
Patchset:
Kind of, limited means the signal values are [16, 235] and full means [0, 255] -- we expand limited […]
Ack
Yes limited range is extremely common in video contexts. […]
Ack
File third_party/blink/renderer/platform/image-decoders/png/png_image_decoder.cc:
Patch Set #2, Line 205:
color_space.ToGfxColorSpace().GetAsFullRangeRGB().ToSkColorSpace();
Are you able to construct a limited range RGB PNG using any tooling today? […]
Added a warning.
File third_party/blink/renderer/platform/image-decoders/png/png_image_decoder.cc:
Patch Set #3, Line 175:
// First, validate the cICP chunk.Optional: Convert lines 175-212 to a function.
Done
Patch Set #3, Line 176:
// cICP must be 4 bytes.
IMPORTANT: We should link to the spec. It's okay to link to a draft of the spec. […]
I propose that we wait to submit this CL until the conversation at https://github.com/w3c/PNG-spec/issues/129 reaches a conclusion.
Patch Set #3, Line 178:
continue;I suggest we return nullptr if the cICP chunk is invalid.
Done
Patch Set #3, Line 181:
primariesNit: primaries => colour primaries
Done
Patch Set #3, Line 182:
transfer curveNit: In CICP, this field is called "transfer characteristics".
Done
Patch Set #3, Line 187:
uint8_t range_u8 = chunk.data[2];
IMPORTANT: This should be `chunk.data[3]`. […]
Ups, I got that wrong when addressing previous comments. Done.
Patch Set #3, Line 198:
chunk.data[3] == 1Nit: change `chunk.data[3] == 1` to `range_u8`.
Done
Nit: Did you mean to add this blank line?
Done
File third_party/blink/renderer/platform/image-decoders/png/png_image_reader.cc:
Change 3 to the symbolic constant PNG_HANDLE_CHUNK_ALWAYS for better readability.
Ah, it wasn't clear to me from the PNG doc that the macro was defined. Done.
To view, visit change 3705739. To unsubscribe, or for help writing mail filters, visit settings.
Dale Curtis (Gerrit)
Attention is currently required from: Chris Blume, Eugene Kliuchnikov, Luca Versari, Wan-Teh Chang.
1 comment:
File third_party/blink/renderer/platform/image-decoders/png/png_image_decoder.cc:
Patch Set #4, Line 214:
png_unknown_chunkp unknown_chunks;Should cicp be preferred over ICC?
To view, visit change 3705739. To unsubscribe, or for help writing mail filters, visit settings.
Chris Blume
Chris Blume (Gerrit)
Attention is currently required from: Chris Blume, Eugene Kliuchnikov, Luca Versari, Wan-Teh Chang.
Yes, CICP should be preferred over ICC. This was agreed upon by the Color for the Web Community Group.
With the exception of when we know the display/surface do not support the desired code point. Then it should ignore the cICP chunk and continue down the list of priorities.
(I believe it is okay to say "I don't know if it is supported or not. I'll assume so.")
1 comment:
Patchset:
Yes, CICP should be preferred over ICC. This was agreed upon by the Color for the Web Community Group.
With the exception of when we know the display/surface do not support the desired code point. Then it should ignore the cICP chunk and continue down the list of priorities.
(I believe it is okay to say "I don't know if it is supported or not. I'll assume so.")
To view, visit change 3705739. To unsubscribe, or for help writing mail filters, visit settings.
Luca Versari (Gerrit)
Attention is currently required from: Chris Blume, Dale Curtis, Eugene Kliuchnikov, Wan-Teh Chang.
2 comments:
Patchset:
Yes, CICP should be preferred over ICC. […]
If I understand the semantics of the construction of a ColorSpace correctly, cICP specifying an unsupported-by-chrome configuration will result in the cICP chunk being ignored, which I believe implements exactly these semantics.
File third_party/blink/renderer/platform/image-decoders/png/png_image_decoder.cc:
Patch Set #4, Line 214:
png_unknown_chunkp unknown_chunks;Should cicp be preferred over ICC?
Ack
To view, visit change 3705739. To unsubscribe, or for help writing mail filters, visit settings.
Wan-Teh Chang (Gerrit)
Attention is currently required from: Chris Blume, Eugene Kliuchnikov, Luca Versari.
Patch set 4:Code-Review -1
2 comments:
File third_party/blink/renderer/platform/image-decoders/png/png_image_decoder.cc:
if (strcmp(reinterpret_cast<const char*>(chunk.name), "cICP") != 0) {
return nullptr;
}I suggest we move these three lines (165-167) back to the caller (with modification, see my comment at line 222). Then, if this function returns nullptr, it means we have an invalid cICP chunk, and the caller can choose to handle that as an error rather than ignore it.
std::unique_ptr<ColorProfile> cicp_color_profile =
ParseCicpChunk(unknown_chunks[i]);
if (cicp_color_profile) {
return cicp_color_profile;
}Please see my comment at line 167.
I suggest we rewrite the for loop's body as follows:
```
const auto& chunk = unknown_chunks[i];
if (strcmp(reinterpret_cast<const char*>(chunk.name), "cICP") == 0) {
return ParseCicpChunk(chunk);
}
```Note that this treats an invalid cICP chunk as an error. You can tweak this if you want to silently ignore an invalid cICP chunk.
To view, visit change 3705739. To unsubscribe, or for help writing mail filters, visit settings.
Chris Blume (Gerrit)
Attention is currently required from: Eugene Kliuchnikov, Luca Versari.
1 comment:
Patchset:
I carefully thumbed through https://source.chromium.org/chromium/chromium/src/+/main:media/base/video_color_space.h;l=14?q=media::VideoColorSpace&ss=chromium which says it follows ISO 23001-8:2016. I then compared its values to H.273. They're nearly the same.
However, H.273 specifies matrix coefficients 12, 13, and 14 which aren't listed here. So if we're strictly matching up against those values, we might run into some problems.
To view, visit change 3705739. To unsubscribe, or for help writing mail filters, visit settings.
Luca Versari (Gerrit)
Attention is currently required from: Eugene Kliuchnikov, Wan-Teh Chang.
3 comments:
Patchset:
Please add a test.
Done
File third_party/blink/renderer/platform/image-decoders/png/png_image_decoder.cc:
if (strcmp(reinterpret_cast<const char*>(chunk.name), "cICP") != 0) {
return nullptr;
}
I suggest we move these three lines (165-167) back to the caller (with modification, see my comment […]
Done
std::unique_ptr<ColorProfile> cicp_color_profile =
ParseCicpChunk(unknown_chunks[i]);
if (cicp_color_profile) {
return cicp_color_profile;
}
Please see my comment at line 167. […]
Done
To view, visit change 3705739. To unsubscribe, or for help writing mail filters, visit settings.
Wan-Teh Chang (Gerrit)
Attention is currently required from: Eugene Kliuchnikov, Luca Versari.
Patch set 5:Code-Review +1
1 comment:
File third_party/blink/renderer/platform/image-decoders/png/png_image_decoder.cc:
Patch Set #5, Line 163:
// Returns nullptr if the cICP chunk is invalid.Nit: Add "if the color profile it describes is not supported"? See the comment at line 219.
To view, visit change 3705739. To unsubscribe, or for help writing mail filters, visit settings.
Luca Versari (Gerrit)
Attention is currently required from: Eugene Kliuchnikov.
1 comment:
File third_party/blink/renderer/platform/image-decoders/png/png_image_decoder.cc:
Patch Set #5, Line 163:
// Returns nullptr if the cICP chunk is invalid.Nit: Add "if the color profile it describes is not supported"? See the comment at line 219.
Done
To view, visit change 3705739. To unsubscribe, or for help writing mail filters, visit settings.
Wan-Teh Chang (Gerrit)
Attention is currently required from: Eugene Kliuchnikov, Luca Versari.
Patch set 6:Code-Review +1
1 comment:
Patchset:
LGTM.
To view, visit change 3705739. To unsubscribe, or for help writing mail filters, visit settings.
Luca Versari (Gerrit)
File third_party/blink/renderer/platform/image-decoders/png/png_image_decoder.cc:
Patch Set #3, Line 176:
// cICP must be 4 bytes.I propose that we wait to submit this CL until the conversation at https://github. […]
I added the link, and the linked issue was resolved.
To view, visit change 3705739. To unsubscribe, or for help writing mail filters, visit settings.
Wan-Teh Chang (Gerrit)
Attention is currently required from: Eugene Kliuchnikov, Luca Versari, Peter Kasting.
Patch set 7:Code-Review +1
1 comment:
Patchset:
LGTM.
Peter, would you like to take another look at this CL?
To view, visit change 3705739. To unsubscribe, or for help writing mail filters, visit settings.
Peter Kasting (Gerrit)
Attention is currently required from: Eugene Kliuchnikov, Luca Versari.
Patch set 7:Code-Review +1
4 comments:
Patchset:
Still LG
File third_party/blink/renderer/platform/image-decoders/png/png_image_decoder.cc:
Patch Set #7, Line 166:
inline std::unique_ptr<ColorProfile> ParseCicpChunk(Does this really need to be marked inline?
Also, if this is a file-scope function, can we define in an anonymous namespace atop the file?
Patch Set #7, Line 194:
DLOG(WARNING) << "Limited range RGB is not fully supported";Is there a bug on this we can reference in a TODO?
Patch Set #7, Line 223:
if (!cicp_color_profile) {Nit: Seems like this would be clearer as:
if (cicp_color_profile)
return cicp_color_profile;
To view, visit change 3705739. To unsubscribe, or for help writing mail filters, visit settings.
Luca Versari (Gerrit)
Attention is currently required from: Eugene Kliuchnikov.
3 comments:
File third_party/blink/renderer/platform/image-decoders/png/png_image_decoder.cc:
Patch Set #7, Line 166:
inline std::unique_ptr<ColorProfile> ParseCicpChunk(
Does this really need to be marked inline? […]
Done. FWIW, the function was `inline` because `ReadColorProfile` was `inline`.
Patch Set #7, Line 194:
DLOG(WARNING) << "Limited range RGB is not fully supported";Is there a bug on this we can reference in a TODO?
Done
Patch Set #7, Line 223:
if (!cicp_color_profile) {
Nit: Seems like this would be clearer as: […]
Done
To view, visit change 3705739. To unsubscribe, or for help writing mail filters, visit settings.
Wan-Teh Chang (Gerrit)
Attention is currently required from: Eugene Kliuchnikov, Luca Versari.
Patch set 8:Code-Review +1
4 comments:
File third_party/blink/renderer/platform/image-decoders/png/png_image_decoder.cc:
Patch Set #8, Line 54:
namespace {Existing code uses `static` to mark file-scope functions. I wonder if we should also just declare ParseCicpChunk() as static for consistency.
Patch Set #8, Line 102:
} // namespaceNit: I suggest adding a blank line after line 54 and before this line, like so:
```
namespace {// Returns nullptr if the cICP chunk is invalid, or if it describes an
// unsupported color profile.
// See https://w3c.github.io/PNG-spec/#11cICP for the definition of this chunk.
std::unique_ptr<ColorProfile> ParseCicpChunk(const png_unknown_chunk& chunk) {
...
}} // namespace
```Patch Set #8, Line 213:
inline std::unique_ptr<ColorProfile> ReadColorProfile(png_structp png,I believe ReadColorProfile() is also used in file scope only, even though it's not declared as static.
Patch Set #8, Line 393:
static inline void SetRGBAPremultiplyRowNeon(png_bytep src_ptr,Note these file-scope static functions.
To view, visit change 3705739. To unsubscribe, or for help writing mail filters, visit settings.
Luca Versari (Gerrit)
Attention is currently required from: Eugene Kliuchnikov.
3 comments:
File third_party/blink/renderer/platform/image-decoders/png/png_image_decoder.cc:
Patch Set #8, Line 54:
namespace {
Existing code uses `static` to mark file-scope functions. […]
Done
Patch Set #8, Line 102:
} // namespace
Nit: I suggest adding a blank line after line 54 and before this line, like so: […]
Ack
Patch Set #8, Line 213:
inline std::unique_ptr<ColorProfile> ReadColorProfile(png_structp png,
I believe ReadColorProfile() is also used in file scope only, even though it's not declared as stati […]
I marked this one as static too.
To view, visit change 3705739. To unsubscribe, or for help writing mail filters, visit settings.
Wan-Teh Chang (Gerrit)
Attention is currently required from: Eugene Kliuchnikov, Luca Versari.
Patch set 9:Code-Review +1
1 comment:
Patchset:
LGTM.
To view, visit change 3705739. To unsubscribe, or for help writing mail filters, visit settings.
Chris Blume (Gerrit)
Attention is currently required from: Eugene Kliuchnikov, Luca Versari.
Patch set 9:Code-Review +1
1 comment:
Patchset:
Although I'm not an OWNER here, LGTM as well.
To view, visit change 3705739. To unsubscribe, or for help writing mail filters, visit settings.
Luca Versari (Gerrit)
Attention is currently required from: Eugene Kliuchnikov, Luca Versari.
Patch set 9:Commit-Queue +2
Luca Versari (Gerrit)
Attention is currently required from: Eugene Kliuchnikov, Luca Versari.
Patch set 9:Commit-Queue +2
To view, visit change 3705739. To unsubscribe, or for help writing mail filters, visit settings.
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted this change.
Implement cICP chunk for PNG as per version 3 of the spec.
See also https://github.com/w3c/PNG-spec/issues/129 for an editorial
issue in the PNG specification wrt the size of the fields of the cICP
chunk.
PAIR=sbou...@google.com
Change-Id: Ia7717f161bd1569691e07c9216957814f2c073f0
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3705739
Reviewed-by: Wan-Teh Chang <w...@google.com>
Reviewed-by: Chris Blume <cbl...@chromium.org>
Commit-Queue: Luca Versari <vel...@google.com>
Reviewed-by: Peter Kasting <pkas...@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1017577}
---
M third_party/blink/renderer/platform/image-decoders/png/png_image_decoder.cc
M third_party/blink/renderer/platform/image-decoders/png/png_image_decoder_test.cc
M third_party/blink/renderer/platform/image-decoders/png/png_image_reader.cc
A third_party/blink/web_tests/images/resources/cicp_pq.png
4 files changed, 119 insertions(+), 8 deletions(-)