Add support for reading jpeg gain maps and converting them to AVIF.

[maryla-uc]

Needs libxml2 and AVIF_ENABLE_EXPERIMENTA_GAIN_MAP to be turned on.

The parsing code is a little long because there are basically three parts:

• MPF (Multi-Picture Format) metadata parsing, to find the offset(s) of the additional image(s) in the file. These offsets are relative fo the position of the MPF segment.
• JPEG metadata segment parsing, to find the offset of the MPF segment, in order to make the image offsets absolute (unfortunately jpeglib does not provide this information)
• XMP parsing (using libxml2) for the gain map metadata

[y-guyon]

Still have apps/shared/avifjpeg.c to review

[y-guyon]

Could there be a reasonable maximum loop count to avoid slow fuzzing?

Note: This file is not even fuzzed yet, because avifJPEGRead() reads a file and not a buffer in memory.

[maryla-uc]

Done. More fuzzer coverage would indeed be good. I'll work on adding more tests/fuzzing.

[wantehchang]

IMPORTANT: Since avifGainMapMetadataDoubleToFractions() is exported from the libavif shared library (for use by avifjpeg.c), it is part of the public API. Please move this declaration to the public header avif.h.

After we move this declaration to avif.h, gainmap.h becomes an empty file and should be deleted.

Note: An alternative is to move avifGainMapMetadataDoubleToFractions() to avifjpeg.c (as a static function) or avifutil.{h,c}, if this function should not be in the public API of the libavif library.

[maryla-uc]

This was intentional. I think avifGainMapMetadataDoubleToFractions should be in the public API to make it easier to fill in avifGainMapMetadata. I also thought this type of gain map related utility function (I expect more to be added in the future) could be in a separate header file, in order not to clutter avif.h
What's the rationale for having all the public api functions in a single header file?

[maryla-uc]

Another option would be to #include gainmap.h from avif.h so that users only need to #include avif.h. But it's not really in the "include what you use" philosophy.

[wantehchang]

The rationale for having all the public api functions in avif/avif.h is simplicity. This design is quite common. For example, libpng declares all the public api functions in png.h:

https://packages.debian.org/sid/amd64/libpng-dev/filelist

Since it's not critical to add an avif/gainmap.h header, it would be better to stick to the current design.

I just wrote a pull request #1589 to document the current libavif header inclusion policy.

[maryla-uc]

A single header file is only "simpler" in the sense that you only have one header file to include, but I think it makes the API more difficult to understand and navigate (the file is 1400 lines long and counting).

This single header file design also means we can't split the encoder and decoder, and even apps that only need decoding need to include the encoder (the libavif part anyway). libaom and libwebp both have separate encoding and decoding headers https://aomedia.googlesource.com/aom/+/refs/heads/main/aom/ https://chromium.googlesource.com/webm/libwebp/+/refs/heads/main/src/webp/

And of course, many more libraries have their API split into multiple header files, for example libxml2 which I used recently https://gitlab.gnome.org/GNOME/libxml2/-/tree/master/include/libxml

In particular, gainmap utilities like this are not really tied to the main libavif API and I still think it would be better not to clutter avif.h with them.
Again, if we really think it's important that users should only need to include one header, we can "export" gainmap.h from avif.h the same way internal.h exports avif.h?

[wantehchang]

Based on my code analysis alone, I would put avifGainMapMetadataDoubleToFractions() in apps/shared/avifutil.{h,c} or avifjpeg.c, and wait until a user requests it to move it to the libavif library. This is a safe strategy when it is not clear whether a new utility function is useful enough to be added to the library, because the library has backward compatibility requirements now that version 1.0.0 has been released. But you are much more familiar with gain maps and may already know this utility function is generally useful. So I deter this decision to you.

As for adding a separate public header, I don't strongly prefer a single header over multiple headers; I am equally comfortable in both designs. It's simply that the current monolithic header has never bothered me (and I read/analyze code frequently), so I see no need to change. (And include-what-you-use is simple when there is only one header.) If we break away from the monolithic header, we need to do it properly.

[maryla-uc]

I see what you mean and I do understand the concern about API maintenainance. But I also think it's a useful utility for anybody who would want to use this API. Double (or float) values are the natural way of representing all of these metadata fields. The fact that we use fractions is just an implementation detail. Anybody who would want to use the API would have to convert floating point values to fractions, which is why I wanted to provide this utility.

Similarly, we have avifCropRectConvertCleanApertureBox() and avifCleanApertureBoxConvertCropRect() to make the clean aperture feature easier to use.

Regarding header files, I think I prefer multiple headers in general but until we make a final decision on this I guess it can go in avif.h.

This is all still behind a compile flag so nothing is set in stone anyway.

[wantehchang]

Do you think our public API should just take double (or float) values and then we convert them internally to fractions? Or is it better for the public API to reflect the file format faithfully?

I guess some clients may prefer to specify the values as fractions so they can control the values precisely.

[maryla-uc]

I wondered about this too, and at first I had double values in the API since I copied the Skia interface. But using fractions indeed gives more control and allows retrieving exactly the values that were written in the file.

Yannis suggested using fractions with a helper utility like we do for clap and I thought it made sense. This way the clap and gain map APIs are consistent in that they expose the numerator/denominator fields.

But overall I don't think any of these arguments are very strong so I'm also open to changing it back to double values if you think it's better/simpler.

[wantehchang]

I think double values are simpler; what I don't know is whether any of our users need to control precisely the values written to the file. We can re-evaluate this when we officially support gain maps.