Significantly simplify gain map API #2481
Conversation
Gain map metadata is now alwayas parsed (when gain map code is enabled at compile time). Ideally, gain map metadata should be stored towards the beginning of the file (since it's in the 'mdat' part).
It can be replaced with a check that image->gainMap is not NULL.
| AVIF_CONTENT_ALL = AVIF_CONTENT_COLOR | AVIF_CONTENT_ALPHA, | ||
| #endif | ||
|
|
||
| AVIF_CONTENT_DECODE_DEFAULT = AVIF_CONTENT_COLOR | AVIF_CONTENT_ALPHA, |
There was a problem hiding this comment.
Nit: DECODE can be omitted, unless we are planning to add AVIF_CONTENT_ENCODE_DEFAULT in the future.
| // Types of image content that can be encoded/decoded. | ||
| typedef enum avifImageContentTypeFlag | ||
| { | ||
| AVIF_CONTENT_NONE = 0, |
There was a problem hiding this comment.
Nit: I suggest adding IMAGE to the enum constant names, i.e., AVIF_IMAGE_CONTENT_NONE, AVIF_IMAGE_CONTENT_COLOR, etc., to match the enum type name avifImageContentTypeFlag.
| // the gain map's planes can also be accessed in the same way. If the gain map's height is different from | ||
| // the main image, then the number of available gain map rows is at least: | ||
| // If a gain map is present and AVIF_ENABLE_EXPERIMENTAL_GAIN_MAP is on and | ||
| // (imageContentToDecode & AVIF_CONTENT_GAIN_MAP) is non zero, the gain map's planes can also be accessed |
There was a problem hiding this comment.
Nit: non zero => nonzero
| @@ -55,7 +54,7 @@ avifResult PrintMetadataCommand::Run() { | |||
| << decoder->diag.error << ")\n"; | |||
| return result; | |||
| } | |||
| if (!decoder->gainMapPresent) { | |||
| if (decoder->image->gainMap == nullptr) { | |||
There was a problem hiding this comment.
Nit: We can change NULL (e.g., at line 41) to nullptr in a separate pull request. Also check the other .cc files in this directory.
| { | ||
| AVIF_CONTENT_NONE = 0, | ||
| AVIF_CONTENT_COLOR = (1 << 1), | ||
| AVIF_CONTENT_ALPHA = (1 << 2), |
There was a problem hiding this comment.
IMPORTANT: These two enum constants are more expressive than the original ignoreColorAndAlpha boolean. I'd like to confirm with you that this is intended.
Note: ignoreColorAndAlpha is equivalent to AVIF_CONTENT_COLOR | AVIF_CONTENT_ALPHA. If we only want to match ignoreColorAndAlpha exactly, these two enum contants should be merge into AVIF_CONTENT_COLOR_AND_ALPHA.
If this is intended, we need to do the following:
- Inspect the API functions that take
avifImageas input and make sure that they can handle anavifImagein whichyuvPlanes[i]are all null butalphaPlaneis not null. I took a quick look and found that most of them already handle a nullyuvPlanes[[AVIF_CHAN_Y]properly. ButavifImagePlaneWitdh()andavifImagePlaneHeight()probably should return 0 ifimage->yuvPlanes[channel]is null. - Then add a test for
decoder->imageContentToDecode == AVIF_CONTENT_ALPHA. Verifydecoder->image->yuvPlanes[i]are all null anddecoder->image->yuvRowBytes[i]are all zero, and call some API functions ondecoder->image.
| @@ -1174,6 +1174,23 @@ typedef enum avifProgressiveState | |||
| } avifProgressiveState; | |||
| AVIF_API const char * avifProgressiveStateToString(avifProgressiveState progressiveState); | |||
|
|
|||
| // Types of image content that can be encoded/decoded. | |||
There was a problem hiding this comment.
Nit: It seems that this enum type is only applicable to decoding. The encoder probably should encode all the image content in the input avifImage. See also my comment at line 1190.
| The `gainMapPresent` field is now only populated if enableParsingGainMapMetadata | ||
| is true. | ||
| Simplify gain map API: remove the enableParsingGainMapMetadata setting, now gain | ||
| map metadata is always parsed if present and if this feature is compiled in. |
There was a problem hiding this comment.
Since gain map metadata is always parsed if present, we should write gain map metadata early in the file.
In src/write.c, we have
static avifResult avifEncoderWriteMediaDataBox(avifEncoder * encoder,
avifRWStream * s,
avifEncoderItemReferenceArray * layeredColorItems,
avifEncoderItemReferenceArray * layeredAlphaItems)
{
...
for (uint32_t itemPasses = 0; itemPasses < 3; ++itemPasses) {
// Use multiple passes to pack in the following order:
// * Pass 0: metadata (Exif/XMP)
// * Pass 1: alpha, gain map (AV1)
// * Pass 2: all other item data (AV1 color)
Should we move gain map from Pass 1 to Pass 0 or even a new pass before the current Pass 0?
enableParsingGainMapMetadata: gain map metadata is always parsed nowenableDecodingGainMapandignoreColorAndAlphawithimageContentToDecodebit fieldgainMapPresent, the same information is available by checkingdecoder->image->gainMap != NULL