Correctly update encoder settings, clear csOptions after avifEncoderAddImage() #1058

tongyuantongyu · 2022-08-21T06:37:30Z

This is a follow up of #1033.

The init logic was skipping some calls if the option is the default value. Now it no longer skip calls so that encoder settings can be reverted to default.

Switch to discard csOptions after being applied to encoder.

…ddImage()

tongyuantongyu · 2022-08-21T08:19:01Z

@y-guyon , @wantehchang : Please review, thanks.

@wantehchang :

the third option (quoted below) seems the best to me

After rethink I agree this is better.

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".

Maybe "the first image" and "subsequent images" is enough?

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

Done. Also removed one of the two conditions that effectively checking the same thing.

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

Done. I wonder if libavif decoder handles frames with different widths and heights correctly.

y-guyon

It seems tests/gtest/avifchangesettingtest.cc passes as is, even with the csOptions behavior change in this PR. Ideally the coverage should be increased to catch future any csOptions behavior change. It can be done in another PR.

src/codec_aom.c

include/avif/avif.h

y-guyon · 2022-08-22T08:35:17Z

src/codec_aom.c

                return AVIF_RESULT_UNKNOWN_ERROR;
            }
+        } else {
+            cfg->g_usage = aomUsage;


I do not see where does this come from. A comment would be useful.

Done. encoder->speed decides aomUsage and aomCpuUsed, so updating g_usage here. In "init" code path this field is set in aom_codec_enc_config_default().

I see. Your message helped me more than the comment though, so maybe consider using the former as the latter. Something like // aomUsage was taken into account by aom_codec_enc_config_default() but it can only be manually updated afterwards.

Changing cfg->g_usage like this is risky, because some other fields in cfg may be tightly coupled to cfg->g_usage and need to be changed accordinly. It is best to avoid this potential issue, by not allowing encoder->speed to change.

Similarly, it is best to not allow encoder->maxThreads to change.

I was imagine some time-constrained encoding application that starts with better quality and tries to speed up if it's too slow. But if changing speed is risky, then I agree we'd better avoid that.

I looked into this. I found that libaom's test/datarate_test.h changes AOME_SET_CPUUSED dynamically in the middle of a video sequence (search for speed_change_test_ in the file), and I also saw webrtc's libaom_av1_encoder.cc file do this. So I believe libaom supports this. However, libavif may need to change cfg->g_usage at the same time it changes AOME_SET_CPUUSED, so it is best to avoid this until it's really needed.

src/codec_aom.c

tongyuantongyu

Ideally the coverage should be increased to catch future any csOptions behavior change.

csOptions field is internal so we can't directly check it. And given that options that can be set in this way are probably all persistent, it's also hard to tell from the encoded file. Maybe we need a test encoder.

include/avif/avif.h

src/codec_aom.c

tongyuantongyu · 2022-08-22T16:29:49Z

src/codec_aom.c

                return AVIF_RESULT_UNKNOWN_ERROR;
            }
+        } else {
+            cfg->g_usage = aomUsage;


Done. encoder->speed decides aomUsage and aomCpuUsed, so updating g_usage here. In "init" code path this field is set in aom_codec_enc_config_default().

src/codec_aom.c

include/avif/avif.h

src/avif.c

wantehchang · 2022-08-23T00:12:48Z

src/codec_aom.c

+        int tileRowsLog2 = AVIF_CLAMP(encoder->tileRowsLog2, 0, 6);
+        aom_codec_control(&codec->internal->encoder, AV1E_SET_TILE_ROWS, tileRowsLog2);
+        int tileColsLog2 = AVIF_CLAMP(encoder->tileColsLog2, 0, 6);
+        aom_codec_control(&codec->internal->encoder, AV1E_SET_TILE_COLUMNS, tileColsLog2);


We should make these aom_codec_control() calls only when necessary. Is it possible to determine which calls are necessary when updateConfig is true?

The reason is that aom_codec_control() is not as cheap as it looks, if you trace its code. It will cause libaom to validate all of its configuration settings. See https://crbug.com/aomedia/2732.

These options can also be set using aom_codec_set_option().
If we only sync when these value changes, we can compare with previous value to find out which value needs to be updated; If these values should always be reflected in encoder, then we have to update them each time, as we can't know the real current value.

src/codec_aom.c

wantehchang

Hi Yuan,

I suggest some changes. I will need to carefully review the current code in src/codec_aom.c after this pull request is merged.

src/codec_aom.c

wantehchang · 2022-08-24T23:52:16Z

include/avif/avif.h

-// key must be non-NULL, but passing a NULL value will delete that key, if it exists.
+// Codec-specific, optional "advanced" tuning settings, in the form of string key/value pairs,
+// to be sent to codec at the next avifEncoderAddImage() call.
+// See the codec documentation to know if a setting is permanent or applied only to the next frame.


Nit: change "permanent" to "persistent" or "sticky"

include/avif/avif.h

wantehchang · 2022-08-25T00:05:48Z

include/avif/avif.h

-// Codec-specific, optional "advanced" tuning settings, in the form of string key/value pairs.
-// key must be non-NULL, but passing a NULL value will delete that key, if it exists.
+// Codec-specific, optional "advanced" tuning settings, in the form of string key/value pairs,
+// to be sent to codec at the next avifEncoderAddImage() call.


Nit: add "the" before "codec"

Optional: Perhaps we can say "to be consumed by the codec ..." The word "consume" more strongly implies that the key/value pairs will be cleared after they are sent to the codec.

wantehchang · 2022-08-25T00:08:29Z

include/avif/avif.h

+// Codec-specific, optional "advanced" tuning settings, in the form of string key/value pairs,
+// to be sent to codec at the next avifEncoderAddImage() call.
+// See the codec documentation to know if a setting is permanent or applied only to the next frame.
+// key must be non-NULL, but passing a NULL value will delete that pending key, if it exists.


Nit: change "that" to "the"

wantehchang · 2022-08-25T00:21:35Z

src/codec_aom.c

                return AVIF_RESULT_UNKNOWN_ERROR;
            }
+        } else {
+            cfg->g_usage = aomUsage;


Changing cfg->g_usage like this is risky, because some other fields in cfg may be tightly coupled to cfg->g_usage and need to be changed accordinly. It is best to avoid this potential issue, by not allowing encoder->speed to change.

Similarly, it is best to not allow encoder->maxThreads to change.

src/codec_aom.c

tongyuantongyu · 2022-08-25T01:31:43Z

include/avif/avif.h

-// Codec-specific, optional "advanced" tuning settings, in the form of string key/value pairs.
-// key must be non-NULL, but passing a NULL value will delete that key, if it exists.
+// Codec-specific, optional "advanced" tuning settings, in the form of string key/value pairs,
+// to be sent to codec at the next avifEncoderAddImage() call.


tongyuantongyu · 2022-08-25T01:31:58Z

include/avif/avif.h

-// key must be non-NULL, but passing a NULL value will delete that key, if it exists.
+// Codec-specific, optional "advanced" tuning settings, in the form of string key/value pairs,
+// to be sent to codec at the next avifEncoderAddImage() call.
+// See the codec documentation to know if a setting is permanent or applied only to the next frame.


include/avif/avif.h

tongyuantongyu · 2022-08-25T01:40:32Z

include/avif/avif.h

+// Codec-specific, optional "advanced" tuning settings, in the form of string key/value pairs,
+// to be sent to codec at the next avifEncoderAddImage() call.
+// See the codec documentation to know if a setting is permanent or applied only to the next frame.
+// key must be non-NULL, but passing a NULL value will delete that pending key, if it exists.


tongyuantongyu · 2022-08-25T01:43:46Z

src/codec_aom.c

                return AVIF_RESULT_UNKNOWN_ERROR;
            }
+        } else {
+            cfg->g_usage = aomUsage;


I was imagine some time-constrained encoding application that starts with better quality and tries to speed up if it's too slow. But if changing speed is risky, then I agree we'd better avoid that.

src/codec_aom.c

tongyuantongyu · 2022-08-25T15:49:09Z

src/codec_aom.c

+    avifBool dimensionChanged = AVIF_FALSE;
+    if ((cfg->g_w != image->width) || (cfg->g_h != image->height)) {
+        cfg->g_w = image->width;
+        cfg->g_h = image->height;


Seems just changing g_w and g_h here only works when sending smaller frames, due to aom writing first frame's dimension to stream header.

To support larger frames, seems we need user to provide the expect dimension before starting encoding, and set g_forced_max_frame_width and g_forced_max_frame_height. I'm going to implement this in another PR.

That's correct. An AV1 bitstream begins with a sequence header OBU, which advertises the maximum frame width and maximum frame height. Then each frame has a frame header OBU that may optionally specify a frame width and height smaller than the maximum. So if we are encoding multiple frames with increasing frame widths and heights, we will need a way to specify the maximum frame width and maximum frame height before encoding the first frame.

wantehchang

Yuan: Please be careful when making this round of suggested changes. For some reason I find it difficult to review the changes to src/codec_aom.c. So we should refrain from making more big changes and try to stabilize this pull request. Thank you!

wantehchang · 2022-08-25T20:49:18Z

src/codec_aom.c

+    if ((cfg->g_w != image->width) || (cfg->g_h != image->height)) {
+        cfg->g_w = image->width;
+        cfg->g_h = image->height;
+        if (codec->internal->encoderInitialized) {


Please do this check first, immediately after line 688.

src/codec_aom.c

include/avif/avif.h

wantehchang · 2022-08-25T21:27:35Z

include/avif/avif.h

 //   a combination of settings are tweaked to simulate this speed range.
-// * AV1 encoder settings and codec specific options set by avifEncoderSetCodecSpecificOption()
-//   will be applied / updated to AV1 encoder before each call to avifEncoderAddImage().
+// * Some encoder settings can be changed. Changes will take effect from next call to


Nit: Add "after encoding starts" to the end of the first sentence. Change "from next call" to "in the next call".

// * Some encoder settings can be changed after encoding starts. Changes will take effect in the next call to

wantehchang · 2022-08-25T21:29:41Z

include/avif/internal.h

+    AVIF_ENCODER_CONFIG_TILE_COLS_LOG_2 = (1 << 5),
+
+    AVIF_ENCODER_CONFIG_CODEC_SPECIFIC = (1 << 31)
+} avifEncoderConfig;


Strictly speaking, if the avifEncoderConfig enum type is defined like this, it cannot take values that are the bitwise-OR of AVIF_ENCODER_CONFIG_xxx. You can find examples of one way to do this properly in avif.h.

I suggest the following:

typedef enum avifEncoderChange { AVIF_ENCODER_CHANGE_MIN_QUANTIZER = (1 << 0), AVIF_ENCODER_CHANGE_MAX_QUANTIZER = (1 << 1), AVIF_ENCODER_CHANGE_MIN_QUANTIZER_ALPHA = (1 << 2), AVIF_ENCODER_CHANGE_MAX_QUANTIZER_ALPHA = (1 << 3), AVIF_ENCODER_CHANGE_TILE_ROWS_LOG2 = (1 << 4), AVIF_ENCODER_CHANGE_TILE_COLS_LOG2 = (1 << 5), AVIF_ENCODER_CHANGE_CODEC_SPECIFIC = (1 << 31) } avifEncoderChange; typedef uint32_t avifEncoderChanges;

and then change the function parameter avifEncoderConfig updateConfig at line 283 to avifEncoderChanges encoderChanges.

wantehchang · 2022-08-26T00:13:48Z

src/codec_aom.c

+            (alpha && ((updatedConfig & AVIF_ENCODER_CONFIG_MIN_QUANTIZER_ALPHA) ||
+                       (updatedConfig & AVIF_ENCODER_CONFIG_MAX_QUANTIZER_ALPHA)))) {
+            aom_codec_control(&codec->internal->encoder, AV1E_SET_LOSSLESS, lossless);
+        }


This conditional expression is very complicated. I suggest we aim for clarify and rewrite it as follows:

if (!alpha) { if ((updatedConfig & AVIF_ENCODER_CONFIG_MIN_QUANTIZER) || (updatedConfig & AVIF_ENCODER_CONFIG_MAX_QUANTIZER)) { aom_codec_control(&codec->internal->encoder, AV1E_SET_LOSSLESS, lossless); } } else { if ((updatedConfig & AVIF_ENCODER_CONFIG_MIN_QUANTIZER_ALPHA) || (updatedConfig & AVIF_ENCODER_CONFIG_MAX_QUANTIZER_ALPHA)) { aom_codec_control(&codec->internal->encoder, AV1E_SET_LOSSLESS, lossless); } }

Optional: We can simplify the conditionals even further:

if (!alpha) { if (updatedConfig & (AVIF_ENCODER_CONFIG_MIN_QUANTIZER | AVIF_ENCODER_CONFIG_MAX_QUANTIZER)) { aom_codec_control(&codec->internal->encoder, AV1E_SET_LOSSLESS, lossless); } } else { if (updatedConfig & (AVIF_ENCODER_CONFIG_MIN_QUANTIZER_ALPHA | AVIF_ENCODER_CONFIG_MAX_QUANTIZER_ALPHA)) { aom_codec_control(&codec->internal->encoder, AV1E_SET_LOSSLESS, lossless); } }

wantehchang · 2022-08-26T00:18:17Z

src/write.c

 // It reports if the change is valid, i.e. if any setting that can't change was changed.
-// It also sets needUpdate to true if valid changes are detected.
-static avifBool avifEncoderSettingsChanged(const avifEncoder * encoder, avifBool * needUpdate)
+// It also reports detected changes in updatedConfig.


This function should sets *updatedConfig to 0 on entry. Otherwise, we need to document that it expects the caller to initialize *updatedConfig to 0.

wantehchang · 2022-08-26T00:35:22Z

src/codec_aom.c

        }
+    }
+
+    avifBool dimensionChanged = AVIF_FALSE;


Nit: dimensionChanged => dimensionsChanged

"Dimensions" should be plural.

wantehchang · 2022-08-26T00:41:12Z

src/codec_aom.c

        if (!avifProcessAOMOptionsPreInit(codec, alpha, cfg)) {
            return AVIF_RESULT_INVALID_CODEC_SPECIFIC_OPTION;
        }
+    }


Since we do not allow "end-usage" to change, we only need to call avifProcessAOMOptionsPreInit() before the aom_codec_enc_init() call. Please move lines 708-710 right before line 736, inside the if (!codec->internal->encoderInitialized) block.

wantehchang · 2022-08-26T00:53:33Z

src/codec_aom.c

            aom_codec_control(&codec->internal->encoder, AOME_SET_CQ_LEVEL, cqLevel);
        }
 #endif
+    }


Lines 774-785 should only be done when initializing the encoder or when cfg->rc_min_quantizer or cfg->rc_max_quantizer changed. If it takes more than five lines to implement this, just add a TODO comment.

wantehchang

LGTM.

Clean up the changes made in the following two pull requests to support updating encoder settings during encoding: AOMediaCodec#1033 AOMediaCodec#1058 In particular, restore the aomCodecEncodeImage() function in src/codec_aom.c to its original structure, plus a new block of code to handle encoder changes. Rename some functions and data members. Edit some comments and messages. In the avifEncoderChange enum, left-shift the unsigned int constant 1u because if we left-shift the signed int constant 1 by 31 bits, it will be shifted into the sign bit. Other miscellaneous cosmetic changes. AOMediaCodec#761

Clean up the changes made in the following two pull requests to support updating encoder settings during encoding: #1033 #1058 In particular, restore the aomCodecEncodeImage() function in src/codec_aom.c to its original structure, plus a new block of code to handle encoder changes. Rename some functions and data members. Edit some comments and messages. In the avifEncoderChange enum, left-shift the unsigned int constant 1u because if we left-shift the signed int constant 1 by 31 bits, it will be shifted into the sign bit. Other miscellaneous cosmetic changes. #761

tongyuantongyu force-pushed the enc-update-settings-2 branch 2 times, most recently from f90ec00 to 331c5aa Compare August 21, 2022 06:46

Correctly update encoder settings, clear csOptions after avifEncoderA…

9a6758c

…ddImage()

tongyuantongyu force-pushed the enc-update-settings-2 branch from 331c5aa to 9a6758c Compare August 21, 2022 06:57

y-guyon reviewed Aug 22, 2022

View reviewed changes

review changes

4ed2425

tongyuantongyu commented Aug 22, 2022

View reviewed changes

y-guyon approved these changes Aug 22, 2022

View reviewed changes

wantehchang reviewed Aug 23, 2022

View reviewed changes

review changes 2

bcc47ea

wantehchang requested changes Aug 25, 2022

View reviewed changes

tongyuantongyu commented Aug 25, 2022

View reviewed changes

only allow quantizer and tileRows/Cols to change

9fec472

tongyuantongyu force-pushed the enc-update-settings-2 branch from 8b18345 to 9fec472 Compare August 25, 2022 15:55

wantehchang requested changes Aug 26, 2022

View reviewed changes

review changes 3

f2db7ed

wantehchang approved these changes Aug 26, 2022

View reviewed changes

wantehchang merged commit 47df37c into AOMediaCodec:main Aug 26, 2022

tongyuantongyu deleted the enc-update-settings-2 branch August 26, 2022 04:18

wantehchang mentioned this pull request Aug 27, 2022

Clean up encoder settings update during encoding #1070

Merged

y-guyon mentioned this pull request Nov 10, 2022

Support for Progressive AVIF encoding #761

Draft

Correctly update encoder settings, clear csOptions after avifEncoderAddImage() #1058

Correctly update encoder settings, clear csOptions after avifEncoderAddImage() #1058

Uh oh!

Conversation

tongyuantongyu commented Aug 21, 2022

Uh oh!

tongyuantongyu commented Aug 21, 2022

Uh oh!

y-guyon left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wantehchang Aug 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

tongyuantongyu left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

wantehchang left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wantehchang left a comment

wantehchang Aug 25, 2022 •

edited

Loading