Don't require crop width,height be even for 4:2:0 #1743
Conversation
These requirements were removed in ISO/IEC 23000-22:2019/Amd. 2:2021 from Section 7.3.6.7. This is one reason most of the clap properties seen by Chrome were considered invalid. Bug: b/308458941
| {99, | ||
| 99, | ||
| AVIF_PIXEL_FORMAT_YUV420, | ||
| {99, 1, 99, 1, static_cast<uint32_t>(-1), 2, static_cast<uint32_t>(-1), |
There was a problem hiding this comment.
This is fine for consistency but please consider uint32_t{-1}.
Also the topic was already discussed but this unsigned-to-signed API should be reworked in my opinion, especially since the specification was updated. Is switching from unsigned to signed for some fields in avifCleanApertureBox a breaking API/ABI change? We should at least add a comment nearby this struct in avif.h to explain.
There was a problem hiding this comment.
I also tried uint32_t{-1} but the compiler didn't allow it. I think the compiler checks if the value is in the range of the target type.
ABI compatibility usually means we can drop in a new version of libavif.so and the old apps, such as avifenc and avifdec, will still work. Under this definition, changing some fields from unsigned to signed doesn't break ABI. This change may cause compiler warnings. We should do some experiment.
There was a problem hiding this comment.
This is the compiler warning for uint32_t{-1}:
libavif/tests/gtest/avifclaptest.cc:75:32: error: narrowing conversion of ‘-1’ from ‘int’ to ‘uint32_t’ {aka ‘unsigned int’} [-Wnarrowing]
75 | {99, 1, 99, 1, uint32_t{-1}, 2, static_cast<uint32_t>(-1),
| ^
There was a problem hiding this comment.
Also the topic was already discussed but this unsigned-to-signed API should be reworked in my opinion, especially since the specification was updated.
When you said "the specification was updated", were you referring to ISO BMFF or MIAF?
The eight fields in the clean aperture property still have the unsigned int(32) type in the latest versions of ISO BMFF and MIAF.
We should at least add a comment nearby this struct in avif.h to explain.
Please see #1749.
There was a problem hiding this comment.
The eight fields in the clean aperture property still have the
unsigned int(32)type in the latest versions of ISO BMFF and MIAF.
Indeed, I was mistaken. The version with signed horizOffN and signed vertOffN I was looking at is only a draft. Maybe it will get published some time soon.
Please see #1749.
Thanks.
These requirements were removed in ISO/IEC 23000-22:2019/Amd. 2:2021 from Section 7.3.6.7. This is one reason most of the clap properties seen by Chrome were considered invalid.
Bug: b/308458941