Allow odd clap dimensions and offsets #2426
Conversation
|
@wantehchang Do you plan on submitting #1749 first? |
Done. The reason I had not submitted #1749 is that I am not sure whether only some of the members of the avifCleanApertureBox struct will be changed to int32_t in the next version of ISO BMFF. But it is fine to submit #1749 first because it describes how libavif interprets the members of the avifCleanApertureBox struct now. We can update the comment when the next version of ISO BMFF is released. |
src/avif.c
Outdated
| @@ -722,6 +699,16 @@ avifBool avifCropRectConvertCleanApertureBox(avifCropRect * cropRect, | |||
| // positive or negative. For cleanApertureWidth and cleanApertureHeight, | |||
| // N shall be positive and D shall be strictly positive. | |||
|
|
|||
| // ISO/IEC 23000-22:2024, Section 7.3.6.7: | |||
There was a problem hiding this comment.
It is best to wait until ISO/IEC 23000-22:2024 is formally published. Its status is still Final Draft Internal Standard (FDIS) right now: https://www.iso.org/standard/87576.html
There was a problem hiding this comment.
Added "FDIS" in the comment.
I am fine with waiting till it is published, although I guess it would not hurt to merge this PR now. As you see fit.
There was a problem hiding this comment.
It is now "Under publication". I wonder if it will be numbered as ISO/IEC 23000-22:2025 or just "2nd edition".
@wantehchang Did you find another approach? |
|
Yannis: I will review this pull request tomorrow. Sorry about the delay. |
| avifDiagnostics * diag); | ||
| // If this function returns true, the image must be upsampled from 4:2:0 or 4:2:2 to 4:4:4 before | ||
| // Clean Aperture values are applied. | ||
| AVIF_NODISCARD AVIF_API avifBool avifCropRectRequiresUpsampling(const avifCropRect * cropRect, avifPixelFormat yuvFormat); |
There was a problem hiding this comment.
Future: I think we should add an avifImageUpsample() or avifImageUpsampleTo444() function for completeness.
I also found that I did not read your reply to my suggestion of adding avifCropRectRequiresUpsampling(). Sorry about that. I sometimes review a new version of the pull request directly, without reading the comments.
You are right that libavif users may neglect to call avifCropRectRequiresUpsampling(). To help prevent that mistake, we should either return the "requires upsampling" boolean as an output parameter (as you did before), or upsample the image for the users. If we do the latter, we might as well crop the image for the users.
There was a problem hiding this comment.
"requires upsampling" boolean as an output parameter (as you did before)
We went away from that approach already, and it does not cover the orientation.
upsample the image for the users
I began writing a PR for that and quickly gave up. It requires a lot of changes, especially because the whole implementation revolves around avifDecoder::image. Since it is part of the public API, it must stay there, and it must be the buffer that is cropped/orientated/upsampled. However it is also used by the underlying codec, so it cannot be cropped/oriented/upsampled (at least till the last frame is decoded or the codec is destroyed).
So some internal avifDecoderData::image must be introduced, and avifDecoder->image is either a copy or a view of it, depending on the transform.
Upsampling can be done in multiple ways (nearest etc.) and has to take into account chroma citing metadata. avifDecoderDecodedRowCount() must also be adapted to cropping/orientation.
A cheaper alternative could be a helper function that returns a new image object with all the necessary transforms applied, and users must call that. But it will not go well with incremental support and may introduce unnecessary buffer copies.
Yet another path is to add a public API avifDecoder::imageWithTransformsApplied. This is roughly the same amount of work as the internal avifDecoderData::image but with fewer risks of breaking something in the existing API.
Overall it felt like a lot of work compared to just writing in avif.h that users must take care of that themselves, which I agree is not great, but also already the current situation.
There was a problem hiding this comment.
Thanks a lot for the analysis.
Do you think we should provide an avifImageUpsample() function? Or should we just document that one can also convert YUV to RGB and then crop the RGB image?
There was a problem hiding this comment.
Do you think we should provide an
avifImageUpsample()function?
In Chrome, samples can be rendered directly without being converted to RGB first, right? If so, it would be useful at least for this kind of use case.
Is orientation and cropping both applied in Chrome?
Or should we just document that one can also convert YUV to RGB and then crop the RGB image?
That sounds useful to mention, even if libavif offers an alternative in the future. Done.
Also added another comment about applying transform next to avifDecoder::image.
There was a problem hiding this comment.
Chrome's blink::AVIFImageDecoder can provide either YUV samples or RGB samples to its callers. Cropping is done by blink::AVIFImageDecoder. (It requires that the top left corner of the clean aperture be located at (0, 0).) Orientation is applied by the callers of blink::AVIFImageDecoder.
The constraint in MIAF was replaced by an upsampling step before cropping.
Remove yuvFormat arg from avifCropRectFromCleanApertureBox(). Make avifCropRectConvertCleanApertureBox() behave exactly as before. Fix comments. Update avifclaptest. Print warnings in avifdec.
Unused unless AVIF_ENABLE_EXPERIMENTAL_MINI is defined.
[skip ci]
by avifCropRectFromCleanApertureBox() in y4m.c.
The constraint in MIAF was replaced by an upsampling step before cropping.
Notes:
avifdecdoes not crop anything when decoding.clapfield inavifImage. It is the user's duty to upsample, hence the added output argument inavifCropRectFromCleanApertureBox().See AOMediaCodec/av1-avif#202 (comment).