-
Notifications
You must be signed in to change notification settings - Fork 201
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
Allow lossless compression using 4:0:0 on grayscale input. #1460
Conversation
85e1365
to
69538fa
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ChangeLog entry?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Vincent: Thank you very much for fixing the bug. I have some questions and comments.
@@ -40,6 +40,7 @@ List of incompatible ABI changes in this release: | |||
* Add API for multi-threaded YUV to RGB color conversion. | |||
* Add experimental support for AV2 behind the compilation flag AVIF_CODEC_AVM. | |||
AVIF_CODEC_CHOICE_AVM is now part of avifCodecChoice. | |||
* Allow lossless 4:0:0 on grayscale input. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We may not need a changelog entry for this pull request. Commit 6122b01, which introduced the bug #1453, was made during the development of this release. So the changelog entry should describe the net effects of commit 6122b01 and this pull request. If this pull request only fixes the bug and has no other changes worth mentioning, then we should omit the changelog entry.
I assume that lossless 4:0:0 compression of grayscale input worked before commit 6122b01. Is that not the case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was working but by converting grayscale to RGB and then compressing it. This is end to end 4:0:0 lossless.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the reply. Then the net change is not a new feature but rather a performance optimization of a working feature. This changelog entry should not be in the "Added" section (see line 25), which means new features.
This changelog entry can be moved to the "Changed" section (see line 46) and it should describe the net change (end-to-end 4:0:0 lossless compression of grayscale PNG input).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed, I made: #1477
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Vincent: I ran the followoing command with the current main branch and with v0.11.1 (the previous libavif release).
avifenc -y 400 -l --ignore-icc kodim03_grayscale_gamma1.6.png output.avif
I found that the net change of this pull request is a new feature. So it is correct to add the changelog entry to the Added section. My apologies for my confusion.
With v0.11.1, the above command does succeed but the output AVIF file has the 4:4:4 format (i.e., not 4:0:0 monochrome) and CICP=1/13/0. So it is lossless 4:4:4, not lossless 4:0:0.
However, I found that Chrome has trouble if MatrixCoefficients=0 (Identity) is used with 4:0:0. I will need to debug it. Firefox has no problem with MatrixCoefficients=0 and 4:0:0. To work around this Chrome issue, we can use MatrixCoefficients=6 (the default) with 4:0:0. Note that this issue is related to my comment in apps/avifenc.c at line 1482.
@@ -88,7 +88,7 @@ pushd ${TMP_DIR} | |||
|
|||
"${AVIFENC}" -s 8 -l "${INPUT_GRAY_PNG}" -o "${ENCODED_FILE}" | |||
"${AVIFDEC}" "${ENCODED_FILE}" "${DECODED_FILE}" | |||
"${IMAGEMAGICK}" "${DECODED_FILE}" -profile "${SRGB_ICC}" "${CORRECTED_FILE}" | |||
"${IMAGEMAGICK}" "${DECODED_FILE}" -define png:color-type=2 -profile "${SRGB_ICC}" "${CORRECTED_FILE}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just curious: why is this change necessary?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To force the PNG to be grayscale.
@@ -384,7 +385,9 @@ avifResult avifImageRGBToYUV(avifImage * image, const avifRGBImage * rgb) | |||
} | |||
|
|||
// Populate any subsampled channels with averages from the 2x2 block | |||
if (image->yuvFormat == AVIF_PIXEL_FORMAT_YUV420) { | |||
if (image->yuvFormat == AVIF_PIXEL_FORMAT_YUV400) { | |||
// Do nothing on chroma planes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change isn't necessary. Is this just to make it clear that we considered all cases here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes.
fprintf(stderr, | ||
"When set, the pixel format can only be 444 in lossless " | ||
"mode.\n"); | ||
"mode. 400 also works if the input is grayscale.\n"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe that this is the key part of the bug fix, and we don't need the changes to allow AVIF_MATRIX_COEFFICIENTS_IDENTITY
to be used with 4:0:0.
On the other hand, it seems harmless to allow AVIF_MATRIX_COEFFICIENTS_IDENTITY
to be used with 4:0:0. I need to think about this more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ack. I also considered that an image stored as RGB but asked to be compressed losslessly using 4:0:0 does not work because it is considered as lossy. Then again, if your original image is grayscale, there will indeed be no visual loss. BUT, instead of 3 channels, you'll have one. I though that was enough to consider the images different, hence lossy.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a technical reason to avoid using CICP=1/13/0 with lossless 4:0:0. See the following excerpt of the color_config() function in Section 5.5.2 (Color config syntax) of the AV1 spec:
if ( mono_chrome ) {
color_range f(1)
subsampling_x = 1
subsampling_y = 1
chroma_sample_position = CSP_UNKNOWN
separate_uv_delta_q = 0
return
} else if ( color_primaries == CP_BT_709 &&
transfer_characteristics == TC_SRGB &&
matrix_coefficients == MC_IDENTITY ) {
color_range = 1
subsampling_x = 0
subsampling_y = 0
} else {
If mono_chrome
is true, then AV1 specifies that subsampling_x
and subsampling_y
be equal to 1.
The "else if" statement means "else if CICP == 1/13/0". Then it specifies that subsampling_x
and subsampling_y
be equal to 0.
So if both mono_chrome
is equal to 1 and CICP == 1/13/0, then the AV1 spec appears to require conflicting values for subsampling_x
and subsampling_y
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
More on the technical reason: Section 6.4.2 (Color config semantics) of the AV1 spec also says, on page 119:
If matrix_coefficients is equal to MC_IDENTITY, it is a requirement of bitstream conformance that subsampling_x is equal to 0 and subsampling_y is equal to 0.
Since subsampling_x
is equal to 1 and subsampling_y
is equal to 1 if mono_chrome
is equal to 1, matrix_coefficients == MC_IDENTITY
seems incompatible with mono_chrome == 1
.
This fixes #1453
AVM does not support monochrome yet, cf
libavif/src/codec_avm.c
Line 648 in 0cd8016