Support display size override for layered image - #1069
Conversation
|
Yuan: I just merged my cleanup pull request #1070. Please rebase this pull request and then I will take a look at the crash. Thanks. |
054b138 to
864c34e
Compare
Legacy contentI tried to run asan and here is the report: The function in question seems related to ssim, but if I set |
wantehchang
left a comment
wantehchang
left a comment
There was a problem hiding this comment.
Yuan: I reviewed everything except the avifRWStreamWrite parts of src/write.c and the decoder part of the new test in avifchangesettingtest.cc. I believe your changes to src/codec_aom.c. So this looks like a libaom bug that certain arrays are allocated to the size of the initial values of cfg.g_w and cfg.g_h.
|
Yuan: Please test this libaom patch. It is likely to be incomplete (e.g., the VMAF and BUTTERAUGLI code may also need the same changes). |
|
I discussed this issue with my colleague Marco Paniconi, who works on real-time encoding and SVC in libaom. Marco told me that they don't use Although AVIF allows the images in an image sequence to have different sizes, the libavif encoder does not need to support that. (The libavif decoder supports that.) |
tongyuantongyu
left a comment
tongyuantongyu
left a comment
There was a problem hiding this comment.
Please test this libaom patch
Thaks for the patch. I confirm that after patch libaom encodes correctly.
let libaom downscale the frames internally for certain spatial layers
Actually I'm planning to support both. Use libaom internal scaler is more convenient for basic usage with only one input image, but if user wants more flexibility (e.g. using small blurred "thumbnail" as first layer) input image of different size can be both more efficient and convenient.
Earlier this year I reported two bugs in libaom's internal scaler. I see aomedia:3210 is fixed by a recent commit https://aomedia-review.googlesource.com/c/aom/+/161961. For aomedia:3203 I have an old CL, but I can't figure out how to update it because it's targeting master branch, so I created a new one instead.
4011553 to
d7fe311
Compare
|
With the latest tip of libaom all added test cases passes.
AVIF use case is different here: we want to use different images for each layer (like the low resolution blurry preview example I mentioned) - instead of sending the same frame many times to encode scalable video. So the SVC approach puts user into an awkward situation: they need to upscale the first layer to match frame size, and then libaom downscales it back. This is inefficient, and upscale-downscale roundtrip can be lossy. |
4761eb5 to
5e208db
Compare
Based on the libavif avifchangedimensiontest AOMDecreasing/ChangeDimensionTest.EncodeDecode/0 in AOMediaCodec/libavif#1069 by Yuan Tong. Test: test_libaom --gtest_filter=*DimensionDecreasing \ --gtest_also_run_disabled_tests Bug: aomedia:3348 Change-Id: I8be3c092234f99402c02aa03340c916f629ce0f2
Based on the libavif avifchangedimensiontest AOMDecreasing/ChangeDimensionTest.EncodeDecode/0 in AOMediaCodec/libavif#1069 by Yuan Tong. Test: test_libaom --gtest_filter=*DimensionDecreasing \ --gtest_also_run_disabled_tests Bug: aomedia:3348 Change-Id: I79aea87012cc3bea43d565911ac88816c860e737
Based on the libavif avifchangedimensiontest AOMDecreasing/ChangeDimensionTest.EncodeDecode/0 in AOMediaCodec/libavif#1069 by Yuan Tong. Test: test_libaom --gtest_filter=*DimensionDecreasing \ --gtest_also_run_disabled_tests Bug: aomedia:3348 Change-Id: I79aea87012cc3bea43d565911ac88816c860e737
7958220 to
c2a0f52
Compare
tongyuantongyu
changed the title
c2a0f52 to
4aff8cf
Compare
|
@wantehchang I'm reviving this PR as recent development in AOM seem to have fixed the crash. @juliobbv-p Thank you for your hard work! |
5e3f660 to
1c2939a
Compare
1c2939a to
e8b0592
Compare
|
Sorry for the late reply. I tested against many resolution sets and verified that:
Test code: https://gist.github.com/tongyuantongyu/61f9910ffdaf26a384342cbde4abf718 |
e8b0592 to
7f6cdb8
Compare
tongyuantongyu
changed the title
|
@wantehchang @juliobbv-p Is there anything I'm missing, or we can move forward the review of this PR? |
|
Yuan: I have started reviewing this PR. I created a copy of this PR in https://aomedia-review.googlesource.com/c/libavif/+/216021 so that I can view it using Gerrit. I will post my review comments here. |
7f6cdb8 to
751afed
Compare
751afed to
25eb44b
Compare
| if (avifEncoderUsesRenderedSizeOverride(encoder)) { | ||
| encoder->data->imageMetadata->width = encoder->width; | ||
| encoder->data->imageMetadata->height = encoder->height; | ||
| } |
There was a problem hiding this comment.
IMPORTANT: Doing it this way is convenient, but it will break the code that assumes imageMetadata->width/height is firstCell->width/height, and it is difficult to track down all such code and verify nothing is broken.
For example, imageMetadata is passed to avifRWStreamWriteProperties(), which has the following code:
// 'clap' is treated as 'irot'/'imir', although it could differ between the base and
// gain map image items if these have different dimensions.
if (imageMetadata->transformFlags & AVIF_TRANSFORM_CLAP) {
AVIF_CHECKERR(imageMetadata->width != itemMetadata->width || imageMetadata->height != itemMetadata->height,
AVIF_RESULT_NOT_IMPLEMENTED);
}
It is not clear to me whether that code is still correct if imageMetadata->width/height is equal to encoder->width/height.
So I recommend using encoder->width/height explicitly even though it will require more changes.
There was a problem hiding this comment.
My reasoning is:
It is equivalent (maybe there are some nuances in AV1 bitstream, but I expect no difference in AVIF container level) between
- adding a full-size image and set
encoder->scalingMode, and - adding a scaled image and set
encoder->width/height:
either way the AV1 bitstream encodes, and will be decoded into a scaled frame. During decode the first thing to do is scaling it back to what the AVIF container declares (which I call it "rendered size"):
Lines 6898 to 6908 in cbb391c
So if it's an imageMetadata, or in another word AVIF container level thing, using encoder->width/height should always be the correct answer, otherwise using firstCell->width/height would also be wrong if encoder->scalingMode is used.
And I have checked that all read to imageMetadata is for image metadata, and nobody is using its value to access pixels.
tongyuantongyu
changed the title
9d3f4ff to
02e6146
Compare
|
@wantehchang the fuzztest failure is due to a general issue about how we handle quality and quantize settings: #3344. |
02e6146 to
ae4d145
Compare
|
Yuan: I will review this pull request today. |
| uint64_t creationTime; | ||
| uint64_t modificationTime; | ||
| uint32_t width; | ||
| uint32_t height; |
There was a problem hiding this comment.
Yuan: Please apply the patch below and review my proposed changes. I moved some code around and edited some comments.
In places where my comment edits may not be easy to see, I also added review comments to describe my edits. In those places my review comments will say "(See patch.)"
Here is my proposed patch:
diff --git a/apps/avifenc.c b/apps/avifenc.c
index b45094e9..d8dad2cc 100644
--- a/apps/avifenc.c
+++ b/apps/avifenc.c
@@ -2230,7 +2230,7 @@ int main(int argc, char * argv[])
goto cleanup;
}
if (avifSettingsUsesDisplaySizeOverride(&settings) && (settings.layers == 1) && (input.filesCount > 1)) {
- fprintf(stderr, "ERROR: --display-size is not supported with image sequences. Use --layered for multiple still inputs.\n");
+ fprintf(stderr, "ERROR: --display-size is not supported with image sequences. Use --layered for multiple still image inputs.\n");
goto cleanup;
}
diff --git a/include/avif/avif.h b/include/avif/avif.h
index 231b8550..556359cb 100644
--- a/include/avif/avif.h
+++ b/include/avif/avif.h
@@ -822,10 +822,11 @@ typedef struct avifImage
// transformFlags. On decode, only honor the values in boxes with the associated transform flag set.
// These also apply to gainMap->image, if any.
//
- // When encoding with avifEncoder.width/height set (see its comment), these values are interpreted
- // relative to that overridden display size rather than to this avifImage's own width/height.
- // No special handling is needed during decode: the decoded avifImage is automatically scaled to
- // the display size, so these values are relative to its width/height as usual.
+ // When encoding with avifEncoder.width/height set (see their comment), these transformations
+ // are interpreted relative to that overridden display size rather than to this avifImage's own
+ // width/height. No special handling is needed during decode: the decoded avifImage is
+ // automatically scaled to the display size, so these transformations are relative to the
+ // decoded avifImage's width/height as usual.
avifTransformFlags transformFlags;
avifPixelAspectRatioBox pasp;
avifCleanApertureBox clap;
@@ -1638,7 +1639,7 @@ typedef struct avifEncoder
// Overrides the image's display size (the width and height AVIF declares this image is meant
// to be shown at) independent of the pixel dimensions actually encoded. Any image passed to
// avifEncoderAddImage()/avifEncoderAddImageGrid() that is encoded smaller than the display size
- // is upscaled to it on decode.
+ // is upscaled to the display size on decode.
//
// Defaults to 0 (disabled). When 0, the display size is the size of the first added image.
// When set, both width and height must be nonzero, and neither can be smaller than the coded
@@ -1661,9 +1662,9 @@ typedef struct avifEncoder
// decoder is required to honor; a coded image smaller than it is upscaled by the decoder to
// match. Separately, an AV1 encoder session declares a maximum frame size once, up front, and
// every later frame must fit within it; by default that maximum is simply the size of the first
- // frame encoded. Setting width/height also declares that AV1-level maximum explicitly, in
- // addition to the ispe box, so that later, larger layers stay within it. This is why it must be
- // at least as large as every layer's coded size.
+ // frame encoded. Setting width/height also declares that AV1-level maximum frame size
+ // explicitly, in addition to the ispe box, so that later, larger layers stay within it. This is
+ // why it must be at least as large as every layer's coded size.
uint32_t width;
uint32_t height;
} avifEncoder;
diff --git a/src/codec_aom.c b/src/codec_aom.c
index 9f23c620..8edd3f8a 100644
--- a/src/codec_aom.c
+++ b/src/codec_aom.c
@@ -893,14 +893,20 @@ static avifResult aomCodecEncodeImage(avifCodec * codec,
}
}
- if ((addImageFlags & AVIF_ADD_IMAGE_FLAG_SINGLE) && (encoder->width == 0) && (encoder->height == 0)) {
+ 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
// 1 in AV1 sequence headers.
- // Still picture header requires frame size to match
- // max_frame_width and max_frame_height,
- // so we can't use it if frame can have a different size.
cfg->g_limit = 1;
+ // Reduced still picture header requires frame size to match max
+ // frame size, so we can't use reduced still picture header if frame
+ // size is different from max frame size.
+ if (cfg->g_forced_max_frame_width > image->width) {
+ cfg->g_forced_max_frame_width = image->width;
+ }
+ if (cfg->g_forced_max_frame_height > image->height) {
+ cfg->g_forced_max_frame_height = image->height;
+ }
}
if (useAllIntra) {
#if !defined(AOM_USAGE_ALL_INTRA)
@@ -939,7 +945,7 @@ static avifResult aomCodecEncodeImage(avifCodec * codec,
cfg->g_lag_in_frames = 0;
}
if ((encoder->width || encoder->height) && (cfg->g_lag_in_frames > 1)) {
- // libaom does not allow changing frame dimension if
+ // aom_codec_enc_config_set() does not allow changing frame dimensions if
// g_lag_in_frames > 1.
cfg->g_lag_in_frames = 1;
}
diff --git a/src/codec_avm.c b/src/codec_avm.c
index adf26990..ad829fc8 100644
--- a/src/codec_avm.c
+++ b/src/codec_avm.c
@@ -444,6 +444,11 @@ static avifResult avmCodecEncodeImage(avifCodec * codec,
avifAddImageFlags addImageFlags,
avifCodecEncodeOutput * output)
{
+ if (encoder->width || encoder->height) {
+ avifDiagnosticsPrintf(codec->diag, "AVM does not support display-size override");
+ return AVIF_RESULT_NOT_IMPLEMENTED;
+ }
+
struct avm_codec_enc_cfg * cfg = &codec->internal->cfg;
avifBool quantizerUpdated = AVIF_FALSE;
const int quantizer = avmQualityToQuantizer(quality, image->depth);
@@ -453,11 +458,6 @@ static avifResult avmCodecEncodeImage(avifCodec * codec,
// two fields.
encoderChanges &= ~AVIF_ENCODER_CHANGE_SCALING_MODE;
- if (encoder->width || encoder->height) {
- avifDiagnosticsPrintf(codec->diag, "AVM does not support display-size override");
- return AVIF_RESULT_NOT_IMPLEMENTED;
- }
-
if (!codec->internal->encoderInitialized) {
int avmCpuUsed = -1;
if (encoder->speed != AVIF_SPEED_DEFAULT) {
diff --git a/src/codec_rav1e.c b/src/codec_rav1e.c
index 4b41bfeb..d94553d2 100644
--- a/src/codec_rav1e.c
+++ b/src/codec_rav1e.c
@@ -82,6 +82,12 @@ static avifResult rav1eCodecEncodeImage(avifCodec * codec,
return AVIF_RESULT_NOT_IMPLEMENTED;
}
+ // rav1e does not support overriding maximum frame width/height in sequence header
+ if (encoder->width || encoder->height) {
+ avifDiagnosticsPrintf(codec->diag, "rav1e does not support display-size override");
+ return AVIF_RESULT_NOT_IMPLEMENTED;
+ }
+
// rav1e does not support encoding layered image.
if (encoder->extraLayerCount > 0) {
return AVIF_RESULT_NOT_IMPLEMENTED;
@@ -90,12 +96,6 @@ static avifResult rav1eCodecEncodeImage(avifCodec * codec,
// rav1e does not support disabling lagged output. See https://github.com/xiph/rav1e/issues/2267. Ignore this setting.
(void)disableLaggedOutput;
- // rav1e does not support overriding maximum frame width/height in sequence header
- if (encoder->width || encoder->height) {
- avifDiagnosticsPrintf(codec->diag, "rav1e does not support display-size override");
- return AVIF_RESULT_NOT_IMPLEMENTED;
- }
-
avifResult result = AVIF_RESULT_UNKNOWN_ERROR;
RaConfig * rav1eConfig = NULL;
diff --git a/src/codec_svt.c b/src/codec_svt.c
index 70be13fa..71bdac06 100644
--- a/src/codec_svt.c
+++ b/src/codec_svt.c
@@ -75,6 +75,11 @@ static avifResult svtCodecEncodeImage(avifCodec * codec,
}
}
+ if (encoder->width || encoder->height) {
+ avifDiagnosticsPrintf(codec->diag, "SVT-AV1 does not support display-size override");
+ return AVIF_RESULT_NOT_IMPLEMENTED;
+ }
+
// SVT-AV1 does not support encoding layered image.
if (encoder->extraLayerCount > 0) {
return AVIF_RESULT_NOT_IMPLEMENTED;
@@ -83,11 +88,6 @@ static avifResult svtCodecEncodeImage(avifCodec * codec,
// SVT-AV1 does not support disabling lagged output. Ignore this setting.
(void)disableLaggedOutput;
- if (encoder->width || encoder->height) {
- avifDiagnosticsPrintf(codec->diag, "SVT-AV1 does not support display-size override");
- return AVIF_RESULT_NOT_IMPLEMENTED;
- }
-
avifResult result = AVIF_RESULT_UNKNOWN_ERROR;
EbColorFormat color_format = EB_YUV420;
uint8_t * uvPlanes = NULL; // 4:2:0 U and V placeholder for alpha because SVT-AV1 does not support 4:0:0.
diff --git a/src/write.c b/src/write.c
index 59095ac5..3da38e2a 100644
--- a/src/write.c
+++ b/src/write.c
@@ -525,8 +525,6 @@ avifEncoder * avifEncoderCreate(void)
encoder->maxQuantizer = AVIF_QUANTIZER_WORST_QUALITY;
encoder->minQuantizerAlpha = AVIF_QUANTIZER_BEST_QUALITY;
encoder->maxQuantizerAlpha = AVIF_QUANTIZER_WORST_QUALITY;
- encoder->width = 0;
- encoder->height = 0;
encoder->tileRowsLog2 = 0;
encoder->tileColsLog2 = 0;
encoder->autoTiling = AVIF_FALSE;
@@ -541,6 +539,8 @@ avifEncoder * avifEncoderCreate(void)
encoder->creationTime = 0;
encoder->modificationTime = 0;
encoder->sampleTransformRecipe = AVIF_SAMPLE_TRANSFORM_NONE;
+ encoder->width = 0;
+ encoder->height = 0;
return encoder;
}
@@ -574,8 +574,6 @@ static void avifEncoderBackupSettings(avifEncoder * encoder)
lastEncoder->timescale = encoder->timescale;
lastEncoder->repetitionCount = encoder->repetitionCount;
lastEncoder->extraLayerCount = encoder->extraLayerCount;
- lastEncoder->width = encoder->width;
- lastEncoder->height = encoder->height;
lastEncoder->minQuantizer = encoder->minQuantizer;
lastEncoder->maxQuantizer = encoder->maxQuantizer;
lastEncoder->minQuantizerAlpha = encoder->minQuantizerAlpha;
@@ -586,6 +584,8 @@ static void avifEncoderBackupSettings(avifEncoder * encoder)
encoder->data->lastTileColsLog2 = encoder->data->tileColsLog2;
lastEncoder->scalingMode = encoder->scalingMode;
lastEncoder->sampleTransformRecipe = encoder->sampleTransformRecipe;
+ lastEncoder->width = encoder->width;
+ lastEncoder->height = encoder->height;
}
// This function detects changes made on avifEncoder. It returns true on success (i.e., if every
@@ -1598,7 +1598,7 @@ static avifCodecType avifEncoderGetCodecType(const avifEncoder * encoder)
return avifCodecTypeFromChoice(encoder->codecChoice, AVIF_CODEC_FLAG_CAN_ENCODE);
}
-static avifResult avifEncoderValidateDisplaySizeOverride(avifEncoder * encoder, uint32_t gridCols, uint32_t gridRows, const avifImage * firstCell)
+static avifResult avifEncoderValidateDisplaySizeOverride(const avifEncoder * encoder, uint32_t gridCols, uint32_t gridRows, const avifImage * firstCell)
{
if (!avifEncoderUsesDisplaySizeOverride(encoder)) {
return AVIF_RESULT_OK;
There was a problem hiding this comment.
Done. I have to revert the addition of const to avifEncoderValidateDisplaySizeOverride because this function need to write to the diag struct when it found some unsupported config.
| printf(" --layered : Encode a layered AVIF. Each input is encoded as one layer and at most %d layers can be encoded.\n", | ||
| AVIF_MAX_AV1_LAYER_COUNT); | ||
| printf(" --display-size WxH : Set the size at which the AVIF image is displayed, independent of the encoded pixel dimensions.\n"); | ||
| printf(" Supported for still images and layered still images only.\n"); |
There was a problem hiding this comment.
Please add an entry to CHANGELOG to summarize the new features in this PR (both the avifenc command-line option and the public header changes).
| // Version 1.4.0 ends here. Add any new members after this line. | ||
| // -------------------------------------------------------------------------------------------- | ||
|
|
||
| // Overrides the image's display size (the width and height AVIF declares this image is meant |
There was a problem hiding this comment.
[This is just a comment, not a request for change.] Display size is a good name for the new encoder setting. I studied the relevant standards. I found that this is called the "visual presentation size" in ISOBMFF. Also, the AV1 spec defines the render size as the "desired display size". So these terms are all equivalent. But presentation size is a little long, and the AV1 spec does not define whether a frame should be scaled or cropped to the render size. So let's go with display size.
| // than to the coded size of whichever layer they came from. | ||
| // | ||
| // Implementation note: the display size is stored as the ispe box, the dimensions an AVIF | ||
| // decoder is required to honor; a coded image smaller than it is upscaled by the decoder to |
There was a problem hiding this comment.
[This is a comment, not a request for change. But this comment is noteworthy.] I found that scaling a coded image to the dispay size is only specified for tracks (which are used for AVIF image sequences) in ISOBMFF. But I did not find any standard that says image items (which are used for AVIF still images) should be scaled to the size in the ispe box. So the scaling of image items to the size in the ispe box seems to be a nonstandard behavior of libavif. If you have spare time, could you also research this?
I think it is too late to change libavif's behavior. I just wanted to verify my understanding.
There was a problem hiding this comment.
I realized ourselves - the AVIF standard - has a stricter rule:
https://aomediacodec.github.io/av1-avif/v1.2.0.html#image-spatial-extents-property
More specifically, for [AV1] images, the values of image_width and image_height shall respectively equal the values of UpscaledWidth and FrameHeight ... for a specific frame ...
NOTE: The dimensions of possible intermediate output images might not match the ones given in the 'ispe' property. If renderers display these intermediate images, they are expected to scale the output image to match the 'ispe' property.
This means we can get rid of the "display size" concept. It's just the size, but for layered image that size must be informed beforehand.
On the decoder side, I'm not sure if we should tighten the checks, but on the encoder side I'm changing to follow the rules.
| // every later frame must fit within it; by default that maximum is simply the size of the first | ||
| // frame encoded. Setting width/height also declares that AV1-level maximum explicitly, in | ||
| // addition to the ispe box, so that later, larger layers stay within it. This is why it must be | ||
| // at least as large as every layer's coded size. |
There was a problem hiding this comment.
[This is just a comment, not a request for change.] Nit: Thank you for adding details I requested. The new comment is longer than I expected. We can try to make it more concise later.
| // 1 in AV1 sequence headers. | ||
| // Still picture header requires frame size to match | ||
| // max_frame_width and max_frame_height, | ||
| // so we can't use it if frame can have a different size. |
There was a problem hiding this comment.
Pleae see if you like how I proposed to handle this issue in the patch.
(See patch.)
There was a problem hiding this comment.
This is indeed better. I didn't realize that if there's only one image, there's no chance for a later larger image, so we don't need the sequence header max_frame_width thing.
|
@wantehchang as stated I'm simplifying & tightening the implementation according to the AVIF spec. I need some more time to reimplement the |
3 participants
A layered image may use different frame for each layer, like using a small and blurred version as the first layer. It is more efficient and flexible to directly encode the small image, instead of scale it up only for the encoder to scale it back down; and it allows the scale ratio to be smaller than 1/8 as well.
Only single cell layered image is supported in this version. Layered grid may be revisited in the future.