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.

Sign up for GitHub

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

Jump to bottom

Encode alpha as 4:2:0 with SVT #2004

Merged
merged 1 commit into from
Feb 12, 2024
Merged

Encode alpha as 4:2:0 with SVT #2004

merged 1 commit into from
Feb 12, 2024

Conversation

y-guyon
Copy link
Collaborator

@y-guyon y-guyon commented Feb 9, 2024

See #1992.

@kloczek
Copy link

kloczek commented Feb 10, 2024

Just tested this PR and all looks good now 😄 👍

+ cd libavif-1.0.4
+ /usr/bin/ctest --test-dir x86_64-redhat-linux-gnu --output-on-failure --force-new-ctest-process -j48 ' '
Internal ctest changing into directory: /home/tkloczko/rpmbuild/BUILD/libavif-1.0.4/x86_64-redhat-linux-gnu
Test project /home/tkloczko/rpmbuild/BUILD/libavif-1.0.4/x86_64-redhat-linux-gnu
      Start  1: aviftest
      Start  2: avifyuv_limited
      Start  3: avifyuv_rgb
      Start  4: avifallocationtest
      Start  5: avifalphapremtest
      Start  6: avifbasictest
      Start  7: avifchangesettingtest
      Start  8: avifclaptest
      Start  9: avifcllitest
      Start 10: avifcodectest
      Start 11: avifdecodetest
      Start 12: avifgridapitest
      Start 13: avifimagetest
      Start 14: avifincrtest
      Start 15: avifiostatstest
      Start 16: aviflosslesstest
      Start 17: avifmetadatatest
      Start 18: avifopaquetest
      Start 19: avifpng16bittest
      Start 20: avifprogressivetest
      Start 21: avifreadimagetest
      Start 22: avifrgbtoyuvtest
      Start 23: avifscaletest
      Start 24: avifstreamtest
      Start 25: aviftilingtest
      Start 26: avify4mtest
      Start 27: test_cmd
      Start 28: test_cmd_animation
      Start 29: test_cmd_grid
      Start 30: test_cmd_icc_profile
      Start 31: test_cmd_lossless
      Start 32: test_cmd_metadata
      Start 33: test_cmd_progressive
      Start 34: test_cmd_targetsize
 1/34 Test  #2: avifyuv_limited ..................   Passed    0.06 sec
 2/34 Test  #3: avifyuv_rgb ......................   Passed    0.06 sec
 3/34 Test  #6: avifbasictest ....................   Passed    0.05 sec
 4/34 Test  #8: avifclaptest .....................   Passed    0.05 sec
 5/34 Test #11: avifdecodetest ...................   Passed    0.05 sec
 6/34 Test #13: avifimagetest ....................   Passed    0.04 sec
 7/34 Test #18: avifopaquetest ...................   Passed    0.03 sec
 8/34 Test #23: avifscaletest ....................   Passed    0.03 sec
 9/34 Test #24: avifstreamtest ...................   Passed    0.03 sec
10/34 Test #25: aviftilingtest ...................   Passed    0.02 sec
11/34 Test  #9: avifcllitest .....................   Passed    0.05 sec
12/34 Test  #5: avifalphapremtest ................   Passed    0.07 sec
13/34 Test #30: test_cmd_icc_profile .............   Passed    0.05 sec
14/34 Test  #1: aviftest .........................   Passed    0.19 sec
15/34 Test #10: avifcodectest ....................   Passed    0.31 sec
16/34 Test #19: avifpng16bittest .................   Passed    0.33 sec
17/34 Test #33: test_cmd_progressive .............   Passed    0.33 sec
18/34 Test #16: aviflosslesstest .................   Passed    0.36 sec
19/34 Test #27: test_cmd .........................   Passed    0.58 sec
20/34 Test #26: avify4mtest ......................   Passed    0.59 sec
21/34 Test  #7: avifchangesettingtest ............   Passed    0.67 sec
22/34 Test #12: avifgridapitest ..................   Passed    0.67 sec
23/34 Test #31: test_cmd_lossless ................   Passed    0.65 sec
24/34 Test #20: avifprogressivetest ..............   Passed    0.71 sec
25/34 Test #15: avifiostatstest ..................   Passed    0.77 sec
26/34 Test #28: test_cmd_animation ...............   Passed    1.03 sec
27/34 Test #29: test_cmd_grid ....................   Passed    1.26 sec
28/34 Test #21: avifreadimagetest ................   Passed    1.55 sec
29/34 Test #32: test_cmd_metadata ................   Passed    1.59 sec
30/34 Test #14: avifincrtest .....................   Passed    2.66 sec
31/34 Test #34: test_cmd_targetsize ..............   Passed    2.85 sec
32/34 Test #17: avifmetadatatest .................   Passed    3.77 sec
33/34 Test  #4: avifallocationtest ...............   Passed    4.15 sec
34/34 Test #22: avifrgbtoyuvtest .................   Passed   25.37 sec

100% tests passed, 0 tests failed out of 34

vrabaud
vrabaud approved these changes Feb 12, 2024
View reviewed changes
Copy link
Collaborator

@vrabaud vrabaud left a comment

Choose a reason for hiding this comment

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

Maybe add a TODO to check if that fix is needed for any version > 1.8.0

@y-guyon
Copy link
Collaborator Author

y-guyon commented Feb 12, 2024

Maybe add a TODO to check if that fix is needed for any version > 1.8.0

I doubt SVT will implement 4:0:0 soon and I am uneasy leaving a TODO for years. I consider this patch to be the expected behavior anyway, not some temporary hotfix.

@y-guyon y-guyon merged commit b10d269 into AOMediaCodec:main Feb 12, 2024
20 checks passed
@y-guyon y-guyon deleted the fix_svt branch February 12, 2024 10:29
wantehchang
wantehchang reviewed Feb 13, 2024
View reviewed changes
src/codec_svt.c
input_picture_buffer->cr_stride = uvWidth;
#else
// This workaround was not needed before SVT-AV1 1.8.0.
// See https://github.com/AOMediaCodec/libavif/issues/1992.
Copy link
Collaborator

Choose a reason for hiding this comment

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

It seems better to reference the SVT-AV1 bug report instead: https://gitlab.com/AOMediaCodec/SVT-AV1/-/issues/2146

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The two are referencing each other, and we have more control on a GitHub issue than on a GitLab issue. If someone looks into that again, they can easily find the two reports.

src/codec_svt.c
if (uvPlanes == NULL) {
goto cleanup;
}
memset(uvPlanes, 0, uvSize);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Optional: It would be "realistic" to set the uvPlanes buffer to the neutral value (8-bit: 128; 10-bit: 512; 12-bit: 2048), corresponding to 0.5. But we can't use memset to do that for high bit depths. Given that the AVIF decoders will ignore the UV planes in the alpha auxiliary image, I think it is fine to just set the uvPlanes buffer to 0.

src/codec_svt.c
input_picture_buffer->cb_stride = uvWidth;
input_picture_buffer->cr_stride = uvWidth;
#else
// This workaround was not needed before SVT-AV1 1.8.0.
Copy link
Collaborator

Choose a reason for hiding this comment

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

It is unusual that older versions work better. Do you know why?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No. I asked about it here but got no answer.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wantehchang wantehchang wantehchang left review comments

@vrabaud vrabaud vrabaud approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@y-guyon @kloczek @vrabaud @wantehchang