Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 8 additions & 1 deletion apps/avifenc.c
Original file line number Diff line number Diff line change
Expand Up @@ -993,6 +993,10 @@ static avifBool avifEncodeRestOfLayeredImage(avifEncoder * encoder,
// reversed lerp, so that last layer reaches exact targetQuality
encoder->quality = targetQuality - (targetQuality - PROGRESSIVE_START_QUALITY) *
(encoder->extraLayerCount - layerIndex) / encoder->extraLayerCount;
// We scaled the first layer by a half (numerator: 1, denominator: 2).
// Don't perform any scaling for the second layer (numerator: 1, denominator: 1).
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,
Expand Down Expand Up @@ -1160,8 +1164,11 @@ static avifBool avifEncodeImagesFixedQuality(const avifSettings * settings,
// we should not reach here.
assert(encoder->quality >= PROGRESSIVE_WORST_QUALITY);
// Encode the base layer with a very low quality to ensure a small encoded size.
encoder->quality = 2;
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).
const avifScalingMode scalingMode = { { 1, 2 }, { 1, 2 } };
encoder->scalingMode = scalingMode;
}

if (settings->layers > 1) {
Expand Down
46 changes: 33 additions & 13 deletions src/codec_aom.c
Original file line number Diff line number Diff line change
Expand Up @@ -666,16 +666,25 @@ 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).
// 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).

// 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:
// 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)
// - Option of choosing tune IQ (which requires AOM_USAGE_ALL_INTRA)
avifBool useAllIntraForLayered = encoder->extraLayerCount == 1 &&
// - 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)
// TODO(wtc): Update constant with the libaom version that contains the layered-encoding improvements
static const int aomVersion_3_14_0 = INT_MAX;
const int aomVersion = aom_codec_version();
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.
avifBool useAllIntra = (addImageFlags & AVIF_ADD_IMAGE_FLAG_SINGLE) || useAllIntraForLayered;
Expand Down Expand Up @@ -713,9 +722,7 @@ static avifResult aomCodecEncodeImage(avifCodec * codec,
}
}

// aom_codec.h says: aom_codec_version() == (major<<16 | minor<<8 | patch)
static const int aomVersion_2_0_0 = (2 << 16);
const int aomVersion = aom_codec_version();
if (aomVersion <= aomVersion_2_0_0) {
// Issue with v1.0.0-errata1-avif: https://github.com/AOMediaCodec/libavif/issues/56
// Issue with v2.0.0: https://aomedia-review.googlesource.com/q/I26a39791f820b4d4e1d63ff7141f594c3c7181f5
Expand Down Expand Up @@ -752,13 +759,18 @@ static avifResult aomCodecEncodeImage(avifCodec * codec,
// AOM_TUNE_IQ has been tuned for the YCbCr family of color spaces, and is favored for
// its low perceptual distortion. AOM_TUNE_IQ partially generalizes to, and benefits
// from other "YUV-like" spaces (e.g. YCgCo and ICtCp) including monochrome (luma only).
// AOM_TUNE_IQ sets --deltaq-mode=6 which can only be used in all intra mode.
//
// AOM_TUNE_IQ was introduced in libaom v3.12.0 but it has significantly different bit
// allocation characteristics compared to v3.13.0. AOM_TUNE_IQ is used by default
// starting with v3.13.0 for fewer behavior changes in libavif.
//
// Starting with libaom v3.14.0, AOM_TUNE_IQ supports all-intra, good-quality and
// realtime modes (for single and layered images). Prior to v3.14.0, AOM_TUNE_IQ is only
// supported in all-intra mode.
static const int aomVersion_3_13_0 = (3 << 16) | (13 << 8);
if (image->matrixCoefficients != AVIF_MATRIX_COEFFICIENTS_IDENTITY && aomUsage == AOM_USAGE_ALL_INTRA &&
aomVersion >= aomVersion_3_13_0) {
if (image->matrixCoefficients != AVIF_MATRIX_COEFFICIENTS_IDENTITY &&
((aomUsage == AOM_USAGE_ALL_INTRA && aomVersion >= aomVersion_3_13_0) ||
(encoder->extraLayerCount > 0 && aomVersion >= aomVersion_3_14_0))) {
libavifDefaultTuneMetric = AOM_TUNE_IQ;
}
#endif
Expand Down Expand Up @@ -906,6 +918,14 @@ static avifResult aomCodecEncodeImage(avifCodec * codec,
// For layered image, disable lagged encoding to always get output
// frame for each input frame.
cfg->g_lag_in_frames = 0;

if (aomVersion >= aomVersion_3_14_0) {
// Disable QP offsets, so CQ level = frame QP for every frame.
// This feature requires libaom 3.14.0 or later.
if (cfg->rc_end_usage == AOM_Q) {
cfg->use_fixed_qp_offsets = 2;
}
}
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Collaborator

@wantehchang wantehchang Apr 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor Author

@juliobbv-p juliobbv-p Apr 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

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

Copy link
Copy Markdown
Collaborator

@wantehchang wantehchang Apr 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Julio: Please see pull request #3170.

}
if (disableLaggedOutput) {
cfg->g_lag_in_frames = 0;
Expand Down
Loading