Add AV2 support #1361

y-guyon · 2023-04-20T14:57:05Z

Experimental. The AV2 specification is not finalized.

Handle av02 image item type and av2C configuration property.
Handle av02 track sample format.
Add avifavmtest.
Disable AV1 tests when all AV1 codecs are disabled.
Add presubmit ci-unix-static-av2.yml.

libavif can only generate AV2 files if it is built with AVIF_CODEC_AVM set to ON.
The avifEncoder API can only output AV2 files if codecChoice is explicitly set to AVIF_CODEC_CHOICE_AVM to avoid mistakes.
avifenc can only output AV2 files if --codec=avm is specified to avoid mistakes.
aom and avm conflict and cannot be used together in the same libavif binary. Other AV1 codecs can be enabled alongside aom.
libavif will decode any AV1 or AV2 file by default as long as the relevant codecs are enabled. The main behavior change for libavif with only AV1 codecs enabled is that before this change, items or tracks of type av02 were ignored. After this change, conformant AV1-AVIF files with extra av02 items will fail to decode unless avm is enabled.

This change allows decoding files with mixed AV1 and AV2 items or tracks (untested because no encoding tool for that). This decision can be changed in the future.

Note: The first commit exists just to reduce diffs. Please review the second and third commits.

jzern

This looks ok, but I'm wondering if this should have an EXPERIMENTAL define to avoid adding av2 support to the common code unnecessarily. Others might have some thoughts.

CMakeLists.txt

y-guyon · 2023-04-24T08:30:39Z

I'm wondering if this should have an EXPERIMENTAL define to avoid adding av2 support to the common code unnecessarily.

codec_avm.c is only compiled if AVIF_CODEC_AVM is ON.
At encoding, AV2 container code paths can only be successful with AVIF_CODEC_AVM=ON and explicitly specifying avm as the codec.
So I guess you are talking about the av02 and av2C boxes that are accepted during parsing, even if AVIF_CODEC_AVM=OFF. I do not think it adds any computational complexity, and leads to AVIF_RESULT_NO_CODEC_AVAILABLE anyway if the user proceeds with decoding without AVIF_CODEC_AVM=ON.
In my opinion it is rather safe to leave it as is and would avoid #defines cluttering the code, but I would be fine with a few #ifdef EXPERIMENTAL guards. If we go with the latter, should we also put all experiment-tagged features behind that define too (incremental, layers etc.)?

vigneshvg · 2023-04-25T16:47:12Z

So I guess you are talking about the av02 and av2C boxes that are accepted during parsing, even if AVIF_CODEC_AVM=OFF. I do not think it adds any computational complexity, and leads to AVIF_RESULT_NO_CODEC_AVAILABLE anyway if the user proceeds with decoding without AVIF_CODEC_AVM=ON.

I didn't look at the code too closely. I have a question about the behavior of avifDecoderParse. Would avifDecoderParse return true for AV2 streams even when AVIF_CODEC_AVM is not set? If so, that would be a potentially not-okay thing to do since some API users rely on avifDecoderParse returning true to conclude if an image is valid AVIF or not.

jzern · 2023-04-26T00:09:45Z

So I guess you are talking about the av02 and av2C boxes that are accepted during parsing, even if AVIF_CODEC_AVM=OFF.

Yes, that's what I was referring to. Anything in the common code.

I do not think it adds any computational complexity, and leads to AVIF_RESULT_NO_CODEC_AVAILABLE anyway if the user proceeds with decoding without AVIF_CODEC_AVM=ON.
In my opinion it is rather safe to leave it as is and would avoid #defines cluttering the code, but I would be fine with a few #ifdef EXPERIMENTAL guards. If we go with the latter, should we also put all experiment-tagged features behind that define too (incremental, layers etc.)?

Making it explicit provides some documentation for the user and does avoid some code in production builds. It's not common right now, so we can get the group's opinion later.

src/codec_avm.c

src/obu.c

y-guyon · 2023-04-26T14:03:25Z

I didn't look at the code too closely. I have a question about the behavior of avifDecoderParse. Would avifDecoderParse return true for AV2 streams even when AVIF_CODEC_AVM is not set?

That was the intent, yes.

If so, that would be a potentially not-okay thing to do since some API users rely on avifDecoderParse returning true to conclude if an image is valid AVIF or not.

Good point. I guarded a few places with #if defined(AVIF_CODEC_AVM) in read.c so that av02 image items or tracks are ignored by default. av2C parsing can no longer be reached without defining AVIF_CODEC_AVM.

Making it explicit provides some documentation for the user and does avoid some code in production builds. It's not common right now, so we can get the group's opinion later.

The code for av02/av2C parsing is so similar to av01/av1C that the production build binary increase is barely noticeable. libavif.so is 198192 bytes at a446c05, 198600 bytes with this change (+0.2%).

vigneshvg

i took a first pass at reviewing the code.

src/obu.c

src/write.c

src/read.c

vigneshvg

mostly okay. just a few minor comments.

CHANGELOG.md

include/avif/internal.h

src/avif.c

src/read.c

src/write.c

vigneshvg

The changes in read.c, write.c and obu.c look fine now. I still haven't looked at codec_avm.c. I will look into it sometime today. Sorry for the delay and thanks for the patience in working through this.

For easier diff of later changes.

Experimental. The AV2 specification is not finalized. Handle av02 image item type and av2C configuration property. Handle av02 track sample format. Add avifavmtest. Disable AV1 tests when all AV1 codecs are disabled. Add presubmit ci-unix-static-av2.yml. libavif can only generate AV2 files if it is built with AVIF_CODEC_AVM set to ON. The avifEncoder API can only output AV2 files if codecChoice is explicitly set to AVIF_CODEC_CHOICE_AVM to avoid mistakes. avifenc can only output AV2 files if --codec=avm is specified to avoid mistakes. aom and avm conflict and cannot be used together in the same libavif binary. Other AV1 codecs can be enabled alongside aom. libavif will decode any AV1 or AV2 file by default as long as the relevant codecs are enabled. The main behavior change for libavif with only AV1 codecs enabled is that before this change, items or tracks of type 'av02' were ignored. After this change, conformant AV1-AVIF files with extra 'av02' items will fail to decode unless avm is enabled. This change allows decoding files with mixed AV1 and AV2 items or tracks (untested because no encoding tool for that). This decision can be changed in the future.

The codecType is now checked in avifDecoderParse() so no need to do it in avifDecoderNextImage(). Add comment.

y-guyon · 2023-04-28T15:38:20Z

I rebased this PR.

The changes in read.c, write.c and obu.c look fine now. I still haven't looked at codec_avm.c. I will look into it sometime today. Sorry for the delay and thanks for the patience in working through this.

No worries, it is not urgent. Thanks for your time.

src/codec_avm.c

vigneshvg

lgtm. thank you.

src/codec_avm.c

jzern · 2023-05-02T20:26:28Z

src/codec_avm.c

+    // We prefer to simply set the aomImage.planes[] pointers to the plane buffers in 'image'. When
+    // doing this, we set aomImage.w equal to aomImage.d_w and aomImage.h equal to aomImage.d_h and
+    // do not "align" aomImage.w and aomImage.h. Unfortunately this exposes a bug in libaom
+    // (https://crbug.com/aomedia/3113) if chroma is subsampled and image->width or image->height is


I wonder if this was carried over to avm

No. See avm's extend.c vs aom's extend.c.

wantehchang

Yannis: My review is supplemental because I only checked avif.h, internal.h, and the top-level CMakelists.txt. You can merge this pull request with James and Vignesh's approval.

src/obu.c

include/avif/avif.h

include/avif/internal.h

CMakeLists.txt

jzern reviewed Apr 21, 2023

View reviewed changes

CMakeLists.txt Outdated Show resolved Hide resolved

CMakeLists.txt Show resolved Hide resolved

y-guyon force-pushed the add_avm branch from 1ccb180 to 4b7c808 Compare April 24, 2023 08:18

y-guyon requested review from jzern, vigneshvg and wantehchang April 24, 2023 17:02

jzern reviewed Apr 26, 2023

View reviewed changes

src/codec_avm.c Outdated Show resolved Hide resolved

jzern reviewed Apr 26, 2023

View reviewed changes

src/obu.c Outdated Show resolved Hide resolved

vigneshvg requested changes Apr 26, 2023

View reviewed changes

y-guyon requested a review from vigneshvg April 27, 2023 14:13

vigneshvg requested changes Apr 27, 2023

View reviewed changes

y-guyon requested a review from vigneshvg April 28, 2023 10:44

vigneshvg reviewed Apr 28, 2023

View reviewed changes

y-guyon added 14 commits April 28, 2023 17:03

Duplicate ext/avm.cmd, src/codec_avm.c and ci-unix

071b312

For easier diff of later changes.

Add avm as a codec dependency in ext/

4e626f3

Apply CMake-format

2449a7b

Add experiment warning when AVIF_CODEC_AVM is ON

ea69b6d

Modify comment in obu.c about AV2 spec

1d99bc4

Check aom_codec_version() for avm anchor 4

6a98d37

Ignore av02 items and tracks unless AVIF_CODEC_AVM

f90d660

Replace function pointer in obu.c by boolean

507df43

avifEncoderUseAV2() in write.c

2bddab1

Use uint8_t* instead of [4] in read.c

0c8f953

Use snprintf instead of memcpy in read.c

ca55e47

Guard av2C by AVIF_CODEC_AVM in read.c

acbd773

Add avifCodecType in internal.h

edc65ff

y-guyon added 7 commits April 28, 2023 17:05

Add entries to CHANGELOG.md

4a7db05

Apply clang-format

e85667b

Remove internal changes from CHANGELOG.md

042eb58

Replace AVIF_CHECKERR by if conditions

f885a69

Check MIAF grid constraints at decoding

4f4c1d8

Skip unrelevant tiles in DecoderItemValidateProps

8c61709

Revert avifTilesCanBeDecodedWithSameCodecInstance

0a6577f

The codecType is now checked in avifDecoderParse() so no need to do it in avifDecoderNextImage(). Add comment.

y-guyon force-pushed the add_avm branch from d89b57c to 0a6577f Compare April 28, 2023 15:13

vigneshvg requested changes Apr 28, 2023

View reviewed changes

src/codec_avm.c Outdated Show resolved Hide resolved

vigneshvg approved these changes Apr 28, 2023

View reviewed changes

Add avmScaleQuantizer to codec_avm.c

261a053

jzern approved these changes May 2, 2023

View reviewed changes

wantehchang reviewed May 2, 2023

View reviewed changes

src/obu.c Outdated Show resolved Hide resolved

include/avif/avif.h Show resolved Hide resolved

include/avif/internal.h Outdated Show resolved Hide resolved

CMakeLists.txt Show resolved Hide resolved

wantehchang reviewed May 2, 2023

View reviewed changes

CMakeLists.txt Show resolved Hide resolved

y-guyon added 3 commits May 3, 2023 14:51

Replace libaom mentions by libavm in codec_avm.c

f63da74

Add more test cases in avifavmtest

854e19e

Remove AVIF_CODEC_TYPE_COUNT

ca0250a

y-guyon merged commit a900b0c into AOMediaCodec:main May 4, 2023
15 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add AV2 support #1361

Add AV2 support #1361

y-guyon commented Apr 20, 2023 •

edited

jzern left a comment

y-guyon commented Apr 24, 2023

vigneshvg commented Apr 25, 2023

jzern commented Apr 26, 2023

y-guyon commented Apr 26, 2023

vigneshvg left a comment

vigneshvg left a comment

vigneshvg left a comment

y-guyon commented Apr 28, 2023

vigneshvg left a comment

jzern May 2, 2023

y-guyon May 3, 2023

wantehchang left a comment

Add AV2 support #1361

Add AV2 support #1361

Conversation

y-guyon commented Apr 20, 2023 • edited

jzern left a comment

Choose a reason for hiding this comment

y-guyon commented Apr 24, 2023

vigneshvg commented Apr 25, 2023

jzern commented Apr 26, 2023

y-guyon commented Apr 26, 2023

vigneshvg left a comment

Choose a reason for hiding this comment

vigneshvg left a comment

Choose a reason for hiding this comment

vigneshvg left a comment

Choose a reason for hiding this comment

y-guyon commented Apr 28, 2023

vigneshvg left a comment

Choose a reason for hiding this comment

jzern May 2, 2023

Choose a reason for hiding this comment

y-guyon May 3, 2023

Choose a reason for hiding this comment

wantehchang left a comment

Choose a reason for hiding this comment

y-guyon commented Apr 20, 2023 •

edited