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.

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 YCgCoRe and YCgCoRo decoding. #2078

Merged
merged 4 commits into from
Feb 11, 2025
Merged

Conversation

vrabaud
Copy link
Collaborator

@vrabaud vrabaud commented Mar 25, 2024

Now that the https://www.itu.int/rec/T-REC-H.273 update is to be published, we can allow for decoding by default.
Encoding will come once decoding is sufficiently deployed.

This is the first phase of
#2077

@vrabaud vrabaud requested a review from y-guyon March 25, 2024 13:43
include/avif/avif.h Outdated Show resolved Hide resolved
tests/gtest/aviflosslesstest.cc Outdated Show resolved Hide resolved
tests/gtest/avifrgbtoyuvtest.cc Outdated Show resolved Hide resolved
@vrabaud vrabaud requested a review from wantehchang March 25, 2024 16:10
CMakeLists.txt Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
@vrabaud
Copy link
Collaborator Author

vrabaud commented Apr 29, 2024

I rebased and updated the enums that indeed got changed.

CMakeLists.txt Outdated Show resolved Hide resolved
@vrabaud vrabaud marked this pull request as ready for review August 28, 2024 15:50
@vrabaud vrabaud mentioned this pull request Aug 29, 2024
@vrabaud vrabaud added this to the v1.2.0 milestone Jan 8, 2025
@y-guyon
Copy link
Collaborator

y-guyon commented Feb 6, 2025

H.273 V4 was published:

Value Matrix coefficients
16 YCgCo-Re
17 YCgCo-Ro

This PR can be merged.

I would like to also remove the encoding compile flag:

  • libavif v1.2.0 will likely stick for a long time
  • It makes it easier to cite a command line without having to mention a compile flag
  • YCgCo-Re/o is not enabled by default so it still requires an explicit action by the user

Copy link
Collaborator

@wantehchang wantehchang left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks!

CMakeLists.txt Outdated
@@ -64,7 +64,7 @@ option(BUILD_SHARED_LIBS "Build shared avif library" ON)

option(AVIF_ENABLE_WERROR "Treat all compiler warnings as errors" OFF)

option(AVIF_ENABLE_EXPERIMENTAL_YCGCO_R "Enable experimental YCgCo-R matrix code" OFF)
option(AVIF_ENABLE_YCGCO_R_ENCODING "Enable YCgCo-R matrix codes for encoding" OFF)
Copy link
Collaborator

Choose a reason for hiding this comment

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

It would be good to add a comment to explain why this option is disabled by default and the risk of enabling it.

I guess we are concerned that there isn't widespread decoder-side support initially?

Copy link
Collaborator

Choose a reason for hiding this comment

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

It would be good to add a comment to explain why this option is disabled by default and the risk of enabling it.

Would it be enough to do that on avifenc --help and print a warning on avifenc --cicp x/x/16? See #2078 (comment).

I guess we are concerned that there isn't widespread decoder-side support initially?

Yes.
What happens when decoding such files with current versions of libavif and libheif with all compile flags set to OFF?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Right now, it depends on what you do with it. E.g., using avifdec: outputing to a png creates an error but not outputing to a y4m ... That should be fixed first.

apps/shared/avifjpeg.c Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
src/write.c Outdated Show resolved Hide resolved
tests/gtest/aviflosslesstest.cc Outdated Show resolved Hide resolved
@wantehchang
Copy link
Collaborator

I just saw Yannis's comment:

I would like to also remove the encoding compile flag:

  • libavif v1.2.0 will likely stick for a long time
  • It makes it easier to cite a command line without having to mention a compile flag
  • YCgCo-Re/o is not enabled by default so it still requires an explicit action by the user

I agree with Yannis's suggestion. I also think we are being a little too cautious. Maybe we can note in a comment the date of addition to alert the users, something like this:

diff --git a/include/avif/avif.h b/include/avif/avif.h
index 0de5b92f..9abc26c7 100644
--- a/include/avif/avif.h
+++ b/include/avif/avif.h
@@ -402,8 +402,8 @@ enum
     AVIF_MATRIX_COEFFICIENTS_CHROMA_DERIVED_NCL = 12,
     AVIF_MATRIX_COEFFICIENTS_CHROMA_DERIVED_CL = 13,
     AVIF_MATRIX_COEFFICIENTS_ICTCP = 14,
-    AVIF_MATRIX_COEFFICIENTS_YCGCO_RE = 16,
-    AVIF_MATRIX_COEFFICIENTS_YCGCO_RO = 17,
+    AVIF_MATRIX_COEFFICIENTS_YCGCO_RE = 16,  // Added to libavif in Feb 2025
+    AVIF_MATRIX_COEFFICIENTS_YCGCO_RO = 17,  // Added to libavif in Feb 2025
     AVIF_MATRIX_COEFFICIENTS_LAST
 };
 typedef uint16_t avifMatrixCoefficients; // AVIF_MATRIX_COEFFICIENTS_*

Also, if the decoding application doesn't support YCgCo-Re, the result is either a failure or noticeable wrong colors. So the lack of support will be detected quickly.

@vrabaud vrabaud force-pushed the ycgco branch 2 times, most recently from 8e37b0a to b14ca3a Compare February 10, 2025 15:52
@vrabaud
Copy link
Collaborator Author

vrabaud commented Feb 10, 2025

YCgCo encoding is now enabled. I added a test for it.

Copy link
Collaborator

@y-guyon y-guyon left a comment

Choose a reason for hiding this comment

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

Should there be a warning when using avifenc --cicp x/x/16? Such as "YCgCo-Ro was added in libavif 1.2.0 (Feb 2025) and will render incorrectly in older implementations, use at your own risk" for users of avifenc that do not look at avif.h.

@wantehchang
Copy link
Collaborator

Yannis: The reason you gave before, "libavif v1.2.0 will likely stick for a long time", is also a reason against having avifenc --cicp x/x/16 emit a warning message. Fortunately the lack of support is either a failure or obviously wrong colors, so it will be detected quickly.

Copy link
Collaborator

@wantehchang wantehchang left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks!

apps/avifenc.c Outdated Show resolved Hide resolved
apps/avifenc.c Outdated Show resolved Hide resolved
Now that the https://www.itu.int/rec/T-REC-H.273 update is out
published, we can allow for decoding by default.
Encoding will come once decoding is sufficiently deployed.

This is the first phase of
AOMediaCodec#2077
@vrabaud vrabaud merged commit 906a8a5 into AOMediaCodec:main Feb 11, 2025
35 checks passed
@vrabaud vrabaud deleted the ycgco branch February 11, 2025 09:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants