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.

Sign up for GitHub

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

Write primaries and transfer characteritics info in decoded png. #1422

Merged
merged 3 commits into from
Jun 7, 2023
Merged

Write primaries and transfer characteritics info in decoded png. #1422

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
473c605
Write primaries and transfer characteritics info in decoded png.
maryla-uc Jun 1, 2023
e733fc3
Simplify png logic to write as many color information chunks as we can.
maryla-uc Jun 6, 2023
abcdfca
Fix function signature and typo.
maryla-uc Jun 7, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
48 changes: 47 additions & 1 deletion apps/shared/avifpng.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -503,8 +503,42 @@ avifBool avifPNGWrite(const char * outputFilename, const avifImage * avif, uint3
}

png_set_IHDR(png, info, avif->width, avif->height, rgbDepth, colorType, PNG_INTERLACE_NONE, PNG_COMPRESSION_TYPE_DEFAULT, PNG_FILTER_TYPE_DEFAULT);
if (avif->icc.data && (avif->icc.size > 0)) {

const avifBool hasIcc = avif->icc.data && (avif->icc.size > 0);
if (hasIcc) {
// If there is an ICC profile, the CICP values are irrelevant and only the ICC profile
// is written. If we could extract the primaries/transfer curve from the ICC profile,
// then they could be written in cHRM/gAMA chunks.
png_set_iCCP(png, info, "libavif", 0, avif->icc.data, (png_uint_32)avif->icc.size);
} else {
const avifBool isSrgb = (avif->colorPrimaries == AVIF_COLOR_PRIMARIES_BT709) &&
(avif->transferCharacteristics == AVIF_TRANSFER_CHARACTERISTICS_SRGB);
if (isSrgb) {
png_set_sRGB_gAMA_and_cHRM(png, info, PNG_sRGB_INTENT_PERCEPTUAL);
} else {
if (avif->colorPrimaries != AVIF_COLOR_PRIMARIES_UNKNOWN && avif->colorPrimaries != AVIF_COLOR_PRIMARIES_UNSPECIFIED) {
float primariesCoords[8];
avifColorPrimariesGetValues(avif->colorPrimaries, primariesCoords);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

@wantehchang wantehchang Jun 7, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

png_set_cHRM(png,
info,
primariesCoords[6],
primariesCoords[7],
primariesCoords[0],
primariesCoords[1],
primariesCoords[2],
primariesCoords[3],
primariesCoords[4],
primariesCoords[5]);
}
float gamma;
// Write the transfer characteristics IF it can be represented as a
// simple gamma value. Most transfer characteristics cannot be
// represented this way. Viewers that support the cICP chunk can use
// that instead, but older viewers might show incorrect colors.
if (avifTransferCharacteristicsGetGamma(avif->transferCharacteristics, &gamma) == AVIF_RESULT_OK) {
png_set_gAMA(png, info, 1.0f / gamma);
}
wantehchang marked this conversation as resolved.
Show resolved Hide resolved
}
}

png_text texts[2];
Expand Down Expand Up @@ -543,6 +577,18 @@ avifBool avifPNGWrite(const char * outputFilename, const avifImage * avif, uint3

png_write_info(png, info);

// Custom chunk writing, must appear after png_write_info.
// With AVIF, an ICC profile takes priority over CICP, but with PNG files, CICP takes priority over ICC.
// Therefore CICP should only be written if there is no ICC profile.
if (!hasIcc) {
const png_byte cicp[5] = "cICP";
const png_byte cicpData[4] = { (png_byte)avif->colorPrimaries,
(png_byte)avif->transferCharacteristics,
AVIF_MATRIX_COEFFICIENTS_IDENTITY,
1 /*full range*/ };
png_write_chunk(png, cicp, cicpData, 4);
}

rowPointers = (png_bytep *)malloc(sizeof(png_bytep) * avif->height);
if (monochrome8bit) {
uint8_t * yPlane = avif->yuvPlanes[AVIF_CHAN_Y];
Expand Down
4 changes: 4 additions & 0 deletions include/avif/avif.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -319,6 +319,10 @@ enum
};
typedef uint16_t avifTransferCharacteristics; // AVIF_TRANSFER_CHARACTERISTICS_*

// If the given transfer characteristics can be expressed with a simple gamma value, sets 'gamma'
// to that value and returns AVIF_RESULT_OK. Returns an error otherwise.
AVIF_API avifResult avifTransferCharacteristicsGetGamma(avifTransferCharacteristics atc, float * gamma);

enum
{
AVIF_MATRIX_COEFFICIENTS_IDENTITY = 0,
Expand Down
17 changes: 17 additions & 0 deletions src/colr.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,23 @@ avifColorPrimaries avifColorPrimariesFind(const float inPrimaries[8], const char
return AVIF_COLOR_PRIMARIES_UNKNOWN;
}

avifResult avifTransferCharacteristicsGetGamma(avifTransferCharacteristics atc, float * gamma)
{
switch (atc) {
case AVIF_TRANSFER_CHARACTERISTICS_BT470M:
*gamma = 2.2f;
return AVIF_RESULT_OK;
case AVIF_TRANSFER_CHARACTERISTICS_BT470BG:
*gamma = 2.8f;
return AVIF_RESULT_OK;
case AVIF_TRANSFER_CHARACTERISTICS_LINEAR:
*gamma = 1.0f;
return AVIF_RESULT_OK;
default:
return AVIF_RESULT_INVALID_ARGUMENT;
}
}

struct avifMatrixCoefficientsTable
{
avifMatrixCoefficients matrixCoefficientsEnum;
Expand Down