Implement CICP Chunk described in PNG 3rd Edition (Priority 1) #508
Comments
|
Spamming the issues list seems inappropriate. |
|
Sorry, I thought I was doing what you requested, describing the specific issues.
***************
“Believe me it's a lot of work. I can suggest other approaches but it's off topic for this issue which, well, what is the issue @digitaltvguy<https://urldefense.com/v3/__https:/github.com/digitaltvguy__;!!PIZeeW5wscynRQ!tyDD7qbD21PEK7W8Fmszb6p46QwMgsSzOqppPMmVeWTY1OOmJWFIOsVMwFYbRHGQYfDtnOQGQWIJfzvGfxoPgXnnIQ$>?”
***************
How can I help?
…-Chris (digitaltvguy)
From: John Bowler ***@***.***>
Date: Friday, December 22, 2023 at 7:13 PM
To: glennrp/libpng ***@***.***>
Cc: Seeger, Chris (NBCUniversal) ***@***.***>, Author ***@***.***>
Subject: [EXTERNAL] Re: [glennrp/libpng] Implement CICP Chunk described in PNG 3rd Edition (Priority 1) (Issue #508)
Spamming the issues list seems inappropriate.
—
Reply to this email directly, view it on GitHub<https://urldefense.com/v3/__https:/github.com/glennrp/libpng/issues/508*issuecomment-1868144939__;Iw!!PIZeeW5wscynRQ!vRjPoeJ1qQsVwIKgogIH8U4J2y2ScXD8LUJpyHkAB4viFG_Qteg-f-VYjRF52gMQll9fslmocghOLifNYZxd_Rra2w$>, or unsubscribe<https://urldefense.com/v3/__https:/github.com/notifications/unsubscribe-auth/AANIO6HQBE5ETROJBJIXTLLYKYOZHAVCNFSM6AAAAABBAMB7NKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQNRYGE2DIOJTHE__;!!PIZeeW5wscynRQ!vRjPoeJ1qQsVwIKgogIH8U4J2y2ScXD8LUJpyHkAB4viFG_Qteg-f-VYjRF52gMQll9fslmocghOLifNYZwATeWMJQ$>.
You are receiving this because you authored the thread.Message ID: ***@***.***>
|
Do what I suggested to Chris Blume (@programmax) - submit a pull request. |
|
Well, ok. Don't respond from your inbox: go to github.com and enter responses there. |
|
We're looking for help from this community in translating the functions of the cICP chunk described in PNG 3rd Edition to libping. Here is the temporary location for our conformance files with the new chunks: https://www.dropbox.com/scl/fo/a44vfxw3p21kvkh8nywpc/h?rlkey=v63ekq5y49kd4omg59ng9q3zr&dl=0 Here is PNG 3rd Edition: https://www.w3.org/TR/png-3/ |
|
I'll say it exactly this once more. There is real work to be done here. You; you personally, need to do it. There's no one else here. |
|
See #565 |
|
Exactly, and @LucasChollet stepped up. Even better his pull request demonstrates an ability to deal with the actual build process and an understanding of the basic architecture. His patch was as good as one I might have contributed in another world and, perhaps, better because I'm sure he is approaching it with a less jaundiced mind. To be clear; he did the things I would have done well just as well and the errors he made were certainly those I might have made (though we would probably have made different errors in practice of course.) I suggest to @LucasChollet and @ctruta that you talk directly to each other about what you are each trying to achieve. I suggest not on 565. I suggest to @ctruta that you need someone else on this and that someone is not me. |
|
@chrisb I suggest you take charge of this, given that you can. There really are three chunks here and some decision needs to be made about the only remaining issue of API compatibility. @ctruta has "spoken" but you have exactly the same capabilities as he, indeed more so because you are the W3C PNG head. I just said to Cosmin that this organisation really needs* a discussion tab and it really requires a security tab; when I was a member I couldn't do either so I'm appealing to all four of you now to actually do something. I'm fingering you @chrisb because you clearly have knowledge and experience sufficient for this task; you have done great work on https://github.com/w3c/png but, as always, more needs to be done. |
|
Thanks, John. I'll re-read this and the patch soon. |
|
Many thanks to @LucasChollet for contributing the original patch, on top of which I added my own set of changes in order to make it behave exactly as the rest of the colorspace changes do. I pushed them both to libpng16 and to libpng18, because I haven't seen a good reason not to.
I guess I'll have to choose my wording better, so as not to sound "dictatorial" next time 🤷♂️ My impression was that some tie-breaking decision was needed. We were stuck in analysis-paralysis, and we really need to move on and catch up with the latest and greatest developments in digital imaging. The alternative -- to remain behind -- is hardly appealing to me. @jbowler, I THINK I understood your rationale, and I THINK that the way in which the cICP implementation looks as of now (commit 823c2d8 in branch libpng16 and commit 1f2cd30 in branch libpng18) is as correct as it can be. If you have further objections specific to this code, something that still needs fixing, please submit a PR, and I'll review it and appreciate it as much as I do all other contributions. Also, if you think something is broken in the PNG-3 draft, in the way in which cICP is being fit among the other colorspaces, and in the way it plays with the other colorspaces, and you can explain it to us (and to me specifically, if you please don't mind me...) then by all means, let us fix the PNG-3 draft. I mean, it's still a draft! Last but not least, @jbowler, if you have a specific use case in mind of an existing application that suddenly gets broken by upgrading from libpng-1.6.44 to libpng-1.6.45 (which I'd like to release sooner instead of later, with the cICP support officially in), then please explain that breakage. Please don't mind me if I don't fully understand your objection... but... I STILL don't FULLY understand your objection... |
I checked QWebEngine from Qt6 but the basic procedure works with any app that uses the system libpng16 and currently supports cICP. I'm using Qt6.8.1 and I can guarantee that this version handles cICP at least with PQ encoding and REC 2020 end-points (the first example in PNG v3 IRC and the encoding in the test image below). You need to display a suitable test image with both the cICP supporting app and with something which doesn't to verify that 1.6.44 works. With the test file below and an sRGB monitor you should see this:
This was produced on gentoo with an older DELL UHD sRGB monitor using Falkon (left) and "magick display" (right, ImageMagick -7.1.1.38-r2) both using an installed libpng-1.6.44. The test file is the one @FotisK provided in this PR: ImageMagick/ImageMagick#7797. There is some spurious stuff in there (the XMP profile) but it's harmless. See my comments at the end of that PR. The screenshot was taken with spectacle (as a PNG), all ancillary chunks were removed (TweakPNG) and I resampled in PhotoShop2024-Classic with bicubic (Alt-5) then saved with the maximum JPEG compression level. After first checking for cICP support change the installed libpng to 1.6.45.git and repeat the test. Nothing should change. It is absolutely necessary to verify that libpng-1.6.44 works with the app to be tested. The image must display correctly as in the screenshot above. It is also absolutely necessary to determine that changing the system libpng does, in fact, change the DLL that the app loads. It is very easy to make mistakes here. I used Any app which gets to be broken needs to add code to handle those chunks as unknown. NOTE: the app may already do that but it doesn't have to in 1.6.44 because cICP, cLLI and mDCV are all treated as unknown by default. @ctruta: there is one thing you didn't do from the mDCV/cLLI discussion, where the author noticed a bug in the cICP implementation. You need to add all three chunks (and, indeed, any other chunk support that gets added) to the list "chunks_to_ignore" currently at line 1420 of pngset.c, because otherwise apps which use that facility with the correct settings will get broken (IRC; unknown chunk handling is incredibly complicated). @chrisb: Cosmin did not implement your suggestion to add an option to explicitly turn on the support. This could have been done in 1.6 to avoid the possibility of breakage because Currently Falkon and Chrome (built with system libpng) are OK. I'm fairly sure than any app which uses Qt6 QWebEngine will be ok. I can't make any statement about other libpng using apps which currently support cICP; the failure will be completely quiet, just the displayed image will change, perhaps only very slightly. See the inability of some of the commentors to see the missing support in the above issue about magick. More test cases are needed. That's on my TODO list but it's lower priority at present. |
|
Ok, @jbowler, thank you for rehashing and summarizing what you had explained previously, for the benefit of my better understanding. Sure can fix. |
The mDCV and cLLI part is already in issue #626 but the chunk names are still wrong and the code does not validate the the 4-byte fields are in range (I can't remember if I saw that before). I.e. the code needs to check this condition:
I can't see it doing that anywhere |
|
It's in #635 with test cases for all three chunks added to pngtest.png. This Issue is already closed. |
|
** BREAKING NEWS ** I will integrate the mDCV and cLLI tomorrow, but with a twist in our workflow, hopefully for the better. I merged the Let us focus on what libpng-1.6.x still is (i.e. production code) and on what libpng-1.8.0 wants to be (i.e. shaking up the build tree and cleaning up the cruft). Eventually, libpng-1.8.0 will become production and libpng-1.6.x will become maintenance. But we're not there yet. I, for one, I'd like to clean up quite a bit from libpng-1.8.0.git, and I assume @jbowler does too. For this reason: Let us continue the business-as-usual development of the We will always take CICP implementation improvements, fixes, test cases, etc., as it is the case for everything else. Besides that, the CICP support has been shipped |
|
Congratulations. The drop into SourceForge is the magic trigger; now things start to happen. This is the current Gentoo state: |
The CICP Chunk identifies explicit characteristics of an image which is described in H.273. CICP is used in every video wrapper and in many codecs to identify the images contained in every framer.
Without this HDR/SDR/WCG (High Dynamic Range, Standard Dynamic Range and Wide Color Gamut) images using BT.709, BT.2100 can't be identified properly.
Identification is critical for content creation described in ITU-R BT.2408 (single-master UHD workflows). The new CICP chunk gives new support for
The text was updated successfully, but these errors were encountered: