Support encoding layered image with pre-scaled inputs - #1069
tongyuantongyu wants to merge 18 commits into
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! |
70f3317 to
c46adb0
Compare
The addition of
"If |
|
Yuan: Thank you for the reply. Please keep |
wantehchang
left a comment
wantehchang
left a comment
There was a problem hiding this comment.
Yuan: I reviewed your new changes to the src/ directory today. I have some questions and suggestions. I will review your new changes to the apps/ directory next (hopefully tomorrow).
| AVIF_CHECKRES(avifEncoderValidateSize(encoder, gridCols, gridRows, firstCell)); | ||
| // Reject mixing scalingMode and adding input images of different sizes. | ||
| // They are different ways the achieve the same result, so mixing them | ||
| // creates confusion without providing any benefit. |
There was a problem hiding this comment.
Can we move this comment to the avifEncoderValidateConsistentSize() function definition?
Also change "the achieve" to "to achieve".
There was a problem hiding this comment.
I merged them since they largely share the theme, are called back-to-back and are only called here.
eff3cae to
09021a9
Compare
|
|
||
| * Add the ignoreICC option to avifDecoder | ||
| * Support ignoring alpha in avifDecoder::imageContentToDecode | ||
| * Support encoding layered image with pre-scaled inputs |
There was a problem hiding this comment.
Yuan: Can you rebase this pull request to the tip of the main branch? This will fix the build-android-jni CI workflow failure. Do not make any other changes, because I am about to finish my review and send you the final review comments. Thanks!
Status update: I may not have time to send you the final review comments until this weekend. I have reviewed everything except avifenc.c. I only have some questions about avifImagePeek() and the new parameters of avifImageDump(). I will ask you to just add some comments so we can merge this PR with minimal changes. But I will also suggest possible follow-up cleanup.
There was a problem hiding this comment.
Let me do a merge instead so the commits you are looking at won't change. You probably will squash when merging this PR, so there shouldn't be other differences.
There was a problem hiding this comment.
Let me guess your questions:
About avifPeekImage(): the semi-public API avifReadImage() is already getting a bit too many arguments, so I avoided overloading it further.
libavif/apps/shared/avifutil.c
Lines 324 to 338 in c46adb0
About avifImageDump(): since gain map info need gridCols/gridRows, we cannot precompute the image size and eliminate them:
libavif/apps/shared/avifutil.c
Lines 156 to 161 in c46adb0
Use cinfo->output_width/height instead of cinfo->image_width/height in avifJPEGCopyPixels() because cinfo->output_width/height is the resolution of the pixels jpeg_read_raw_data() produces. Suggested by Yuan Tong in #1069 (comment).
| // are interpreted relative to that size rather than to this intermediate layer's own | ||
| // width/height. No special handling is needed during decode: the decoded layer is | ||
| // automatically scaled to the configured size, so these transformations are relative to the | ||
| // decoded avifImage's width/height as usual. |
There was a problem hiding this comment.
Note: This is just a comment, not a request for change.
If I understand it correctly, libavif also scales a non-layered still image to the size in the ispe box. I believe this scaling is a "nonstandard" behavior of libavif. Do you agree?
431b0b9 to
862cbd4
Compare
| .WithDomains(ArbitraryAvifLayered(), ArbitraryScalingModes(), | ||
| ArbitraryAvifEncoder(), ArbitraryAvifDecoder()); | ||
|
|
||
| FUZZ_TEST(LayeredEncodeDecodeAvifFuzzTest, EncodeDecodeDimensionChangeExternal) |
There was a problem hiding this comment.
This test crashed with a segmentation fault in the CI workflow. I don't know how to reproduce it. I can merge this PR first and debug it when oss-fuzz reproduces the crash.
There was a problem hiding this comment.
FuzzTest prints the seed it used, and you can reproduce the exact case with that seed:
FUZZTEST_PRNG_SEED=Au4QmbGabXIyniIaXGJf3kW1n2GlG3xMpoj3E8l38Hc ./tests/avif_fuzztest_enc_dec_layered --gtest_filter="*EncodeDecodeDimensionChangeExternal*"
Thr crash is inside AOM, and likely related to extremely small intermediate layer size with large tiling. The case encodes a 3rd layer with size 1x141, with encoder size 8x256 and tileRowsLog2=6. It also allows maxThreads=64 workers.
Thread 7 received signal SIGSEGV, Segmentation fault.
[Switching to Thread 8964.0x7030]
0x00007ff67f55009e in encode_sb_row ()
#0 0x00007ff67f55009e in encode_sb_row ()
#1 0x00007ff67f550c8c in av1_encode_sb_row ()
#2 0x00007ff67f599837 in enc_row_mt_worker_hook ()
#3 0x00007ff67fa6eb28 in thread_loop ()
There was a problem hiding this comment.
Yuan: Thank you! I reproduced the crash now. I will debug it.
There was a problem hiding this comment.
cc: @jzern
This crash can be fixed by https://aomedia-review.googlesource.com/c/aom/+/216921. I don't know why. Ideally we should figure out a way for libavif to avoid this crash.
The stack trace of the crash is:
lowbd_compute_gradient_info_sb (x=0x55555710b100, sb_size=BLOCK_64X64, plane=0 '\000')
at aom/av1/encoder/intra_mode_search_utils.h:208
208 grad_info_sb[r * sb_width + c].is_dx_zero = (dx == 0);
(gdb) bt
#0 lowbd_compute_gradient_info_sb (x=0x55555710b100, sb_size=BLOCK_64X64, plane=0 '\000')
at aom/av1/encoder/intra_mode_search_utils.h:208
#1 0x00005555559c6ed8 in compute_gradient_info_sb (x=0x55555710b100, sb_size=BLOCK_64X64, plane=0 '\000')
at aom/av1/encoder/intra_mode_search_utils.h:312
#2 0x00005555559c7010 in produce_gradients_for_sb (cpi=0x555556f017e0, x=0x55555710b100, sb_size=BLOCK_64X64, mi_row=16,
mi_col=32) at aom/av1/encoder/intra_mode_search_utils.h:353
#3 0x00005555559cb205 in encode_sb_row (cpi=0x555556f017e0, td=0x55555710b100, tile_data=0x5555581ace00, mi_row=16,
tp=0x7ffff6805cd8) at aom/av1/encoder/encodeframe.c:1330
#4 0x00005555559cbca4 in av1_encode_sb_row (cpi=0x555556f017e0, td=0x55555710b100, tile_row=1, tile_col=2, mi_row=16)
at aom/av1/encoder/encodeframe.c:1524
#5 0x0000555555a3a121 in enc_row_mt_worker_hook (arg1=0x55555784de30, unused=0x0)
at aom/av1/encoder/ethread.c:727
#6 0x0000555555f04389 in execute (worker=0x555556cd73a0)
at aom/aom_util/aom_thread.c:199
#7 0x0000555555f03f52 in thread_loop (ptr=0x555556cd73a0)
at aom/aom_util/aom_thread.c:84
#8 0x00007ffff789e918 in start_thread (arg=<optimized out>) at ./nptl/pthread_create.c:454
#9 0x00007ffff791cfd8 in __GI___clone3 () at ../sysdeps/unix/sysv/linux/x86_64/clone3.S:78
(gdb) p grad_info_sb
$1 = (PixelLevelGradientInfo * const) 0x0
(gdb) up
#1 0x00005555559c6ed8 in compute_gradient_info_sb (x=0x55555710b100, sb_size=BLOCK_64X64, plane=0 '\000')
at aom/av1/encoder/intra_mode_search_utils.h:312
312 lowbd_compute_gradient_info_sb(x, sb_size, plane);
(gdb) p x->pixel_gradient_info
$2 = (PixelLevelGradientInfo *) 0x0
(gdb
There was a problem hiding this comment.
The crash actually does not necessarily require a change to the input size. It needs That also happens when encoding an animation, and larger g_lag_in_frames <= 1, but we also set that when encoding an animation with alphag_lag_in_frames only delays the number of frames needed for it to happen:
magick -size 128x1024 xc:gray -alpha set -channel A -evaluate set 60% +channel flat.png
avifenc -s 6 -j 8 -k 2 flat.png flat.png --tilerowslog2:u 2 flat.png -o flat.avif # Segmentation fault
It happens when the number of encoding worker threads grows (happens when new frame is larger or number of tiles increased) while the previous frame was not a keyframe. In that case the new workers are allocated without their gradient cache, so any later keyframe crashes.
Current AOM release forces keyframe when new frame is larger, unless cpi->svc.number_spatial_layers > 1, but libavif uses AOME_SET_NUMBER_SPATIAL_LAYERS(ppi->number_spatial_layers) instead, so this can only be triggered by
- Increasing
tileRowsLog2/tileColsLog2 - Encoding layer 0 and layer 1 of the same size and layer 2 of a larger size.
We can track whether the previous frame was a keyframe, and reject increasing frame size or changing tiling config if the previous frame was not.
8521fb3 to
c0d3281
Compare
| } | ||
| } | ||
| } else { | ||
| // Detect the libaom bug before v3.15.2 described in |
There was a problem hiding this comment.
I'm not sure if the next version will be v3.15.2 or v3.16.0.
c0d3281 to
497d35f
Compare
|
@wantehchang while debugging, I found that the current release of AOM will make every larger frame a keyframe when using pre-scaled inputs, and https://aomedia-review.googlesource.com/c/aom/+/216241 only lifted that restriction for REALTIME usage. There's actually only a little work left to enable that for GOOD_QUALITY as well, so I submitted a change to do that: https://aomedia-review.googlesource.com/c/aom/+/217061. |
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.