Allow creating images with a different grid for the gain map. #2397

maryla-uc · 2024-08-19T14:52:04Z

The main image can have a different number of cols/rows than the gain map, or one can be a grid and the other be a single image. Also allow setting differnet values for gain map metadata version fields.
Add code in gainmaptest.cc to generate test images. These images are used in tests and as seeds by the fuzzer.

This PR removes the need to maintain a separate branch to generate/update some of the test images which was becoming burdernsome.
WIth the ability to create these test images in the main branch, the images don't actually need to be stored in the repo but could be created on the fly in the relevant tests. However, having them in the repo allows them to be used as seeds by the fuzzer, and they can be copied easily to CrabbyAvif for testing there.

#2391

y-guyon · 2024-08-19T15:00:58Z

avifEncoderInternalOptions seems convenient so LGTM on the internal API change but I will wait for other opinions before reviewing the whole PR.

maryla-uc · 2024-08-19T15:22:08Z

Thanks for the feedback. I thought this might be a bit controversial so happy to hear other people's opinions. I think it's nice to be able to create a wider variety of images from within the code base without having to keep some old branch lying around, or doing some surgery on the files (either manual or in test code as in this example). But I acknowledge that this adds some extra complexity to the already complex/long write.c file.

The main image can have a different number of cols/rows than the gain map, or one can be a grid and the other be a single image. Also allow setting differnet values for gain map metadata version fields. Add code in gainmaptest.cc to generate test images. These images are used in tests and as seeds by the fuzzer.

wantehchang · 2024-08-22T05:16:31Z

Maryla: I will take a closer look at this pull request tomorrow.

wantehchang · 2024-08-21T21:36:25Z

include/avif/internal.h

+    uint16_t tmapWriterVersion;  // Value that should be written for the 'writerVersion' field (use default if 0)
+    avifBool tmapAddExtraBytes;  // Add arbitrary bytes at the end of the box
+#endif
+    char dummy; // Avoid emptry struct error when AVIF_ENABLE_EXPERIMENTAL_GAIN_MAP is off


Nit: dummy => unused

wantehchang · 2024-08-22T22:45:36Z

include/avif/internal.h

@@ -786,6 +786,37 @@ avifResult avifFindMinMaxWithoutOutliers(const float * gainMapF, int numPixels,

 #endif // AVIF_ENABLE_EXPERIMENTAL_GAIN_MAP

+// ---------------------------------------------------------------------------
+// Internal encode


(This is a comment on the pull request.) I probably would add the generated test images to the source tree rather than generate them on the fly during testing.

This PR consists of two parts. The avifEncoderInternalOptions part is straightforward. On the other hand, it is also easy to rewrite this part from scratch when necessary -- one just needs to make simple changes to the avifWriteToneMappedImagePayload() function. So it is less valuable to check in this part.

The avifEncoderAddImageInternal part is more complicated. It could be difficult to maintain. The new function parameters also need to be documented.

wantehchang · 2024-08-22T22:46:09Z

include/avif/internal.h

+// ---------------------------------------------------------------------------
+// Internal encode
+//
+// These functions/options give extra flexibility to create non standard images for use in testing.


Nit: non standard => nonstandard

wantehchang · 2024-08-22T22:46:22Z

include/avif/internal.h

+    uint8_t tmapVersion;         // Value that should be written for the 'version' field (use default if 0)
+    uint16_t tmapMinimumVersion; // Value that should be written for the 'minimum_version' field (use default if 0)
+    uint16_t tmapWriterVersion;  // Value that should be written for the 'writerVersion' field (use default if 0)
+    avifBool tmapAddExtraBytes;  // Add arbitrary bytes at the end of the box


Nit: replace "the box" with the name of the class (ToneMapImage or GainMapMetadata?)

wantehchang · 2024-08-22T22:47:20Z

src/write.c

@@ -952,7 +966,7 @@ size_t avifEncoderGetGainMapSizeBytes(avifEncoder * encoder)
 }

 // Sets altImageMetadata's metadata values to represent the "alternate" image as if applying the gain map to the base image.
-static avifResult avifImageCopyAltImageMetadata(avifImage * altImageMetadata, const avifImage * imageWithGainMap)
+static avifResult avifImageCopyAltImageMetadata(avifImage * altImageMetadata, const avifImage * imageWithGainMap, const avifImage * gainMapImage)


The only thing this function needs from gainMapImage is gainMapImage->depth. Should we change this parameter to uint32_t gainMapImageDepth?

wantehchang · 2024-08-22T22:48:07Z

src/write.c

@@ -1596,6 +1609,9 @@ static avifResult avifEncoderAddImageInternal(avifEncoder * encoder,
                                              uint32_t gridCols,
                                              uint32_t gridRows,
                                              const avifImage * const * cellImages,
+                                              uint32_t gainMapGridCols,
+                                              uint32_t gainMapGridRows,
+                                              const avifImage * const * gainMapCellImages,


This change introduces an asymmetry between the alpha grid and the gain map grid, because the alpha grid is not allowed to have a different tile configuration.

wantehchang · 2024-08-22T22:48:42Z

src/write.c

+    if ((gridCols == 0) || (gridCols > 256) || (gridRows == 0) || (gridRows > 256) || (gainMapGridCols == 0) ||
+        (gainMapGridCols > 256) || (gainMapGridRows == 0) || (gainMapGridRows > 256)) {
+        return AVIF_RESULT_INVALID_IMAGE_GRID;
+    }


Nit: It is better to ignore gainMapGridCols and gainMapGridRows when gainMapCellImages is null:

if ((gridCols == 0) || (gridCols > 256) || (gridRows == 0) || (gridRows > 256)) { return AVIF_RESULT_INVALID_IMAGE_GRID; } if (gainMapCellImages && ((gainMapGridCols == 0) || (gainMapGridCols > 256) || (gainMapGridRows == 0) || (gainMapGridRows > 256))) { return AVIF_RESULT_INVALID_IMAGE_GRID; }

wantehchang · 2024-08-22T22:48:58Z

src/write.c

+                                       /*gridRows=*/1,
+                                       &image,
+                                       /*gainMapGridCols=*/1,
+                                       /*gainMapGridRows=*/1,


If we make the changes I suggested at line 2163, we can pass 0 and 0 here.

wantehchang · 2024-08-22T22:49:05Z

src/write.c

+                                                       gridRows,
+                                                       cellImages,
+                                                       /*gainMapGridCols=*/gridCols,
+                                                       /*gainMapGridRows=*/gridRows,


If we make the changes I suggested at line 2163, we can pass 0 and 0 here.

wantehchang · 2024-08-22T22:58:43Z

src/write.c

@@ -1596,6 +1609,9 @@ static avifResult avifEncoderAddImageInternal(avifEncoder * encoder,
                                              uint32_t gridCols,
                                              uint32_t gridRows,
                                              const avifImage * const * cellImages,
+                                              uint32_t gainMapGridCols,
+                                              uint32_t gainMapGridRows,
+                                              const avifImage * const * gainMapCellImages,


It would be clearer if we always use the gainMapCellImages input parameter when there is a gain map. This requires building the gainMapCellImages array in the normal/standard case. If we do this, then a null gainMapCellImages means there is no gain map.

maryla-uc · 2024-08-26T09:11:55Z

Thanks for the review Wan-Teh.
I'm a bit unclear on your overall opinion, if you think it's reasonable to check in part or all of this. I have to say I'm not fully convinced anymore this is the best solution either.
On the one hand, I thought it was a pain to update this branch for the umpteenth time to update the images, and it seemed hacky to have this branch lying around in my personnal fork.
On the other hand, now that the gain map spec should be finalized, hopefully this branch won't need much updating anymore, and this introduces extra complexity in the already complex write.c file.
An other option would also be simply to add this branch to the main repo instead of having it just in my fork?

wantehchang · 2024-08-27T22:02:54Z

Hi Maryla,

I would add the generated test images to the source tree rather than generate them on the fly during testing. The code you used to generate the test images should be made available so that other people can reproduce your work. Publishing this pull request achieves that goal; we don't need to merge the pull request.

If you ask us for our opinion, we are likely to only consider the maintenance cost and ignore the efforts you spent on generating the test images. You are in the best position to evaluate the trade-offs and decide whether this pull request should be merged. I trust your judgment.

maryla-uc · 2024-08-28T11:07:45Z

Indeed currently we guarantee that gainMapPresent == (gainMap != NULL) so getting rid of the boolean could be an option. Although that might change if we changed the definition of gainMapPresent... We can revisit this later. As long as this is behind and EXPERIMENTAL compile flag I think it's ok to change the API (and I doubt a lot of people use it right now).

maryla-uc force-pushed the weirdgainmaps branch from 9ad2707 to af5863a Compare August 20, 2024 14:01

maryla-uc requested a review from wantehchang August 21, 2024 09:13

wantehchang reviewed Aug 22, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow creating images with a different grid for the gain map. #2397

Allow creating images with a different grid for the gain map. #2397

maryla-uc commented Aug 19, 2024 •

edited

Loading

y-guyon commented Aug 19, 2024

maryla-uc commented Aug 19, 2024

wantehchang commented Aug 22, 2024

wantehchang Aug 21, 2024

wantehchang Aug 22, 2024

wantehchang Aug 22, 2024

wantehchang Aug 22, 2024

wantehchang Aug 22, 2024

wantehchang Aug 22, 2024

wantehchang Aug 22, 2024

wantehchang Aug 22, 2024

wantehchang Aug 22, 2024

wantehchang Aug 22, 2024

maryla-uc commented Aug 26, 2024

wantehchang commented Aug 27, 2024

maryla-uc commented Aug 28, 2024

Allow creating images with a different grid for the gain map. #2397

Are you sure you want to change the base?

Allow creating images with a different grid for the gain map. #2397

Conversation

maryla-uc commented Aug 19, 2024 • edited Loading

y-guyon commented Aug 19, 2024

maryla-uc commented Aug 19, 2024

wantehchang commented Aug 22, 2024

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

maryla-uc commented Aug 26, 2024

wantehchang commented Aug 27, 2024

maryla-uc commented Aug 28, 2024

maryla-uc commented Aug 19, 2024 •

edited

Loading