Skip to content
Merged
Show file tree
Hide file tree
Changes from 2 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
13 changes: 4 additions & 9 deletions include/avif/avif.h
Original file line number Diff line number Diff line change
Expand Up @@ -148,6 +148,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
Copy link
Contributor

Choose a reason for hiding this comment

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

Note: I do not mind adding values to this enum but maybe we should be careful not to have too many. There is avifEncoder::diag that was meant for that, so maybe we should just return INVALID_PARAMETER and use a descriptive diag string for rare-ish errors in future PRs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Understand. If both are newly added, I would make AVIF_RESULT_INVALID_IMAGE_GRID and AVIF_RESULT_INCOMPATIBLE_IMAGE share one value, but AVIF_RESULT_INVALID_IMAGE_GRID is already there and is very specific about grid.

} avifResult;

AVIF_API const char * avifResultToString(avifResult result);
Expand Down Expand Up @@ -1038,8 +1039,8 @@ struct avifCodecSpecificOptions;
// image in less bytes. AVIF_SPEED_DEFAULT means "Leave the AV1 codec to its default speed settings"./
// If avifEncoder uses rav1e, the speed value is directly passed through (0-10). If libaom is used,
// a combination of settings are tweaked to simulate this speed range.
// * AV1 encoder settings and codecSpecificOptions will be applied to AV1 encoder when changed or when
// AVIF_ADD_IMAGE_FLAG_UPDATE_SETTINGS flag is explicitly requested.
// * AV1 encoder settings and codec specific options set by avifEncoderSetCodecSpecificOption()
// will be applied / updated to AV1 encoder before each call to avifEncoderAddImage().
typedef struct avifEncoder
{
// Defaults to AVIF_CODEC_CHOICE_AUTO: Preference determined by order in availableCodecs table (avif.c)
Expand Down Expand Up @@ -1084,10 +1085,6 @@ typedef enum avifAddImageFlag
// tweaks various compression rules. This is enabled automatically when using the
// avifEncoderWrite() single-image encode path.
AVIF_ADD_IMAGE_FLAG_SINGLE = (1 << 1),

// Use this flag to update encode settings of AV1 encoder.
// This is enabled automatically if encoder settings is changed.
AVIF_ADD_IMAGE_FLAG_UPDATE_SETTINGS = (1 << 2)
} avifAddImageFlag;
typedef uint32_t avifAddImageFlags;

Expand All @@ -1112,9 +1109,7 @@ AVIF_API avifResult avifEncoderAddImageGrid(avifEncoder * encoder,
avifAddImageFlags addImageFlags);
AVIF_API avifResult avifEncoderFinish(avifEncoder * encoder, avifRWData * output);

// Codec-specific, optional "advanced" tuning settings, in the form of string key/value pairs. These
// should be set as early as possible, preferably just after creating avifEncoder but before
// performing any other actions.
// 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.
// Setting an incorrect or unknown option for the current codec will cause errors of type
// AVIF_RESULT_INVALID_CODEC_SPECIFIC_OPTION from avifEncoderWrite() or avifEncoderAddImage().
Expand Down
1 change: 1 addition & 0 deletions include/avif/internal.h
Original file line number Diff line number Diff line change
Expand Up @@ -265,6 +265,7 @@ typedef avifResult (*avifCodecEncodeImageFunc)(struct avifCodec * codec,
avifEncoder * encoder,
const avifImage * image,
avifBool alpha,
avifBool updateConfig,
avifAddImageFlags addImageFlags,
avifCodecEncodeOutput * output);
typedef avifBool (*avifCodecEncodeFinishFunc)(struct avifCodec * codec, avifCodecEncodeOutput * output);
Expand Down
3 changes: 2 additions & 1 deletion src/avif.c
Original file line number Diff line number Diff line change
Expand Up @@ -96,7 +96,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_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_UNKNOWN_ERROR:
default:
break;
Expand Down
3 changes: 2 additions & 1 deletion src/codec_aom.c
Original file line number Diff line number Diff line change
Expand Up @@ -526,10 +526,11 @@ static avifResult aomCodecEncodeImage(avifCodec * codec,
avifEncoder * encoder,
const avifImage * image,
avifBool alpha,
avifBool updateConfig,
avifAddImageFlags addImageFlags,
avifCodecEncodeOutput * output)
{
if (!codec->internal->encoderInitialized || (addImageFlags & AVIF_ADD_IMAGE_FLAG_UPDATE_SETTINGS)) {
if (!codec->internal->encoderInitialized || updateConfig) {
// Map encoder speed to AOM usage + CpuUsed:
// Speed 0: GoodQuality CpuUsed 0
// Speed 1: GoodQuality CpuUsed 1
Expand Down
3 changes: 2 additions & 1 deletion src/codec_rav1e.c
Original file line number Diff line number Diff line change
Expand Up @@ -51,10 +51,11 @@ static avifResult rav1eCodecEncodeImage(avifCodec * codec,
avifEncoder * encoder,
const avifImage * image,
avifBool alpha,
avifBool updateConfig,
uint32_t addImageFlags,
avifCodecEncodeOutput * output)
{
if (addImageFlags & AVIF_ADD_IMAGE_FLAG_UPDATE_SETTINGS) {
if (updateConfig) {
return AVIF_RESULT_NOT_IMPLEMENTED;
}

Expand Down
3 changes: 2 additions & 1 deletion src/codec_svt.c
Original file line number Diff line number Diff line change
Expand Up @@ -46,10 +46,11 @@ static avifResult svtCodecEncodeImage(avifCodec * codec,
avifEncoder * encoder,
const avifImage * image,
avifBool alpha,
avifBool updateConfig,
uint32_t addImageFlags,
avifCodecEncodeOutput * output)
{
if (addImageFlags & AVIF_ADD_IMAGE_FLAG_UPDATE_SETTINGS) {
if (updateConfig) {
return AVIF_RESULT_NOT_IMPLEMENTED;
}

Expand Down
71 changes: 47 additions & 24 deletions src/write.c
Original file line number Diff line number Diff line change
Expand Up @@ -322,14 +322,35 @@ void avifEncoderSetCodecSpecificOption(avifEncoder * encoder, const char * key,
encoder->data->csOptionsUpdated = AVIF_TRUE;
}

avifBool avifEncoderCheckSettingsChange(avifEncoder * encoder, avifAddImageFlag * flags)
static void avifBackupSettings(avifEncoder * encoder)
{
avifEncoder * lastEncoder = &encoder->data->lastEncoder;

// lastEncoder->data is used to mark that lastEncoder is initialized.
lastEncoder->data = encoder->data;
lastEncoder->codecChoice = encoder->codecChoice;
lastEncoder->keyframeInterval = encoder->keyframeInterval;
lastEncoder->timescale = encoder->timescale;
lastEncoder->maxThreads = encoder->maxThreads;
lastEncoder->minQuantizer = encoder->minQuantizer;
lastEncoder->maxQuantizer = encoder->maxQuantizer;
lastEncoder->minQuantizerAlpha = encoder->minQuantizerAlpha;
lastEncoder->maxQuantizerAlpha = encoder->maxQuantizerAlpha;
lastEncoder->tileRowsLog2 = encoder->tileRowsLog2;
lastEncoder->tileColsLog2 = encoder->tileColsLog2;
lastEncoder->speed = encoder->speed;
encoder->data->csOptionsUpdated = AVIF_FALSE;
}

// This function detect changes made on avifEncoder.
// 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)
{
const avifEncoder * lastEncoder = &encoder->data->lastEncoder;

if (lastEncoder->data == NULL) {
lastEncoder->data = encoder->data;
goto copy;
return AVIF_TRUE;
}

if ((lastEncoder->codecChoice != encoder->codecChoice) || (lastEncoder->keyframeInterval != encoder->keyframeInterval) ||
Expand All @@ -342,23 +363,9 @@ avifBool avifEncoderCheckSettingsChange(avifEncoder * encoder, avifAddImageFlag
(lastEncoder->maxQuantizerAlpha != encoder->maxQuantizerAlpha) || (lastEncoder->tileRowsLog2 != encoder->tileRowsLog2) ||
(lastEncoder->tileColsLog2 != encoder->tileColsLog2) || (lastEncoder->speed != encoder->speed) ||
(encoder->data->csOptionsUpdated)) {
*flags |= AVIF_ADD_IMAGE_FLAG_UPDATE_SETTINGS;
goto copy;
*needUpdate = AVIF_TRUE;
}

copy:
lastEncoder->codecChoice = encoder->codecChoice;
lastEncoder->keyframeInterval = encoder->keyframeInterval;
lastEncoder->timescale = encoder->timescale;
lastEncoder->maxThreads = encoder->maxThreads;
lastEncoder->minQuantizer = encoder->minQuantizer;
lastEncoder->maxQuantizer = encoder->maxQuantizer;
lastEncoder->minQuantizerAlpha = encoder->minQuantizerAlpha;
lastEncoder->maxQuantizerAlpha = encoder->maxQuantizerAlpha;
lastEncoder->tileRowsLog2 = encoder->tileRowsLog2;
lastEncoder->tileColsLog2 = encoder->tileColsLog2;
lastEncoder->speed = encoder->speed;
encoder->data->csOptionsUpdated = AVIF_FALSE;
return AVIF_TRUE;
}

Expand Down Expand Up @@ -649,8 +656,11 @@ static avifResult avifEncoderAddImageInternal(avifEncoder * encoder,
return AVIF_RESULT_NO_CODEC_AVAILABLE;
}

if (!avifEncoderCheckSettingsChange(encoder, &addImageFlags)) {
avifBool updateConfig = AVIF_FALSE;
if (!avifEncoderSettingsChanged(encoder, &updateConfig)) {
return AVIF_RESULT_CANNOT_CHANGE_SETTING;
} else {
Copy link
Contributor

Choose a reason for hiding this comment

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

No need for else

avifBackupSettings(encoder);
}

// -----------------------------------------------------------------------
Expand Down Expand Up @@ -853,10 +863,23 @@ static avifResult avifEncoderAddImageInternal(avifEncoder * encoder,
} else {
// Another frame in an image sequence

if (encoder->data->alphaPresent && !firstCell->alphaPlane) {
// If the first image in the sequence had an alpha plane (even if fully opaque), all
// subsequence images must have alpha as well.
return AVIF_RESULT_ENCODE_ALPHA_FAILED;
const avifImage * imageMetadata = encoder->data->imageMetadata;
// HEIF (ISO 23008-12:2017), Section 6.6.2.3.1:
// All input images shall have exactly the same width and height; call those tile_width and tile_height.
// MIAF (ISO 23000-22:2019), Section 7.3.11.4.1:
// All input images of a grid image item shall use the same coding format, chroma sampling format, and the
// same decoder configuration (see 7.3.6.2).
// If the first image in the sequence had an alpha plane (even if fully opaque), all
// subsequence images must have alpha as well.
if ((imageMetadata->width != firstCell->width) || (imageMetadata->height != firstCell->height) ||
(imageMetadata->depth != firstCell->depth) || (imageMetadata->yuvFormat != firstCell->yuvFormat) ||
(imageMetadata->yuvRange != firstCell->yuvRange) || (imageMetadata->colorPrimaries != firstCell->colorPrimaries) ||
(imageMetadata->transferCharacteristics != firstCell->transferCharacteristics) ||
(imageMetadata->matrixCoefficients != firstCell->matrixCoefficients) ||
(!!imageMetadata->alphaPlane != !!firstCell->alphaPlane) ||
(imageMetadata->alphaPremultiplied != firstCell->alphaPremultiplied) ||
(encoder->data->alphaPresent && !firstCell->alphaPlane)) {
return AVIF_RESULT_INCOMPATIBLE_IMAGE;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Yuan:

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

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

        // HEIF (ISO 23008-12:2017), Section 6.6.2.3.1:
        //   All input images shall have exactly the same width and height; call those tile_width and tile_height.
        // MIAF (ISO 23000-22:2019), Section 7.3.11.4.1:
        //   All input images of a grid image item shall use the same coding format, chroma sampling format, and the
        //   same decoder configuration (see 7.3.6.2).

This new comment talks about a grid image, but we have already done the grid image checks at lines 682-714 above.

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 (width and height are specified in AV1 frame header), but we should certainly check if they have the same depth, yuvRange, etc (these are specified in AV1 sequence header).

}
}

Expand All @@ -872,7 +895,7 @@ static avifResult avifEncoderAddImageInternal(avifEncoder * encoder,
if (item->codec) {
const avifImage * cellImage = cellImages[item->cellIndex];
avifResult encodeResult =
item->codec->encodeImage(item->codec, encoder, cellImage, item->alpha, addImageFlags, item->encodeOutput);
item->codec->encodeImage(item->codec, encoder, cellImage, item->alpha, updateConfig, addImageFlags, item->encodeOutput);
if (encodeResult == AVIF_RESULT_UNKNOWN_ERROR) {
encodeResult = item->alpha ? AVIF_RESULT_ENCODE_ALPHA_FAILED : AVIF_RESULT_ENCODE_COLOR_FAILED;
}
Expand Down
10 changes: 5 additions & 5 deletions tests/gtest/avifchangesettingtest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -96,7 +96,11 @@ void TestEncodeDecode(avifCodecChoice codec,
}

TEST(ChangeSettingTest, AOM) {
// Test if changes to AV1 encode settings are detected.
TestEncodeDecode(AVIF_CODEC_CHOICE_AOM, {{"end-usage", "cbr"}}, true, false);

// Test if changes to codec specific options are detected.
TestEncodeDecode(AVIF_CODEC_CHOICE_AOM, {}, true, true);
}

TEST(ChangeSettingTest, RAV1E) {
Expand All @@ -107,11 +111,7 @@ TEST(ChangeSettingTest, SVT) {
TestEncodeDecode(AVIF_CODEC_CHOICE_SVT, {}, false, false);
}

TEST(ChangeSettingTest, ChangeCsOptions) {
TestEncodeDecode(AVIF_CODEC_CHOICE_AOM, {}, true, true);
}

TEST(ChangeSettingTest, UnchangableSetting) {
TEST(ChangeSettingTest, UnchangeableSetting) {
if (avifCodecName(AVIF_CODEC_CHOICE_AOM, AVIF_CODEC_FLAG_CAN_ENCODE) ==
nullptr) {
GTEST_SKIP() << "Codec unavailable, skip test.";
Expand Down