Skip to content

Clean up encoder settings update during encoding #1070

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Aug 29, 2022
Merged

Clean up encoder settings update during encoding #1070

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
404ab2f
Clean up encoder settings update during encoding
wantehchang Aug 20, 2022
5fc9c72
Run clang-format
wantehchang Aug 27, 2022
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
8 changes: 4 additions & 4 deletions include/avif/avif.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -151,7 +151,7 @@ typedef enum avifResult
AVIF_RESULT_NOT_IMPLEMENTED, // a requested code path is not (yet) implemented
AVIF_RESULT_OUT_OF_MEMORY,
AVIF_RESULT_CANNOT_CHANGE_SETTING, // a setting that can't change is changed during encoding
AVIF_RESULT_INCOMPATIBLE_IMAGE // given image is not compatible with already encoded image
AVIF_RESULT_INCOMPATIBLE_IMAGE // the image is incompatible with already encoded images
} avifResult;

AVIF_API const char * avifResultToString(avifResult result);
Expand Down Expand Up @@ -1054,11 +1054,11 @@ typedef struct avifEncoder
avifCodecChoice codecChoice;

// settings (see Notes above)
int keyframeInterval; // How many frames between automatic forced keyframes; 0 to disable (default).
uint64_t timescale; // timescale of the media (Hz)
int maxThreads;
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)
Copy link
Collaborator Author

@wantehchang wantehchang Aug 27, 2022

Choose a reason for hiding this comment

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

Commonly used members are listed before rarely used members.

// changeable encoder settings
int minQuantizer;
int maxQuantizer;
int minQuantizerAlpha;
Expand Down
16 changes: 8 additions & 8 deletions include/avif/internal.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -262,14 +262,14 @@ struct avifCodecInternal;

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)
Copy link
Collaborator Author

@wantehchang wantehchang Aug 27, 2022

Choose a reason for hiding this comment

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

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

AVIF_ENCODER_CHANGE_MIN_QUANTIZER = (1u << 0),
AVIF_ENCODER_CHANGE_MAX_QUANTIZER = (1u << 1),
AVIF_ENCODER_CHANGE_MIN_QUANTIZER_ALPHA = (1u << 2),
AVIF_ENCODER_CHANGE_MAX_QUANTIZER_ALPHA = (1u << 3),
AVIF_ENCODER_CHANGE_TILE_ROWS_LOG2 = (1u << 4),
AVIF_ENCODER_CHANGE_TILE_COLS_LOG2 = (1u << 5),

AVIF_ENCODER_CHANGE_CODEC_SPECIFIC = (1u << 31)
} avifEncoderChange;
typedef uint32_t avifEncoderChanges;

Expand Down
4 changes: 2 additions & 2 deletions src/avif.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,8 +98,8 @@ const char * avifResultToString(avifResult result)
case AVIF_RESULT_INVALID_ARGUMENT: return "Invalid argument";
case AVIF_RESULT_NOT_IMPLEMENTED: return "Not implemented";
case AVIF_RESULT_OUT_OF_MEMORY: return "Out of memory";
case AVIF_RESULT_CANNOT_CHANGE_SETTING: return "Can not change some settings during encoding";
case AVIF_RESULT_INCOMPATIBLE_IMAGE: return "This image is incompatible with already encoded image";
case AVIF_RESULT_CANNOT_CHANGE_SETTING: return "Cannot change some setting during encoding";
case AVIF_RESULT_INCOMPATIBLE_IMAGE: return "The image is incompatible with already encoded images";
case AVIF_RESULT_UNKNOWN_ERROR:
default:
break;
Expand Down
127 changes: 61 additions & 66 deletions src/codec_aom.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -532,10 +532,7 @@ static avifResult aomCodecEncodeImage(avifCodec * codec,
avifCodecEncodeOutput * output)
{
struct aom_codec_enc_cfg * cfg = &codec->internal->cfg;
aom_codec_iface_t * encoderInterface = NULL;
unsigned int aomUsage = AOM_USAGE_GOOD_QUALITY;
int aomCpuUsed = -1;
avifBool lossless = AVIF_FALSE;
avifBool quantizerUpdated = AVIF_FALSE;
Copy link
Collaborator Author

@wantehchang wantehchang Aug 27, 2022

Choose a reason for hiding this comment

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

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


if (!codec->internal->encoderInitialized) {
// Map encoder speed to AOM usage + CpuUsed:
Expand All @@ -550,13 +547,15 @@ static avifResult aomCodecEncodeImage(avifCodec * codec,
// Speed 8: RealTime CpuUsed 8
// Speed 9: RealTime CpuUsed 9
// Speed 10: RealTime CpuUsed 9
unsigned int aomUsage = AOM_USAGE_GOOD_QUALITY;
// Use the new AOM_USAGE_ALL_INTRA (added in https://crbug.com/aomedia/2959) for still
// image encoding if it is available.
#if defined(AOM_USAGE_ALL_INTRA)
if (addImageFlags & AVIF_ADD_IMAGE_FLAG_SINGLE) {
aomUsage = AOM_USAGE_ALL_INTRA;
}
#endif
int aomCpuUsed = -1;
if (encoder->speed != AVIF_SPEED_DEFAULT) {
aomCpuUsed = AVIF_CLAMP(encoder->speed, 0, 9);
if (aomCpuUsed >= 7) {
Expand Down Expand Up @@ -600,7 +599,7 @@ static avifResult aomCodecEncodeImage(avifCodec * codec,

avifGetPixelFormatInfo(image->yuvFormat, &codec->internal->formatInfo);

encoderInterface = aom_codec_av1_cx();
aom_codec_iface_t * encoderInterface = aom_codec_av1_cx();
aom_codec_err_t err = aom_codec_enc_config_default(encoderInterface, cfg, aomUsage);
if (err != AOM_CODEC_OK) {
avifDiagnosticsPrintf(codec->diag, "aom_codec_enc_config_default() failed: %s", aom_codec_err_to_string(err));
Expand Down Expand Up @@ -665,7 +664,8 @@ static avifResult aomCodecEncodeImage(avifCodec * codec,
cfg->g_profile = seqProfile;
cfg->g_bit_depth = image->depth;
cfg->g_input_bit_depth = image->depth;

cfg->g_w = image->width;
cfg->g_h = image->height;
if (addImageFlags & AVIF_ADD_IMAGE_FLAG_SINGLE) {
// Set the maximum number of frames to encode to 1. This instructs
// libaom to set still_picture and reduced_still_picture_header to
Expand All @@ -688,6 +688,15 @@ static avifResult aomCodecEncodeImage(avifCodec * codec,
cfg->g_threads = encoder->maxThreads;
}

if (alpha) {
cfg->rc_min_quantizer = AVIF_CLAMP(encoder->minQuantizerAlpha, 0, 63);
cfg->rc_max_quantizer = AVIF_CLAMP(encoder->maxQuantizerAlpha, 0, 63);
} else {
cfg->rc_min_quantizer = AVIF_CLAMP(encoder->minQuantizer, 0, 63);
cfg->rc_max_quantizer = AVIF_CLAMP(encoder->maxQuantizer, 0, 63);
}
quantizerUpdated = AVIF_TRUE;

codec->internal->monochromeEnabled = AVIF_FALSE;
if (aomVersion > aomVersion_2_0_0) {
// There exists a bug in libaom's chroma_check() function where it will attempt to
Expand All @@ -703,103 +712,84 @@ static avifResult aomCodecEncodeImage(avifCodec * codec,
cfg->monochrome = 1;
}
}
}

avifBool dimensionsChanged = AVIF_FALSE;
if (!codec->internal->encoderInitialized) {
cfg->g_w = image->width;
cfg->g_h = image->height;
} else if ((cfg->g_w != image->width) || (cfg->g_h != image->height)) {
// We are not ready for dimension change for now.
return AVIF_RESULT_NOT_IMPLEMENTED;
}

if (!codec->internal->encoderInitialized || encoderChanges) {
int minQuantizer = AVIF_CLAMP(encoder->minQuantizer, 0, 63);
int maxQuantizer = AVIF_CLAMP(encoder->maxQuantizer, 0, 63);
if (alpha) {
minQuantizer = AVIF_CLAMP(encoder->minQuantizerAlpha, 0, 63);
maxQuantizer = AVIF_CLAMP(encoder->maxQuantizerAlpha, 0, 63);
if (!avifProcessAOMOptionsPreInit(codec, alpha, cfg)) {
return AVIF_RESULT_INVALID_CODEC_SPECIFIC_OPTION;
}
lossless = ((minQuantizer == AVIF_QUANTIZER_LOSSLESS) && (maxQuantizer == AVIF_QUANTIZER_LOSSLESS));
cfg->rc_min_quantizer = minQuantizer;
cfg->rc_max_quantizer = maxQuantizer;
}

if (!codec->internal->encoderInitialized) {
aom_codec_flags_t encoderFlags = 0;
if (image->depth > 8) {
encoderFlags |= AOM_CODEC_USE_HIGHBITDEPTH;
}

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

if (aom_codec_enc_init(&codec->internal->encoder, encoderInterface, cfg, encoderFlags) != AOM_CODEC_OK) {
avifDiagnosticsPrintf(codec->diag,
"aom_codec_enc_init() failed: %s: %s",
aom_codec_error(&codec->internal->encoder),
aom_codec_error_detail(&codec->internal->encoder));
return AVIF_RESULT_UNKNOWN_ERROR;
}
codec->internal->encoderInitialized = AVIF_TRUE;
Copy link
Collaborator Author

@wantehchang wantehchang Aug 27, 2022

Choose a reason for hiding this comment

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

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.


avifBool lossless = ((cfg->rc_min_quantizer == AVIF_QUANTIZER_LOSSLESS) && (cfg->rc_max_quantizer == AVIF_QUANTIZER_LOSSLESS));
if (lossless) {
aom_codec_control(&codec->internal->encoder, AV1E_SET_LOSSLESS, 1);
}
if (encoder->maxThreads > 1) {
aom_codec_control(&codec->internal->encoder, AV1E_SET_ROW_MT, 1);
}
if (encoder->tileRowsLog2 != 0) {
int tileRowsLog2 = AVIF_CLAMP(encoder->tileRowsLog2, 0, 6);
aom_codec_control(&codec->internal->encoder, AV1E_SET_TILE_ROWS, tileRowsLog2);
}
if (encoder->tileColsLog2 != 0) {
int tileColsLog2 = AVIF_CLAMP(encoder->tileColsLog2, 0, 6);
aom_codec_control(&codec->internal->encoder, AV1E_SET_TILE_COLUMNS, tileColsLog2);
}
if (aomCpuUsed != -1) {
if (aom_codec_control(&codec->internal->encoder, AOME_SET_CPUUSED, aomCpuUsed) != AOM_CODEC_OK) {
return AVIF_RESULT_UNKNOWN_ERROR;
}
}
} else if ((encoderChanges & ~AVIF_ENCODER_CHANGE_CODEC_SPECIFIC) || dimensionsChanged) {
// Codec specific options does not change cfg, so no need to update it.
aom_codec_err_t err = aom_codec_enc_config_set(&codec->internal->encoder, cfg);
if (err != AOM_CODEC_OK) {
avifDiagnosticsPrintf(codec->diag,
"aom_codec_enc_config_set() failed: %s: %s",
aom_codec_error(&codec->internal->encoder),
aom_codec_error_detail(&codec->internal->encoder));
return AVIF_RESULT_UNKNOWN_ERROR;
}
}

if (!codec->internal->encoderInitialized || (encoderChanges & AVIF_ENCODER_CHANGE_CODEC_SPECIFIC)) {
if (!avifProcessAOMOptionsPostInit(codec, alpha)) {
return AVIF_RESULT_INVALID_CODEC_SPECIFIC_OPTION;
}
}

avifBool quantizerUpdated = AVIF_FALSE;
if (!codec->internal->encoderInitialized) {
quantizerUpdated = AVIF_TRUE;
if (lossless) {
aom_codec_control(&codec->internal->encoder, AV1E_SET_LOSSLESS, lossless);
}
int tileRowsLog2 = AVIF_CLAMP(encoder->tileRowsLog2, 0, 6);
if (tileRowsLog2 > 0) {
aom_codec_control(&codec->internal->encoder, AV1E_SET_TILE_ROWS, tileRowsLog2);
}
int tileColsLog2 = AVIF_CLAMP(encoder->tileColsLog2, 0, 6);
if (tileColsLog2 > 0) {
aom_codec_control(&codec->internal->encoder, AV1E_SET_TILE_COLUMNS, tileColsLog2);
}
if (!codec->internal->tuningSet) {
if (aom_codec_control(&codec->internal->encoder, AOME_SET_TUNING, AOM_TUNE_SSIM) != AOM_CODEC_OK) {
return AVIF_RESULT_UNKNOWN_ERROR;
}
}
codec->internal->encoderInitialized = AVIF_TRUE;
} else if (encoderChanges) {
} else {
Copy link
Collaborator Author

@wantehchang wantehchang Aug 27, 2022

Choose a reason for hiding this comment

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

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

Please review this else block carefully.

avifBool dimensionsChanged = AVIF_FALSE;
if ((cfg->g_w != image->width) || (cfg->g_h != image->height)) {
// We are not ready for dimension change for now.
return AVIF_RESULT_NOT_IMPLEMENTED;
}
if (alpha) {
if (encoderChanges & (AVIF_ENCODER_CHANGE_MIN_QUANTIZER_ALPHA | AVIF_ENCODER_CHANGE_MAX_QUANTIZER_ALPHA)) {
cfg->rc_min_quantizer = AVIF_CLAMP(encoder->minQuantizerAlpha, 0, 63);
cfg->rc_max_quantizer = AVIF_CLAMP(encoder->maxQuantizerAlpha, 0, 63);
quantizerUpdated = AVIF_TRUE;
aom_codec_control(&codec->internal->encoder, AV1E_SET_LOSSLESS, lossless);
}
} else {
if (encoderChanges & (AVIF_ENCODER_CHANGE_MIN_QUANTIZER | AVIF_ENCODER_CHANGE_MAX_QUANTIZER)) {
cfg->rc_min_quantizer = AVIF_CLAMP(encoder->minQuantizer, 0, 63);
cfg->rc_max_quantizer = AVIF_CLAMP(encoder->maxQuantizer, 0, 63);
quantizerUpdated = AVIF_TRUE;
aom_codec_control(&codec->internal->encoder, AV1E_SET_LOSSLESS, lossless);
}
}
if (quantizerUpdated) {
avifBool lossless =
((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) {
Copy link
Contributor

@tongyuantongyu tongyuantongyu Aug 27, 2022

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

@wantehchang wantehchang Aug 27, 2022

Choose a reason for hiding this comment

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

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.

aom_codec_err_t err = aom_codec_enc_config_set(&codec->internal->encoder, cfg);
if (err != AOM_CODEC_OK) {
avifDiagnosticsPrintf(codec->diag,
"aom_codec_enc_config_set() failed: %s: %s",
aom_codec_error(&codec->internal->encoder),
aom_codec_error_detail(&codec->internal->encoder));
return AVIF_RESULT_UNKNOWN_ERROR;
}
}
if (encoderChanges & AVIF_ENCODER_CHANGE_TILE_ROWS_LOG2) {
Expand All @@ -808,10 +798,15 @@ static avifResult aomCodecEncodeImage(avifCodec * codec,
if (encoderChanges & AVIF_ENCODER_CHANGE_TILE_COLS_LOG2) {
aom_codec_control(&codec->internal->encoder, AV1E_SET_TILE_COLUMNS, AVIF_CLAMP(encoder->tileColsLog2, 0, 6));
}
if (encoderChanges & AVIF_ENCODER_CHANGE_CODEC_SPECIFIC) {
if (!avifProcessAOMOptionsPostInit(codec, alpha)) {
return AVIF_RESULT_INVALID_CODEC_SPECIFIC_OPTION;
}
}
}

#if defined(AOM_USAGE_ALL_INTRA)
if (aomUsage == AOM_USAGE_ALL_INTRA && !codec->internal->endUsageSet && !codec->internal->cqLevelSet && quantizerUpdated) {
if (quantizerUpdated && cfg->g_usage == AOM_USAGE_ALL_INTRA && !codec->internal->endUsageSet && !codec->internal->cqLevelSet) {
// The default rc_end_usage in all intra mode is AOM_Q, which requires cq-level to
// function. A libavif user may not know this internal detail and therefore may only
// set the min and max quantizers in the avifEncoder struct. If this is the case, set
Expand Down
8 changes: 4 additions & 4 deletions src/codec_rav1e.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,16 +53,16 @@ static avifResult rav1eCodecEncodeImage(avifCodec * codec,
avifEncoder * encoder,
const avifImage * image,
avifBool alpha,
avifEncoderChanges updatedConfig,
avifEncoderChanges encoderChanges,
uint32_t addImageFlags,
avifCodecEncodeOutput * output)
{
// rav1e does not support changing config.
if (updatedConfig) {
// rav1e does not support changing encoder settings.
if (encoderChanges) {
return AVIF_RESULT_NOT_IMPLEMENTED;
}

// rav1e does not support changing encoding dimension.
// rav1e does not support changing image dimensions.
if (!codec->internal->rav1eContext) {
codec->internal->encodeWidth = image->width;
codec->internal->encodeHeight = image->height;
Expand Down
8 changes: 4 additions & 4 deletions src/codec_svt.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,16 +46,16 @@ static avifResult svtCodecEncodeImage(avifCodec * codec,
avifEncoder * encoder,
const avifImage * image,
avifBool alpha,
avifEncoderChanges updatedConfig,
avifEncoderChanges encoderChanges,
uint32_t addImageFlags,
avifCodecEncodeOutput * output)
{
// svt does not support changing config.
if (updatedConfig) {
// SVT-AV1 does not support changing encoder settings.
if (encoderChanges) {
return AVIF_RESULT_NOT_IMPLEMENTED;
}

// svt does not support changing encoding dimension.
// SVT-AV1 does not support changing image dimensions.
if (codec->internal->svt_encoder != NULL) {
if ((codec->internal->svt_config.source_width != image->width) || (codec->internal->svt_config.source_height != image->height)) {
return AVIF_RESULT_NOT_IMPLEMENTED;
Expand Down
Loading