Support ignoring alpha in imageContentToDecode#3288
Conversation
3ca2859 to
b2e64ab
Compare
f698134 to
69e7f17
Compare
| // Do not check the track's handlerType. It should be "auxv" according to | ||
| // HEIF (ISO/IEC 23008-12:2022), Section 7.5.3.1, but old versions of libavif used to write | ||
| // "pict" instead. See https://github.com/AOMediaCodec/libavif/commit/65d0af9 | ||
| if (decoder->imageContentToDecode & AVIF_IMAGE_CONTENT_ALPHA) { |
There was a problem hiding this comment.
Here I added if (decoder->imageContentToDecode & AVIF_IMAGE_CONTENT_ALPHA) { around this block of code. The only change is to the block of code is that I moved the declaration of alphaCodecType outside the if block, because it is used after the if block.
| if ((decoder->imageContentToDecode & AVIF_IMAGE_CONTENT_COLOR_AND_ALPHA) != 0 && | ||
| (decoder->imageContentToDecode & AVIF_IMAGE_CONTENT_COLOR_AND_ALPHA) != AVIF_IMAGE_CONTENT_COLOR_AND_ALPHA) { | ||
| avifDiagnosticsPrintf(&decoder->diag, "imageContentToDecode set to only color or only alpha is not supported"); | ||
| // Alpha only is not currently supported. |
There was a problem hiding this comment.
All the changes in this PR are straightforward except the changes to this file. Please review the changes to src/read.c carefully.
Maryla: If you are not familiar with the code in src/read.c, we can wait for Yannis's review.
There was a problem hiding this comment.
Yannis: In the interest of time, I have merged this PR. It would be great if you could still do a post-commit review of the changes to include/avif/avif.h and src/read.c in this PR. Thanks!
maryla-uc
left a comment
There was a problem hiding this comment.
Currently the documentation of alphaPresent is "This is true when avifDecoderParse() detects an alpha plane."
It's unclear from the documentation what the value should be when imageContentToDecode does not include alpha. To me it seems that the intention of the API is that imageContentToDecode is about the pixels actually decoded. E.g. for gain maps, we still read the gain map metadata even when we don't decode the pixels. So it would make sense to me that alphaPresent would still be set even when not decoding alpha plane pixels.
Feel free to disagree and keep the current behavior but in any case the behavior should be clarified in avif.h
| AVIF_IMAGE_CONTENT_COLOR = (1 << 0), | ||
| // Alpha only is not currently supported. | ||
| AVIF_IMAGE_CONTENT_ALPHA = (1 << 1), | ||
| AVIF_IMAGE_CONTENT_COLOR_AND_ALPHA = AVIF_IMAGE_CONTENT_COLOR | AVIF_IMAGE_CONTENT_ALPHA, |
There was a problem hiding this comment.
Maryla: Thank you for pointing out that the documentation of decoder->alphaPresent suggests it should be independent of whether the AVIF_IMAGE_CONTENT_ALPHA bit is set in decoder->imageContentToDecode.
When decoder->alphaPresent was added (for Chrome), it was intended to indicate whether the decoder image would have an alpha plane. Since decoder->imageContentToDecode didn't exist then, we didn't need to deal with this issue.
Before this PR, decoder->imageContentToDecode could already be set to AVIF_IMAGE_CONTENT_GAIN_MAP without the AVIF_IMAGE_CONTENT_ALPHA bit. I think it is better to preserve the behavior of decoder->alphaPresent when decoder->imageContentToDecode is set to AVIF_IMAGE_CONTENT_GAIN_MAP. I will find out what the behavior is, but I suspect it is what the comment says, "This is true when avifDecoderParse() detects an alpha plane."
There was a problem hiding this comment.
Maryla: PR #3301 shows you are right. I reverted decoder->alphaPresent to the original meaning. Thanks again for catching this bug.
Since the new changes to src/read.c are quite different, it would be good to review this PR again. I suggest first reviewing the second commit except src/read.c, and then reviewing the net changes to src/read.c.
69e7f17 to
4ebf7b9
Compare
Add AVIF_IMAGE_CONTENT_COLOR and AVIF_IMAGE_CONTENT_ALPHA and allow decoding color only. Decoding alpha only is still not supported. Remove the ignoreAlpha parameter from the avifImageCreateView() function. If alpha should be ignored, the caller of avifImageCreateView() is responsible for clearing the AVIF_IMAGE_CONTENT_ALPHA bit in decoder->imageContentToDecode before decoding (assuming srcImage is decoder->image).
Restore the meaning of decoder->alphaPresent. Add animation tests.
4ebf7b9 to
1a3f155
Compare
A follow-up to PR #3288. Fix avifanimationtest test failures (avifDecoderNextImage() failed with AVIF_RESULT_NO_CODEC_AVAILABLE) in the "build-static-av2 (OFF)" CI workflow.
Add AVIF_IMAGE_CONTENT_COLOR and AVIF_IMAGE_CONTENT_ALPHA and allow
decoding color only. Decoding alpha only is still not supported.
Remove the ignoreAlpha parameter from the avifImageCreateView()
function. If alpha should be ignored, the caller of
avifImageCreateView() is responsible for clearing the
AVIF_IMAGE_CONTENT_ALPHA bit in decoder->imageContentToDecode before
decoding (assuming srcImage is decoder->image).