Clean up encoder settings update during encoding #1070

wantehchang · 2022-08-27T00:37:24Z

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

wantehchang · 2022-08-27T00:53:13Z

@tongyuantongyu Yuan, please review. Thanks.

I reviewed your recent changes carefully and made these adjustments.

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

wantehchang

Yuan: I annotated some changes in this pull request.

wantehchang · 2022-08-27T01:08:05Z

include/avif/avif.h

    int speed;
-    // changeable encoder settings.
+    int keyframeInterval; // How many frames between automatic forced keyframes; 0 to disable (default).
+    uint64_t timescale;   // timescale of the media (Hz)


Commonly used members are listed before rarely used members.

wantehchang · 2022-08-27T01:09:03Z

include/avif/internal.h

-    AVIF_ENCODER_CHANGE_TILE_ROWS_LOG2 = (1 << 4),
-    AVIF_ENCODER_CHANGE_TILE_COLS_LOG2 = (1 << 5),
-
-    AVIF_ENCODER_CHANGE_CODEC_SPECIFIC = (1 << 31)


Google's internal ClangTidy check warns that 1 << 31 shifts the 1 to the sign bit of a signed int.

wantehchang · 2022-08-27T01:10:42Z

src/codec_aom.c

-    unsigned int aomUsage = AOM_USAGE_GOOD_QUALITY;
-    int aomCpuUsed = -1;
-    avifBool lossless = AVIF_FALSE;
+    avifBool quantizerUpdated = AVIF_FALSE;


I restored this function to its original structure. There is only one if (!codec->internal->encoderInitialized) block now (at line 537).

wantehchang · 2022-08-27T01:11:46Z

src/codec_aom.c

        }
-        codec->internal->encoderInitialized = AVIF_TRUE;
-    } else if (encoderChanges) {
+    } else {


This else block handles the "update config" case after the encoder has been initialized.

Please review this else block carefully.

wantehchang · 2022-08-27T01:13:02Z

src/codec_aom.c

                                  aom_codec_error_detail(&codec->internal->encoder));
            return AVIF_RESULT_UNKNOWN_ERROR;
        }
+        codec->internal->encoderInitialized = AVIF_TRUE;


Note that we must set codec->internal->encoderInitialized to true immediately after a successful aom_codec_enc_init() call, because we also check codec->internal->encoderInitialized to determine if aom_codec_destroy(codec->internal->encoder) should be called:

#if defined(AVIF_CODEC_AOM_ENCODE) if (codec->internal->encoderInitialized) { aom_codec_destroy(&codec->internal->encoder); } #endif

Perhaps this should be documented better.

wantehchang · 2022-08-27T01:17:12Z

src/write.c

-static avifBool avifEncoderSettingsChanged(const avifEncoder * encoder, avifEncoderChanges * encoderChanges)
+// This function detects changes made on avifEncoder. It returns true on success (i.e., if every
+// change is valid), or false on failure (i.e., if any setting that can't change was changed). It
+// reports detected changes in encoderChanges.


I edited this comment. Please check if the comment is accurate.

wantehchang · 2022-08-27T01:19:24Z

src/write.c


-    avifEncoderChanges encoderChanges = 0;
-    if (!avifEncoderSettingsChanged(encoder, &encoderChanges)) {
+    avifEncoderChanges encoderChanges;


Note that I removed the initialization of this variable, to test that the avifEncoderDetectChanges() call below sets its output parameter properly.

src/write.c

tests/gtest/aviftest_helpers.h

wantehchang · 2022-08-27T01:28:18Z

tests/gtest/aviftest_helpers.cc

                                          avifROData* out) {
  auto reader = reinterpret_cast<AvifIOLimitedReader*>(io);

+  if (offset > UINT64_MAX - size) {


This check detects overflow of the addition offset + size at line 226.

tongyuantongyu · 2022-08-27T13:52:06Z

src/codec_aom.c

+                ((cfg->rc_min_quantizer == AVIF_QUANTIZER_LOSSLESS) && (cfg->rc_max_quantizer == AVIF_QUANTIZER_LOSSLESS));
+            aom_codec_control(&codec->internal->encoder, AV1E_SET_LOSSLESS, lossless);
+        }
+        if (quantizerUpdated || dimensionsChanged) {


For now using quantizerUpdated here is fine, but if more options are added, we may need a dedicated cfgUpdated here.

You're right. The quantizerUpdated || dimensionsChanged condition here detects whether any fields (currently g_w, g_h, rc_min_quantizer, and rc_max_quantizer) in cfg are updated. We can replace it with a dedicated cfgUpdated boolean flag. Note that we still need quantizerUpdated for the code at lines 808-819 below.

tongyuantongyu · 2022-08-27T14:01:27Z

LGTM. Thanks for the update.

wantehchang

Yuan: Thank you for the review. I am wondering if you can approve this pull request using the GitHub UI.

If you select the "Files changed" tab when viewing this PR, do you see a green "Review changes" or "Finish your review" drop-down menu on the right?

In that menu, you can check the "Approve" radio button and hit the "Submit review" button.

wantehchang · 2022-08-27T16:54:44Z

src/codec_aom.c

+                ((cfg->rc_min_quantizer == AVIF_QUANTIZER_LOSSLESS) && (cfg->rc_max_quantizer == AVIF_QUANTIZER_LOSSLESS));
+            aom_codec_control(&codec->internal->encoder, AV1E_SET_LOSSLESS, lossless);
+        }
+        if (quantizerUpdated || dimensionsChanged) {


You're right. The quantizerUpdated || dimensionsChanged condition here detects whether any fields (currently g_w, g_h, rc_min_quantizer, and rc_max_quantizer) in cfg are updated. We can replace it with a dedicated cfgUpdated boolean flag. Note that we still need quantizerUpdated for the code at lines 808-819 below.

tests/gtest/aviftest_helpers.h

src/write.c

tongyuantongyu

I am wondering if you can approve this pull request using the GitHub UI.

Got it.

wantehchang force-pushed the edit-comments-and-minor-fixes branch from d22bfd2 to 6976e49 Compare August 27, 2022 00:44

wantehchang requested a review from y-guyon August 27, 2022 00:51

wantehchang force-pushed the edit-comments-and-minor-fixes branch from 6976e49 to 404ab2f Compare August 27, 2022 01:06

Run clang-format

5fc9c72

wantehchang commented Aug 27, 2022

View reviewed changes

tongyuantongyu reviewed Aug 27, 2022

View reviewed changes

wantehchang commented Aug 27, 2022

View reviewed changes

tongyuantongyu approved these changes Aug 28, 2022

View reviewed changes

y-guyon approved these changes Aug 29, 2022

View reviewed changes

wantehchang merged commit 8fd2027 into AOMediaCodec:main Aug 29, 2022

wantehchang deleted the edit-comments-and-minor-fixes branch August 29, 2022 15:55

wantehchang mentioned this pull request Aug 29, 2022

Support changing frame size during encoding #1069

Draft

Clean up encoder settings update during encoding #1070

Clean up encoder settings update during encoding #1070

Uh oh!

Conversation

wantehchang commented Aug 27, 2022

Uh oh!

wantehchang commented Aug 27, 2022

Uh oh!

wantehchang left a comment

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!

Choose a reason for hiding this comment

Uh oh!

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!

tongyuantongyu commented Aug 27, 2022

Uh oh!

wantehchang left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

tongyuantongyu left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants