Write primaries and transfer characteritics info in decoded png. #1422
Conversation
If there is no ICC but the cicp values are different from sRGB, they are added to the png file using the cHRM, gAMA and cICP chunks. The cHRM and gAMA chunks are preferred since they are older and more widely understood (although some viewers may ignore them). The new cICP chunk is added if gAMA is not enough to specify the transfer characteristics.
apps/shared/avifpng.c
Outdated
| } else if (hasNonSrgbPrimaries || hasNonSrgbTransfer) { | ||
| float primariesCoords[8]; | ||
| avifColorPrimariesGetValues(avif->colorPrimaries, primariesCoords); | ||
| png_set_cHRM(png, | ||
| info, | ||
| primariesCoords[6], | ||
| primariesCoords[7], | ||
| primariesCoords[0], | ||
| primariesCoords[1], | ||
| primariesCoords[2], | ||
| primariesCoords[3], | ||
| primariesCoords[4], | ||
| primariesCoords[5]); | ||
| float gamma; | ||
| if (avifTransferCharacteristicsGamma(avif->transferCharacteristics, &gamma)) { | ||
| png_set_gAMA(png, info, 1.0f / gamma); | ||
| } else { | ||
| // The transfer characteristics cannot be represented with a simple gAma chunk. | ||
| // A cICP chunk is needed, but might not be understood by all viewers since it's new. | ||
| writeCicp = AVIF_TRUE; | ||
| } |
There was a problem hiding this comment.
Just an idea: rather than making these exclusive, it could be ok (and even recommended) to write everything that is available (cICP, iCCP and cHRM + gAMA), and let the PNG readers parse the color encoding chunks in order of priority in the spec and/or their level of support?
There was a problem hiding this comment.
I'm adding the cICP chunk only if it's needed as a small size optimization, but I guess we don't really care, so I could simplify this. However, even when cICP is added, I am still adding cHR+gAMA as a fallback as you said, so it's not exclusive.
We can't easily add other chunks in case ICC is present, because we would need to parse the ICC to get the corresponding primaries/gamma. I don't think we should use avif->colorPrimaries and avif->transferCharacteristics in this case, as they are supposed to be irrelevant when an ICC is present as explained in https://github.com/AOMediaCodec/libavif/wiki/CICP
As an aside also note that the priority between CICP and ICC is different in the two formats: ICC has priority in avif, while CICP has priority in png.
Another chunk that could also be added is the sRGB chunk, but I think all viewers just assume sRGB if no color info is present? so I don't think it's really necessary but could be added if colorPrimaries is 1 and transferCharacteristics is 13.
There was a problem hiding this comment.
I'm not sure we're overly concerned about png size, so simplifying things where you can might make sense.
There was a problem hiding this comment.
Fair enough. I changed the logic to write as many chunks as we can, including the sRGB chunk.
apps/shared/avifpng.c
Outdated
| } else if (hasNonSrgbPrimaries || hasNonSrgbTransfer) { | ||
| float primariesCoords[8]; | ||
| avifColorPrimariesGetValues(avif->colorPrimaries, primariesCoords); | ||
| png_set_cHRM(png, | ||
| info, | ||
| primariesCoords[6], | ||
| primariesCoords[7], | ||
| primariesCoords[0], | ||
| primariesCoords[1], | ||
| primariesCoords[2], | ||
| primariesCoords[3], | ||
| primariesCoords[4], | ||
| primariesCoords[5]); | ||
| float gamma; | ||
| if (avifTransferCharacteristicsGamma(avif->transferCharacteristics, &gamma)) { | ||
| png_set_gAMA(png, info, 1.0f / gamma); | ||
| } else { | ||
| // The transfer characteristics cannot be represented with a simple gAma chunk. | ||
| // A cICP chunk is needed, but might not be understood by all viewers since it's new. | ||
| writeCicp = AVIF_TRUE; | ||
| } |
There was a problem hiding this comment.
I'm not sure we're overly concerned about png size, so simplifying things where you can might make sense.
Includes writing of sRGB chunk.
| } else { | ||
| if (avif->colorPrimaries != AVIF_COLOR_PRIMARIES_UNKNOWN && avif->colorPrimaries != AVIF_COLOR_PRIMARIES_UNSPECIFIED) { | ||
| float primariesCoords[8]; | ||
| avifColorPrimariesGetValues(avif->colorPrimaries, primariesCoords); |
There was a problem hiding this comment.
Optional: You don't need to address this issue in this pull request.
I looked at the implementation of avifColorPrimariesGetValues. If its first parameter is not supported, it returns the color primaries of AVIF_COLOR_PRIMARIES_BT709. If that is not desirable here, we can modify the avifColorPrimariesGetValues to return a success/failure value.
There was a problem hiding this comment.
I think it's fine as is for now, all colorspaces are supported right now except UNKONWN and UNDEFINED. I don't see why a colorspace wouldn't be supported.
There was a problem hiding this comment.
Thank you for the reply. I didn't realize avifColorPrimariesTables has an entry for each of the avifColorPrimaries enum values.
So the only way for avifColorPrimariesGetValues to fail now is an invalid or a "future" avifColorPrimaries enum value. In that case we will use AVIF_COLOR_PRIMARIES_BT709 as a reasonable default. I was just wondering if it would be better to not write a cHRM chunk in that case.
There was a problem hiding this comment.
Ha I see what you mean, I hadn't considered the possibility of a new value being read by an old decoder. I agree it would be better not to write the cHRM chunk in this case. I might send a follow up patch.
If there is no ICC but the cicp values are different from sRGB, they are added to the png file using the cHRM, gAMA and cICP chunks.
The cHRM and gAMA chunks are preferred since they are older and more widely understood (although some viewers may ignore them). The new cICP chunk is added if gAMA is not enough to specify the transfer characteristics.
Fixes #1421 for png.