Use Tune IQ for layered image inter-frame encoding

[juliobbv-p]

Additionally, for avifenc's --progressive: scale the first layer by a half for further file size savings.

Note: Tune IQ for inter-frame encoding is expected to be available in libaom v3.14.0.

[wantehchang]

Julio: Overall this looks good. Please apply the following patch first before reading my comments. The patch makes some of the changes I suggested.

diff --git a/apps/avifenc.c b/apps/avifenc.c
index f0951cff..d1444111 100644
--- a/apps/avifenc.c
+++ b/apps/avifenc.c
@@ -994,7 +994,8 @@ static avifBool avifEncodeRestOfLayeredImage(avifEncoder * encoder,
             encoder->quality = targetQuality - (targetQuality - PROGRESSIVE_START_QUALITY) *
                                                    (encoder->extraLayerCount - layerIndex) / encoder->extraLayerCount;
             // Don't perform any scaling for the second layer (numerator: 1, denominator: 1).
-            encoder->scalingMode = scalingModeSettingsEntryOf(1, 1).value;
+            const avifScalingMode scalingMode = {{1, 1}, {1, 1}};
+            encoder->scalingMode = scalingMode;
         } else {
             const avifInputFile * nextFile = avifInputGetFile(input, layerIndex);
             // main() function should set number of layers to number of input,
@@ -1165,7 +1166,8 @@ static avifBool avifEncodeImagesFixedQuality(const avifSettings * settings,
         encoder->quality = 10;
         // Low alpha quality resulted in weird artifact, so we don't do it.
         // For further size savings, scale the first layer by a half (numerator: 1, denominator: 2).
-        encoder->scalingMode = scalingModeSettingsEntryOf(1, 2).value;
+        const avifScalingMode scalingMode = {{1, 2}, {1, 2}};
+        encoder->scalingMode = scalingMode;
     }
 
     if (settings->layers > 1) {
diff --git a/src/codec_aom.c b/src/codec_aom.c
index 9b88bb6e..07941682 100644
--- a/src/codec_aom.c
+++ b/src/codec_aom.c
@@ -664,20 +664,23 @@ static avifResult aomCodecEncodeImage(avifCodec * codec,
         codec->internal->qualityFirstLayer = quality;
     }
 
+    // Determine whether the encoder should be configured to use intra frames only, either by setting aomUsage to
+    // AOM_USAGE_ALL_INTRA, or by manually configuring the encoder so all frames will be key frames (if AOM_USAGE_ALL_INTRA isn't
+    // available).
+
+    // For libaom versions older than 3.14.0, all-intra encoding is beneficial when encoding a two-layer image item and the
+    // quality of the first layer is very low. Switching to all-intra encoding comes with the following benefits:
+    // - The first layer will be smaller than the second layer (which is often not the case with inter encoding)
+    // - Outputs have predictable file sizes: the sum of the first layer (quality <= 10) plus the second layer (quality set by
+    //   the caller)
+    // - Because the first layer is very small, layered encoding overhead is also smaller and more stable (about 5-8% for quality
+    //   40 and 2-4% for quality 60)
+    // Note: libaom 3.14.0 introduces a mechanism to completely control each layer's QP, and extends tune IQ to inter-frame
+    // encoding modes (AOM_USAGE_GOOD_QUALITY and AOM_USAGE_REALTIME), so there's no need to use all-intra encoding for layered.
+
     // aom_codec.h says: aom_codec_version() == (major<<16 | minor<<8 | patch)
     static const int aomVersion_3_14_0 = (3 << 16) | (14 << 8);
     const int aomVersion = aom_codec_version();
-
-    // Determine whether the encoder should be configured to use intra frames only, either by setting aomUsage to AOM_USAGE_ALL_INTRA,
-    // or by manually configuring the encoder so all frames will be key frames (if AOM_USAGE_ALL_INTRA isn't available).
-
-    // For libaom versions older than 3.14.0, all-intra encoding is beneficial when encoding a two-layer image item and the quality of the first layer is
-    // very low. Switching to all-intra encoding comes with the following benefits:
-    // - The first layer will be smaller than the second layer (which is often not the case with inter encoding)
-    // - Outputs have predictable file sizes: the sum of the first layer (quality <= 10) plus the second layer (quality set by the caller)
-    // - Because the first layer is very small, layered encoding overhead is also smaller and more stable (about 5-8% for quality 40 and 2-4% for quality 60)
-    // Note: libaom 3.14.0 introduces a mechanism to completely control each layer's QP, and extends tune IQ to inter-frame encoding modes
-    // (AOM_USAGE_GOOD_QUALITY and AOM_USAGE_REALTIME), so there's no need to use all-intra encoding for layered.
     avifBool useAllIntraForLayered = aomVersion < aomVersion_3_14_0 && encoder->extraLayerCount == 1 &&
                                      codec->internal->qualityFirstLayer <= TWO_LAYER_ALL_INTRA_QUALITY_THRESHOLD;
     // Also use all-intra encoding when encoding still images.

[juliobbv-p]

@wantehchang thanks for reviewing this PR! I pushed new changes with your patch applied, and reacted to your feedback.

I'm planning to add the layer scaling bug guard check in a separate PR.

[juliobbv-p]

I had a meeting with @wantehchang to come up with a plan to best handle the two frame scaling bugs we recently found in libaom (targeting layered encoding scenarios):

1. In src/codec_aom.c: if libaom version is <= 3.13.2 and frame scaling is enabled, disable both comp mask prediction and restoration filtering with their respective codec controls.
2. (if 1 doesn't work) In src/codec_aom.c: if libaom version is <= 3.13.2 and frame scaling is enabled, disable the frame scaling feature. If the quality of the first layer is 10, re-adjust the quality to 2 (to compensate for the increased frame dimensions).

[wantehchang]

Julio: I found that the crash in loop restoration filtering is unrelated to frame scaling. It is, however, related to multithreading. We can avoid this crash by either setting the AV1E_SET_ENABLE_RESTORATION codec control to 0 or setting g_threads in aom_codec_enc_cfg_t to 1.

Setting the AV1E_SET_ENABLE_MASKED_COMP codec control to 0 unfortunately does not avoid the crash in aom_comp_mask_upsampled_pred(). By trial and error I found that setting the AV1E_SET_ENABLE_INTERINTRA_WEDGE codec control to 0 avoids the crash in your ResizeCrashTest.TestCompoundMaskPredictionCrash test, but I cannot show by code analysis that this works in general. So the best way to avoid this crash is to disable the frame scaling feature.

[juliobbv-p]

Thanks Wan-Teh for the investigation. What you said about loop restoration filter makes sense, given that the it was two parts of the code that disagreed on the number of actual LR workers during multithreading.

Now that we don't know for certain we can't avoid the crash setting in aom_comp_mask_upsampled_pred(), I agree that disabling the frame scaling feature in libavif is the way to go.

[tongyuantongyu]

I'd like to mention that without cfg->use_fixed_qp_offsets = 2, the outcome of setting quality is often unpredictable or at least confusing, so maybe we should force all intra usage for libaom < 3.14.

Not sure if the loop restoration filtering crash still happens with all intra usage, but this should avoid the aom_comp_mask_upsampled_pred() crash at least.

[wantehchang]

The loop restoration filtering crash also occurs with all intra usage. I just wrote down more info on the loop restoration filtering crash in the libaom CL that fixed the crash. I found that this crash affects libaom versions as old as v3.2.0, but for some reason we have not received reports of this crash. Since the crash is unrelated to the encoding of layered images, I don't think libavif needs to avoid it by turnning off loop restoration or multithreading.

---

Yuan: I believe we already do what you suggested:

    avifBool useAllIntraForLayered = aomVersion < aomVersion_3_14_0 && encoder->extraLayerCount == 1 &&
                                     codec->internal->qualityFirstLayer <= TWO_LAYER_ALL_INTRA_QUALITY_THRESHOLD;

This already checks libaom < 3.14. Or are you suggesting we broaden the condition to the following?

    avifBool useAllIntraForLayered = aomVersion < aomVersion_3_14_0 && encoder->extraLayerCount > 0;

[wantehchang]

Julio: I can now characterize the condition under which the multithreading loop restoration crash occurs: the number of worker threads for loop restoration needs to decrease. For a fixed frame size, this can happen if we

• decrease the number of tiles, or
• switch from AV1E_SET_ROW_MT=1 to AV1E_SET_ROW_MT=0.

Note: The number of worker threads for loop restoration is calculated by the libaom function compute_num_lr_workers(), which does not consider the scaling factor. This explains why this crash is unrelated to frame scaling.

When we use avifenc --progressive to encode a two-layered image, whether we downscale the first layer by 1/2 or not, the number of worker threads for loop restoration is the same for the two layers because the number of tiles and the row-MT mode won't change.

Therefore we don't need to defend against this crash for avifenc --progressive.

[juliobbv-p]

Hi @wantehchang,

First of all, thanks for the analysis!

When we use avifenc --progressive to encode a two-layered image, whether we downscale the first layer by 1/2 or not, the number of worker threads for loop restoration is the same for the two layers because the number of tiles and the row-MT mode won't change.

BTW, this isn't what I saw in practice. When I configured --progressive to downscale the first layer by a 1/2, I could see the number of reported workers change on my machine (as reported by av1_get_num_mod_workers_for_alloc()) in between layers, which triggered the crash that was fixed by https://aomedia-review.googlesource.com/c/aom/+/209681.

For libaom < 3.14.0, we will use all-intra mode. Since aom_comp_mask_upsampled_pred() is not called in all-intra mode, we are protected from this crash.
For libaom >= 3.14.0, we will use good-quality or real-time mode (depending on the encoder speed). This crash is fixed in libaom >= 3.14.0.
Is my anaysis correct?

If we're using all-intra mode for libaom < 3.14.0, (which protects us from the crash in [aom_comp_mask_upsampled_pred()](https://aomedia-review.git.corp.google.com/c/aom/+/208801), this means that we can disable loop restoration with the corresponding codec control and be fully protected from both crashes. This particular version check could also be tightened up to libaom < 3.13.3, given that it contains the LR multithreading fix.

[juliobbv-p]

BTW, here's an image that crashes avifenc if you encode with a version of libaom that doesn't have the LR fix:

./avifenc -s 0 --progressive -c aom -q 10 -y 444 "original/US_Navy_091029-M-5645B-091_A_landing_craft,_air_cushion_(LCAC)_and_the_U.S._Coast_Guard_patrol_boat_Albacore_are_underway_near_Naval_Station_Norfolk.png" -o usnavy.avif"

[wantehchang]

Julio: Thank you very much for the steps to reproduce the bug. I can reproduce the crash now. I was mistaken. The libaom function compute_num_lr_workers() does consider the scaling factor when the avifenc --progressive command executes. I suspect that libaom ignores the scaling factors in libaom's ResizeCrashTest.TestRestorationFilterCrash test because that test does not use spatial layers. (I haven't verified this.) Another libaom bug I found is that compute_num_lr_workers() is called before the current frame size is set, so the number of worker threads for loop restoration is calculated based on the previous frame size. This is why the number of worker threads decreases when the avifenc --progressive command executes. Without this bug, the number of worker threads should increase because the frame size increases.

To avoid this crash, we can disable either loop restoration or multithreading. It is also possible to avoid this crash by setting cfg.g_threads to a small enough number that depends on the frame size, but that requires duplicating the logic of the libaom function compute_num_lr_workers() and is not worth the effort.

[wantehchang]

Julio: Please see pull request #3170.