Add experimental support for reading and writing gain maps. #1501

maryla-uc · 2023-08-11T14:05:05Z

The feature is hidden behind the AVIF_ENABLE_EXPERIMENTAL_GAIN_MAP compile flag.

See https://helpx.adobe.com/camera-raw/using/gain-map.html for an introduction to gain maps. Note that in this change, gain maps are not stored as auxiliary items as proposed by Adobe, but as a hidden input to a new 'tmap' (tone mapped) derived item.
An 'altr' (alternatives group) box is also added to tell the decoder that it may show the tone mapped image (preferrably) or the base image as a fall back. The primary item id still points to the base image for backward compatibility.

CMakeLists.txt

include/avif/avif.h

src/write.c

src/read.c

include/avif/avif.h

y-guyon · 2023-08-19T16:51:41Z

src/write.c

-                // only process alpha payloads when alphaPass is true
+            avifBool isAlphaOrGainMap = item->itemCategory == AVIF_ITEM_ALPHA;
+#if defined(AVIF_ENABLE_EXPERIMENTAL_GAIN_MAP)
+            isAlphaOrGainMap = isAlphaOrGainMap || item->itemCategory == AVIF_ITEM_GAIN_MAP || !memcmp(item->type, "tmap", 4);


Could be isAlphaOrGainMap |=, fine as is as well

That would work but it's not exactly the same thing since |= is a bitwise operator, and won't skip evaluating the right side if the value is already true. Not that I think the performance matters much, but I think it's better to keep |= for actual bitwise operations like flags like we do now.

tests/gtest/avifgainmaptest.cc

y-guyon · 2023-08-19T17:16:53Z

tests/gtest/avifgridapitest.cc

+  const int num_cols = (int)cell_rows[0].size();
+  AVIF_CHECKRES(
+      testutil::MergeGrid(num_cols, num_rows, cell_images, grid.get()));
+


Rows below should be removed now that there is testutil::MergeGrid()

Note: testutil::MergeGrid() could be made in another PR to reduce the size of this one

Indeed I forgot to remove them, thanks for spotting it.
I agree it could be split... but I'm afraid of spending a while fighting with git and merges again... So if it's not a strong request I'd rather leave it as is.

tests/gtest/aviftest_helpers.cc

maryla-uc · 2023-08-21T15:56:56Z

Note I rebased this patch but had some issues with the merge and I had to add my latests fixes to the last commit, apologies for the confusing history.

wantehchang

Maryla: I only reviewed the public header avif.h. I suggest some changes in anticipation of the libavif 1.0.0 release.

include/avif/avif.h

CMakeLists.txt

tests/gtest/avifgridapitest.cc

src/write.c

include/avif/avif.h

Requested changes have been made

wantehchang · 2023-08-25T11:38:30Z

apps/avifenc.c

@@ -1945,7 +1945,7 @@ int main(int argc, char * argv[])
        }
    }

-    avifIOStats ioStats = { 0, 0, 0 };
+    avifIOStats ioStats = { 0, 0 };


Maryla: It would be good to modify the following code in avifenc.c:

printf("Encoded successfully.\n"); printf(" * Color AV1 total size: %" AVIF_FMT_ZU " bytes\n", ioStats.colorOBUSize); printf(" * Alpha AV1 total size: %" AVIF_FMT_ZU " bytes\n", ioStats.alphaOBUSize);

to also print the gain map AV1 total size, and see if you find it useful to report that information.

Actually, there's no way to encode a gain map with avifenc right now, only through the API, so this can be done later I think.

wantehchang

avif.h LGTM.

The feature is hidden behind the AVIF_ENABLE_EXPERIMENTAL_GAIN_MAP compile flag. See https://helpx.adobe.com/camera-raw/using/gain-map.html for an introduction to gain maps. Note that in this change, gain maps are not stored as auxiliary items as proposed by Adobe, but as a hidden input to a new 'tmap' (tone mapped) derived item. An 'altr' (alternatives group) box is also added to tell the decoder that it may show the tone mapped image (preferrably) or the base image as a fall back. The primary item id still points to the base image for backward compatibility.

Move new fields to the end of structs. Add back accidentally deleted test code. Remove gainMapOBUSize.

wantehchang

Maryla: Could you add an entry to CHANGELOG.md? Please indicate that this is an experimental feature. Thanks.

wantehchang

Maryla: In this round of review, I only checked whether the pull request is essentially a no-op if the experimental feature is disabled. Unfortunately I didn't make a complete pass through the PR. (I stopped after src/read.c, so I haven't reviewed src/write.c.)

This PR has more code and comments outside #if defined(AVIF_ENABLE_EXPERIMENTAL_GAIN_MAP). We should try to move as much code into #if defined(AVIF_ENABLE_EXPERIMENTAL_GAIN_MAP) as possible. It is more difficult to deal with the comments. I suggested adding "if AVIF_ENABLE_EXPERIMENTAL_GAIN_MAP is defned" to the comments, but that may become a little wordy.

wantehchang · 2023-08-30T23:51:53Z

include/avif/avif.h

+// images containing a gain map encoded with the current version of libavif might
+// not decode with a feature future version of libavif. Use are your own risk.
+// This is based on ISO/IEC JTC 1/SC 29/WG 3 m64379
+// This product includes Gain Map technology under license by Adobe.


Nit: Move this comment block inside #if defined(AVIF_ENABLE_EXPERIMENTAL_GAIN_MAP)

wantehchang · 2023-08-30T23:52:22Z

include/avif/avif.h

@@ -577,6 +680,7 @@ AVIF_API avifResult avifImageSetMetadataExif(avifImage * image, const uint8_t *
 // Sets XMP metadata.
 AVIF_API avifResult avifImageSetMetadataXMP(avifImage * image, const uint8_t * xmp, size_t xmpSize);

+// Allocate/free/steal planes. These functions do not affect the gain map image if present.


Nit/optional: In these new comments (lines 668, 670, 683), add "if AVIF_ENABLE_EXPERIMENTAL_GAIN_MAP is defined".

wantehchang · 2023-08-30T23:52:59Z

include/avif/internal.h

@@ -92,6 +93,7 @@ void avifImageCopyNoAlloc(avifImage * dstImage, const avifImage * srcImage);
 // Copies the samples from srcImage to dstImage. dstImage must be allocated.
 // srcImage and dstImage must have the same width, height, and depth.
 // If the AVIF_PLANES_YUV bit is set in planes, then srcImage and dstImage must have the same yuvFormat and yuvRange.
+// The gain map (if present) is not affected.


Nit/optional: Add "if AVIF_ENABLE_EXPERIMENTAL_GAIN_MAP is defined" to this comment.

wantehchang · 2023-08-30T23:53:43Z

src/read.c

@@ -681,7 +683,7 @@ typedef struct avifTile
    avifCodecDecodeInput * input;
    avifCodecType codecType;
    // This may point to a codec that it owns or point to a shared codec that it does not own. In the shared case, this will
-    // point to one of avifDecoderData.codec or avifDecoderData.codecAlpha.
+    // point to one of avifDecoderData.codec, avifDecoderData.codecAlpha or avifDecoderData.codecGainMap.


Nit: Add "if AVIF_ENABLE_EXPERIMENTAL_GAIN_MAP is defined" to this comment.

wantehchang · 2023-08-30T23:54:04Z

src/read.c

@@ -815,6 +817,7 @@ typedef struct avifDecoderData
    avifTileArray tiles;
    avifTileInfo color;
    avifTileInfo alpha;
+    avifTileInfo gainMap;


Nit: Put inside #if defined(AVIF_ENABLE_EXPERIMENTAL_GAIN_MAP).

wantehchang · 2023-08-30T23:54:20Z

src/read.c

@@ -830,6 +833,7 @@ typedef struct avifDecoderData
    //   decoder instance (same as above).
    avifCodec * codec;
    avifCodec * codecAlpha;
+    avifCodec * codecGainMap;


Nit: Put inside #if defined(AVIF_ENABLE_EXPERIMENTAL_GAIN_MAP).

wantehchang · 2023-08-30T23:55:17Z

src/read.c

@@ -4025,7 +4246,7 @@ avifResult avifDecoderReset(avifDecoder * decoder)

            if (tile->input->itemCategory == AVIF_ITEM_ALPHA) {
                decoder->ioStats.alphaOBUSize += sample->size;
-            } else {
+            } else if (tile->input->itemCategory == AVIF_ITEM_COLOR) {


Nit: Reorder this in more natural order:

if (tile->input->itemCategory == AVIF_ITEM_COLOR) { decoder->ioStats.colorOBUSize += sample->size; } else if (tile->input->itemCategory == AVIF_ITEM_ALPHA) { decoder->ioStats.alphaOBUSize += sample->size; }

wantehchang · 2023-08-30T23:56:12Z

src/read.c

+                    assert(dstImage);
+                }
+#endif
+                AVIF_CHECKRES(avifDecoderDataAllocateGridImagePlanes(decoder->data, info, dstImage));


Should we also change decoder->image to dstImage below, at line 4495?

wantehchang · 2023-08-30T23:58:00Z

src/read.c

+            } else
+#endif
+                if ((decoder->image->width != src->width) || (decoder->image->height != src->height) ||
+                    (decoder->image->depth != src->depth)) {


The nesting of #if and if breaks indentation. We should try to restructure this, e.g., as a switch statement:

switch (tile->input->itemCategory) { #if defined(AVIF_ENABLE_EXPERIMENTAL_GAIN_MAP) case AVIF_ITEM_GAIN_MAP: decoder->image->gainMap.image->width = src->width; decoder->image->gainMap.image->height = src->height; decoder->image->gainMap.image->depth = src->depth; break; #endif default: ... break }

You can experiment with other alternatives.

wantehchang · 2023-08-30T23:58:37Z

src/write.c

 typedef struct avifEncoderData
 {
    avifEncoderItemArray items;
    avifEncoderFrameArray frames;
    // Map the encoder settings quality and qualityAlpha to quantizer and quantizerAlpha
    int quantizer;
    int quantizerAlpha;
+    int quantizerGainMap;


Nit: Put this inside #if defined(AVIF_ENABLE_EXPERIMENTAL_GAIN_MAP).

Reverts changes in avifDecoderItemShouldBeSkipped() introduced in AOMediaCodec#1501 The new changes are made in code garded by AVIF_ENABLE_EXPERIMENTAL_GAIN_MAP.

Reverts changes in avifDecoderItemShouldBeSkipped() introduced in #1501 The new changes are made in code guarded by AVIF_ENABLE_EXPERIMENTAL_GAIN_MAP.

And adress some more minor comments from AOMediaCodec#1501.

y-guyon reviewed Aug 17, 2023

View reviewed changes

src/read.c Show resolved Hide resolved

maryla-uc force-pushed the apple_gainmaps branch 5 times, most recently from 8801215 to f02b994 Compare August 18, 2023 13:45

maryla-uc requested a review from y-guyon August 18, 2023 16:42

y-guyon approved these changes Aug 19, 2023

View reviewed changes

maryla-uc force-pushed the apple_gainmaps branch from f02b994 to 2e80228 Compare August 21, 2023 15:53

wantehchang previously requested changes Aug 22, 2023

View reviewed changes

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

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

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

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

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

wantehchang reviewed Aug 22, 2023

View reviewed changes

CMakeLists.txt Show resolved Hide resolved

y-guyon reviewed Aug 22, 2023

View reviewed changes

tests/gtest/avifgridapitest.cc Show resolved Hide resolved

src/write.c Outdated Show resolved Hide resolved

wantehchang reviewed Aug 22, 2023

View reviewed changes

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

wantehchang mentioned this pull request Aug 23, 2023

Add "Version 1.0.0 ends here" comments #1510

Merged

wantehchang reviewed Aug 25, 2023

View reviewed changes

wantehchang reviewed Aug 28, 2023

View reviewed changes

maryla-uc added 8 commits August 29, 2023 14:23

Address minor review comments.

9af52dc

Only add avifgainmaptest when the cmake flag is on.

9ece071

Use explicit numerators and denominators in the API.

4d7b970

Remove float to fraction code, it can be added later.

378a26f

Address more small review comments.

82ced16

More review comments addressed.

3fd40ee

Move new fields to the end of structs. Add back accidentally deleted test code. Remove gainMapOBUSize.

Move TODO.

072c49a

maryla-uc force-pushed the apple_gainmaps branch from 0074195 to 072c49a Compare August 29, 2023 12:58

maryla-uc merged commit 50a5414 into AOMediaCodec:main Aug 29, 2023
14 checks passed

wantehchang reviewed Aug 29, 2023

View reviewed changes

wantehchang reviewed Aug 31, 2023

View reviewed changes

maryla-uc mentioned this pull request Aug 31, 2023

Fix bug with unhandled 'tmap' items as primary image or color. #1545

Merged

maryla-uc added a commit that referenced this pull request Aug 31, 2023

Fix 'tmap' items not getting filtered out. (#1545)

0d9ef99

Reverts changes in avifDecoderItemShouldBeSkipped() introduced in #1501 The new changes are made in code guarded by AVIF_ENABLE_EXPERIMENTAL_GAIN_MAP.

maryla-uc added a commit to maryla-uc/libavif that referenced this pull request Aug 31, 2023

Put more code behind AVIF_ENABLE_EXPERIMENTAL_GAIN_MAP.

1844685

And adress some more minor comments from AOMediaCodec#1501.

maryla-uc added a commit to maryla-uc/libavif that referenced this pull request Aug 31, 2023

Put more code behind AVIF_ENABLE_EXPERIMENTAL_GAIN_MAP.

bde02e3

And adress some more minor comments from AOMediaCodec#1501.

maryla-uc mentioned this pull request Aug 31, 2023

Put more code behind AVIF_ENABLE_EXPERIMENTAL_GAIN_MAP. #1547

Merged

y-guyon mentioned this pull request Sep 10, 2023

Remove unused avifDecoderData::codecGainMap #1569

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add experimental support for reading and writing gain maps. #1501

Add experimental support for reading and writing gain maps. #1501

maryla-uc commented Aug 11, 2023

y-guyon Aug 19, 2023

maryla-uc Aug 21, 2023

y-guyon Aug 19, 2023

maryla-uc Aug 21, 2023

maryla-uc commented Aug 21, 2023

wantehchang left a comment

wantehchang Aug 25, 2023

maryla-uc Aug 25, 2023

wantehchang left a comment

wantehchang left a comment

wantehchang left a comment

wantehchang Aug 30, 2023

wantehchang Aug 30, 2023

wantehchang Aug 30, 2023

wantehchang Aug 30, 2023

wantehchang Aug 30, 2023

wantehchang Aug 30, 2023

wantehchang Aug 30, 2023

wantehchang Aug 30, 2023

wantehchang Aug 30, 2023

wantehchang Aug 30, 2023

Add experimental support for reading and writing gain maps. #1501

Add experimental support for reading and writing gain maps. #1501

Conversation

maryla-uc commented Aug 11, 2023

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

maryla-uc commented Aug 21, 2023

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

wantehchang left a comment

Choose a reason for hiding this comment

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment