Support update encoder settings during encoding

[tongyuantongyu]

This is the first part for #761.

@y-guyon, I'm using AVIF_ADD_IMAGE_FLAG_UPDATE_SETTINGS due to the following considerations, but I'm open to change:

• We want to update settings in avifEncoder.csOptions as well, but it's an kv map emulated using array, which is hard to do comparison.
• This avoids changing libavif's current behavior.

[y-guyon]

avifIO* is short enough, no need for auto.

[tongyuantongyu]

Done

[y-guyon]

kNoClamp

[tongyuantongyu]

Done

[y-guyon]

std::numeric_limits<uint64_t>::max() and #include <limits>

[tongyuantongyu]

Done

[y-guyon]

Prefer reinterpret_cast<>() in C++ code (although here we could argue that it is C code).

[tongyuantongyu]

Done

[y-guyon]

This could be a TEST_P with avifCodecChoice, /*allows_modifying_settings*/bool as parameters. It would shorten this file as the three tests share most logic and can be factored into a single one.

[tongyuantongyu]

TEST_P seems to test against different combinations of parameters. A helper function with some parameters looks more suitable here.

[y-guyon]

encoderFlags is only used at line 725 so the four lines above could be moved inside the condition.

(Unrelated note: I wonder what happens without this PR if you call avifEncoderAddImage() with different avifImage::depths)

[tongyuantongyu]

Done.

avifEncoderAddImage calls avifEncoderAddImageInternal, which already ensured subsequent images has same metadata with the first one at the very beginning "Validate images" section.

[y-guyon]

avifEncoderAddImage calls avifEncoderAddImageInternal, which already ensured subsequent images has same metadata with the first one at the very beginning "Validate images" section.

Thank you for taking a look.
Unfortunately the code you are referring to only makes sure all cells in a layer have the same features as the first cell, but they are not compared with the first layer.

libavif/src/write.c

Lines 642 to 649 in 1ce971e

	if ((cellImage->width != firstCell->width) || (cellImage->height != firstCell->height) ||
	(cellImage->depth != firstCell->depth) || (cellImage->yuvFormat != firstCell->yuvFormat) ||
	(cellImage->yuvRange != firstCell->yuvRange) || (cellImage->colorPrimaries != firstCell->colorPrimaries) ||
	(cellImage->transferCharacteristics != firstCell->transferCharacteristics) ||
	(cellImage->matrixCoefficients != firstCell->matrixCoefficients) || (!!cellImage->alphaPlane != !!firstCell->alphaPlane) ||
	(cellImage->alphaPremultiplied != firstCell->alphaPremultiplied)) {
	return AVIF_RESULT_INVALID_IMAGE_GRID;
	}

[tongyuantongyu]

Thanks for your careful check. The validation is indeed missing, and I'm adding it.

[y-guyon]

This is the first part for #761.

Thanks for doing that and also for adding a good unit test.

@y-guyon, I'm using AVIF_ADD_IMAGE_FLAG_UPDATE_SETTINGS due to the following considerations, but I'm open to change

I would rather avoid adding a flag if there is no need to. I agree it adds a few functions and it requires a backup of avifEncoder fields at every avifEncoderAddImage() call, so up to you.

• We want to update settings in avifEncoder.csOptions as well, but it's an kv map emulated using array, which is hard to do comparison.

avifBool avifEncoderChanged(const avifEncoder* before, const avifEncoder* after) {
    if (before->speed != after->speed ||
        ... ||
        before->csOptions.count != after->csOptions.count) {
        return AVIF_TRUE;
    }
    for (uint32_t i = 0; i < csOptions->count; ++i) {
        avifCodecSpecificOption * entryBefore = &before->csOptions.entries[i];
        avifCodecSpecificOption * entryAfter = &after->csOptions.entries[i];
        if (strcmp(entryBefore->key, entryAfter->key) || strcmp(entryBefore->value, entryAfter->value)) {
            // This could be a false positive if options got reordered but it is unlikely and it is not a big issue.
            return AVIF_TRUE;
        }
    }
    return AVIF_FALSE;
}

avifCodecSpecificOptionsSet() has a similar behavior.

• This avoids changing libavif's current behavior.

It's probably fine. I would be surprised if a single user changed avifEncoder fields after avifEncoderAddImage() and expected them not to be taken into account.

[tongyuantongyu]

I now agree with you.

For codec specific options, I realized user can only change it using avifEncoderSetCodecSpecificOption(). We can then simply record if this function was called by user, and the complicated comparison can be avoid then.

I was thinking about some users accidentally changed encoder settings, and found the program start to fail (when they are using rav1e or svt). But you are right this is certainly a bug and user should correct their usage.

From comment of ResizePendingParams in libaom, AOME_SET_SCALEMODE is reset after encoding each frame. I'm not sure if there are other options work in this way, so I kept AVIF_ADD_IMAGE_FLAG_UPDATE_SETTINGS as a way to explicitly request reconfiguring.

/*!
 * \brief Desired dimensions for an externally triggered resize.
 *
 * When resize is triggered externally, the desired dimensions are stored in
 * this struct until used in the next frame to be coded. These values are
 * effective only for one frame and are reset after they are used.
 */
typedef struct {
  int width;  /*!< Desired resized width */
  int height; /*!< Desired resized height */
} ResizePendingParams;

[y-guyon]

From comment of ResizePendingParams in libaom, AOME_SET_SCALEMODE is reset after encoding each frame. I'm not sure if there are other options work in this way, so I kept AVIF_ADD_IMAGE_FLAG_UPDATE_SETTINGS as a way to explicitly request reconfiguring.

Good catch. I still think libavif should have a consistent behavior, so why not remove AVIF_ADD_IMAGE_FLAG_UPDATE_SETTINGS for simplicity and apply all codec flags at every avifEncoderAddImage() (or at least the ones we know will be reset by the codec)? The user can then remove them before doing another avifEncoderAddImage() if needed. The fact that it is not documented anywhere in avif.h makes me think it is more of a bug or oversight than a feature.

On the other hand, before this PR there was no way to modify a codec flag and take it into account past the first avifEncoderAddImage() call. I also wonder how bad calling avifProcessAOMOptionsPreInit() after aom_codec_enc_init() can be.

On top of that, the current documentation says that avifEncoderSetCodecSpecificOption() should not be called after avifEncoderAddImage():

libavif/include/avif/avif.h

Lines 1117 to 1123 in bbf459e

	// Codec-specific, optional "advanced" tuning settings, in the form of string key/value pairs. These
	// should be set as early as possible, preferably just after creating avifEncoder but before
	// performing any other actions.
	// key must be non-NULL, but passing a NULL value will delete that key, if it exists.
	// Setting an incorrect or unknown option for the current codec will cause errors of type
	// AVIF_RESULT_INVALID_CODEC_SPECIFIC_OPTION from avifEncoderWrite() or avifEncoderAddImage().
	AVIF_API void avifEncoderSetCodecSpecificOption(avifEncoder * encoder, const char * key, const char * value);

This does not seem like a hard rule, just discouraged.

I can think of these options:

• Make it a hard rule, and return AVIF_RESULT_CANNOT_CHANGE_SETTING if avifEncoderSetCodecSpecificOption() is called after avifEncoderAddImage(). This may be a limiting behavior but not a breaking change. No need for AVIF_ADD_IMAGE_FLAG_UPDATE_SETTINGS.
• Remove the rule, update all/some codec flags at every avifEncoderAddImage(), and make sure everything works as intended. No need for AVIF_ADD_IMAGE_FLAG_UPDATE_SETTINGS. This may require more work, especially testing, likely in another PR.
• Remove the rule, discard all codec flags at every avifEncoderAddImage(), and apply new ones added with avifEncoderSetCodecSpecificOption() at next avifEncoderAddImage(). No need for AVIF_ADD_IMAGE_FLAG_UPDATE_SETTINGS. This is closer to the libaom API spirit in my opinion, but slightly more obscure on the libavif side. It should be easier to implement as the burden of understanding libaom flags is forwarded to the libavif user, but it will require a lengthy and clear explanation as a comment in avif.h.

What do you think? Personally I would lean toward the last option.

[tongyuantongyu]

avifProcessAOMOptionsPreInit()

This function only set up value of cfg.rc_end_usage. Naming it "PreInit" is because aom_codec_enc_init() uses aom_codec_enc_cfg so we need to set up it before the call. aom_codec_enc_config_set() is meant to update options in aom_codec_enc_cfg so this is totally fine and intended. We may rename avifProcessAOMOptionsPreInit() to avifProcessAOMOptionsSetupCfg() and avifProcessAOMOptionsPostInit() to avifProcessAOMOptionsSetupExtraCfg() (or something like these) to better describe what they do.

current documentation says

Thanks for remind this. I believe we should update the document, as we would definitely want to allow some options, cq-level for example, to be changed during encoding.

What do you think?

After reading AOM code again, I found that AOME_SET_SCALEMODE cannot to be set by aom_codec_set_option() (See av1/av1_cx_iface.c#encoder_set_option()), and those options that can be set by aom_codec_set_option() can also be set using command line flags when encoding AV1 video, which clearly won't make sense if they only affects the first frame. So I'd assume that they are persistent and AOME_SET_SCALEMODE is the special case. We probably won't put scale mode into csOptions, so I believe the second choice should be fine, and there should be no need to list any special cases for now.

[y-guyon]

Note: I do not mind adding values to this enum but maybe we should be careful not to have too many. There is avifEncoder::diag that was meant for that, so maybe we should just return INVALID_PARAMETER and use a descriptive diag string for rare-ish errors in future PRs.

[tongyuantongyu]

Understand. If both are newly added, I would make AVIF_RESULT_INVALID_IMAGE_GRID and AVIF_RESULT_INCOMPATIBLE_IMAGE share one value, but AVIF_RESULT_INVALID_IMAGE_GRID is already there and is very specific about grid.

[y-guyon]

No need for else

[wantehchang]

Hi Yuan:

Thank you for the pull request and I am sorry about the late review. I reviewed this pull request while trying to import a new libavif snapshot into Google's internal repository today.

Regarding the rule for when avifEncoderSetCodecSpecificOption() can be called, the third option (quoted below) seems the best to me:

• Remove the rule, discard all codec flags at every avifEncoderAddImage(), and apply new ones added with avifEncoderSetCodecSpecificOption() at next avifEncoderAddImage().

It seems that you did not implement this option. Instead, you implemented the second option (quoted below):

• Remove the rule, update all/some codec flags at every avifEncoderAddImage(), and make sure everything works as intended.

Did I understand it correctly?

[wantehchang]

Yuan:

I think the original comment at line 863 "Another frame in an image sequence" should be updated to "Another frame in an image sequence, or another layer in a layered image" or simply "Another frame in an image sequence or layered image".

I think the following new comment added at lines 866-870 should be removed.

        // HEIF (ISO 23008-12:2017), Section 6.6.2.3.1:
        //   All input images shall have exactly the same width and height; call those tile_width and tile_height.
        // MIAF (ISO 23000-22:2019), Section 7.3.11.4.1:
        //   All input images of a grid image item shall use the same coding format, chroma sampling format, and the
        //   same decoder configuration (see 7.3.6.2).

This new comment talks about a grid image, but we have already done the grid image checks at lines 682-714 above.

I believe we are dealing with an image sequence or layered image here. So we should allow the frames to have different widths and heights (width and height are specified in AV1 frame header), but we should certainly check if they have the same depth, yuvRange, etc (these are specified in AV1 sequence header).