Experimental AVIF Sample Transform #2050

y-guyon · 2024-03-12T10:55:55Z

Allow encoding and decoding images of more than 12 bits, the limit of the AV1 format. This commit experiments 16-bit AVIF images, made of one primary lossless 8-bit image and one hidden lossy/lossless bit depth extension 8-bit image. The two are combined thanks to a Sample Transform derived image item. Alpha is supported.

Warning: This feature is experimental and not covered by the current AVIF specification.

wantehchang

Yannis: I made a first pass through the pull request, checking only that the pull request is essentially a no-op if AVIF_ENABLE_EXPERIMENTAL_SAMPLE_TRANSFORM is not defined.

I noticed some general cleanup or improvements in src/read.c and src/write.c. I suggest moving those changes and the new avifIsAlpha() function to a separate pull request that can be reviewed and checked in first.

The first version of avifIsAlpha() function would be simply:

avifBool avifIsAlpha(avifItemCategory itemCategory)
{
    if (itemCategory == AVIF_ITEM_ALPHA) {
        return AVIF_TRUE;
    }
    return AVIF_FALSE;
}

src/read.c

src/write.c

y-guyon · 2024-04-05T11:46:25Z

Rebased to include #2057 and #2059.

wantehchang · 2024-04-08T23:16:27Z

Yannis: I have just started reviewing this pull request. I may not be able to post comments until tomorrow. I will mainly verify that the pull request is essentially a no-op if AVIF_ENABLE_EXPERIMENTAL_SAMPLE_TRANSFORM is not defined.

wantehchang

I only reviewed up to avif.h carefully. The rest of the files I only verified that the changes are a no-op if the experimental flag is disabled.

I suggest some small changes (mostly comments).

include/avif/avif.h

src/write.c

wantehchang

LGTM. I verified that the pull request is essentially a no-op if the experimental feature is disabled.

As real code review, I have reviewed up to avif.c. I also did a little checking in read.c.

include/avif/avif.h

include/avif/internal.h

src/read.c

wantehchang · 2024-04-09T23:33:05Z

src/read.c

+
+        AVIF_CHECK(avifROStreamRead(s, &token->value, /*size=*/1)); // unsigned int(8) token;
+        if (token->value == AVIF_SAMPLE_TRANSFORM_CONSTANT) {
+            // TODO(yguyon): Verify two's complement representation is guaranteed here.


I think we have verified this.

Is there some public resource I can cite here?

No. We have verified this by private email with Cyril Concolato and Emmanuel Thomas. There is a clarification in the current draft 14496-34: “Signed integer values shall be represented using the two’s-complement representation.”

wantehchang · 2024-04-09T23:33:58Z

src/read.c

+            // TODO(yguyon): Verify two's complement representation is guaranteed here.
+            uint32_t constant;
+            AVIF_CHECK(avifROStreamReadU32(s, &constant)); // signed int(1<<(bit_depth+3)) constant;
+            token->constant = *(int32_t *)&constant;       // maybe =(int32_t)constant; is enough


Yes, =(int32_t)constant; is enough. Alternatively, we can do this:

AVIF_CHECK(avifROStreamReadU32(s, (uint32_t *)&token->constant)); // signed int(1<<(bit_depth+3)) constant;

Yes, =(int32_t)constant; is enough.

https://en.cppreference.com/w/c/language/conversion#Integer_conversions seems to say it is implementation-defined. Should we consider it as widespread as two's complement representation?

Alternatively, we can do this:

AVIF_CHECK(avifROStreamReadU32(s, (uint32_t *)&token->constant)); // signed int(1<<(bit_depth+3)) constant;

I do not think it is equivalent depending on the platform's endianness. Should we consider it as widespread as two's complement representation?

The reason casting a uint32_t value greater than INT32_MAX to int32_t is implementation-defined is that there are three representations of signed integers in C. In practice, the only widely-used repesentation is two's complement.

src/read.c

src/write.c

wantehchang · 2024-04-10T22:24:36Z

include/avif/internal.h

+typedef struct avifSampleTransformToken
+{
+    uint8_t type;                // avifSampleTransformTokenType
+    int32_t constant;            // If value is AVIF_SAMPLE_TRANSFORM_CONSTANT.


Nit: value => type

wantehchang · 2024-04-10T22:25:01Z

src/read.c

@@ -2006,6 +2023,108 @@ static avifBool avifParseToneMappedImageBox(avifGainMapMetadata * metadata, cons
 }
 #endif // AVIF_ENABLE_EXPERIMENTAL_GAIN_MAP

+#if defined(AVIF_ENABLE_EXPERIMENTAL_SAMPLE_TRANSFORM)
+// bit-depth is assumed to be 2 (32-bit).


Nit: bit-depth => bit_depth ?

src/read.c

Allow encoding and decoding images of more than 12 bits, the limit of the AV1 format. This commit experiments 16-bit AVIF images, made of one primary lossless 8-bit image and one hidden lossy/lossless bit depth extension 8-bit image. The two are combined thanks to a Sample Transform derived image item. Alpha is supported. Warning: This feature is experimental and not covered by the current AVIF specification.

wantehchang

Yannis: I checked parts of read.c and sampletransform.c. This is probably the best I can do by the end of this week. You can go ahead and merge this pull request.

src/read.c

src/sampletransform.c

y-guyon mentioned this pull request Mar 12, 2024

Experimental AVIF Sample Transform bit depth ext #1572

Closed

y-guyon changed the title ~~Experimental AVIF Sample Transform bit depth ext~~ Experimental AVIF Sample Transform Mar 12, 2024

y-guyon force-pushed the sample_transform_expression branch from ef9dda8 to 8960194 Compare March 12, 2024 11:50

y-guyon marked this pull request as ready for review March 12, 2024 16:38

y-guyon requested a review from wantehchang March 12, 2024 16:38

wantehchang reviewed Mar 14, 2024

View reviewed changes

src/read.c Outdated Show resolved Hide resolved

wantehchang reviewed Mar 15, 2024

View reviewed changes

src/write.c Outdated Show resolved Hide resolved

y-guyon force-pushed the sample_transform_expression branch 2 times, most recently from b8e3537 to acfbd1e Compare April 5, 2024 11:46

y-guyon requested a review from wantehchang April 5, 2024 15:36

wantehchang reviewed Apr 8, 2024

View reviewed changes

y-guyon requested a review from wantehchang April 9, 2024 11:25

wantehchang approved these changes Apr 9, 2024

View reviewed changes

wantehchang approved these changes Apr 10, 2024

View reviewed changes

y-guyon force-pushed the sample_transform_expression branch from e568a38 to 148f165 Compare April 11, 2024 09:24

y-guyon added 8 commits April 11, 2024 17:07

Move isSampleTransformImage behind compile flag

954901b

Add single-operand operators

c45532d

Fix comments

c80761f

Fix comment

b992180

Rename avifSampleTransformToken::value to type

63cf01e

Apply comments

a70db43

Nits

c4919d5

y-guyon force-pushed the sample_transform_expression branch from 148f165 to c4919d5 Compare April 11, 2024 15:07

wantehchang approved these changes Apr 11, 2024

View reviewed changes

src/read.c Show resolved Hide resolved

src/read.c Outdated Show resolved Hide resolved

src/read.c Outdated Show resolved Hide resolved

src/read.c Outdated Show resolved Hide resolved

src/read.c Outdated Show resolved Hide resolved

src/sampletransform.c Outdated Show resolved Hide resolved

Apply comments

1aafcb1

y-guyon merged commit 834019c into AOMediaCodec:main Apr 12, 2024
28 checks passed

y-guyon deleted the sample_transform_expression branch April 12, 2024 09:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Experimental AVIF Sample Transform #2050

Experimental AVIF Sample Transform #2050

y-guyon commented Mar 12, 2024

wantehchang left a comment

y-guyon commented Apr 5, 2024 •

edited

Loading

wantehchang commented Apr 8, 2024

wantehchang left a comment

wantehchang left a comment

wantehchang Apr 9, 2024

y-guyon Apr 10, 2024

wantehchang Apr 10, 2024

wantehchang Apr 9, 2024

y-guyon Apr 10, 2024

wantehchang Apr 10, 2024

wantehchang Apr 10, 2024

wantehchang Apr 10, 2024

wantehchang left a comment

Experimental AVIF Sample Transform #2050

Experimental AVIF Sample Transform #2050

Conversation

y-guyon commented Mar 12, 2024

wantehchang left a comment

Choose a reason for hiding this comment

y-guyon commented Apr 5, 2024 • edited Loading

wantehchang commented Apr 8, 2024

wantehchang left a comment

Choose a reason for hiding this comment

wantehchang left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wantehchang left a comment

Choose a reason for hiding this comment

y-guyon commented Apr 5, 2024 •

edited

Loading