avidec: output all frames of animations when passed --index all
#2670
Conversation
`avifdec image.avif out.png` will write `out.png` containing the first frame as before, and `out-xxx.png` files, one for each frame, including `out-000.png` containing the first frame, which is therefore written twice. If this behavior is not desired, `--index N` can still be used to specify the frame to decode. Refactor avifdec.c to remove duplication between --info mode and normal mode. AOMediaCodec#2634
4e7a476 to
a660360
Compare
By default, only the first frame is decoded. --index all must be used explicitly to decode all frames. In that case, the first frame is no longer written twice, only the -xxx files are written. Use ten padding zeros for the frame number, to accomodate the max value of an int32. A fixed number of leading zeros is easier for scripts to deal with.
--index all
apps/avifdec.c
Outdated
| if (decodeAllFrames) { | ||
| result = AVIF_RESULT_OK; | ||
| } else { | ||
| fprintf(stderr, "ERROR: Frame at index %d requested but the file does not contain enough frames\n", frameIndex); |
There was a problem hiding this comment.
decoder->imageCount could be mentioned in this error
There was a problem hiding this comment.
The reason I didn't add it originally was because in my understanding, we don't know if that numer matches the actual number of frames. But you're right that it's useful information in most cases.
There was a problem hiding this comment.
Looking at the code it seems to be equal to the number of track samples. I have no idea if that is equal to the number of frames in an AVIF image sequence.
There was a problem hiding this comment.
I think that in theory they should be the same, but the actual movie data might end up having a different number of frames if non compliant? In --info mode we print "(XXX expected frames)" before actually decoding the frames. But this is just a corner case.
|
Thanks for the thorough review! |
| @@ -38,7 +39,7 @@ static void syntax(void) | |||
| printf(" -u,--upsampling U : Chroma upsampling (for 420/422). One of 'automatic' (default), 'fastest', 'best', 'nearest', or 'bilinear'\n"); | |||
| printf(" -r,--raw-color : Output raw RGB values instead of multiplying by alpha when saving to opaque formats\n"); | |||
| printf(" (JPEG only; not applicable to y4m)\n"); | |||
| printf(" --index I : When decoding an image sequence or progressive image, specify which frame index to decode (Default: 0)\n"); | |||
| printf(" --index I : When decoding an image sequence or progressive image, specify which frame index to decode, where the first frame has index 0, or 'all' to decode all frames. (Default: 0)\n"); | |||
There was a problem hiding this comment.
Should this be in the changelog?
There was a problem hiding this comment.
Indeed, I forgot (again)
| fprintf(stderr, "Warning: metadata dropped when saving to y4m.\n"); | ||
| } | ||
| return y4mWrite(outputFilename, image); | ||
| } else if (outputFormat == AVIF_APP_FILE_FORMAT_JPEG) { |
There was a problem hiding this comment.
elses after returns can be avoided
There was a problem hiding this comment.
It can but I'm not sure it makes the code inhrently better? It's a code style preference, and there's nothing in the styleguide about this AFAIK. I prefer it this way, it's more compact and I find it clearer.
There was a problem hiding this comment.
As long as things are consistent. You're right, it's not in the style guide, but a clang-tidy warning.
| // This cannot fail. | ||
| result = avifImageSetProfileICC(decoder->image, NULL, 0); | ||
| assert(result == AVIF_RESULT_OK); | ||
| const int isSequence = decoder->imageCount > 1; |
| char frameFilename[1024]; | ||
| int res = snprintf(frameFilename, | ||
| sizeof(frameFilename), | ||
| "%.*s-%010d.%.*s", |
There was a problem hiding this comment.
ffmpeg allows for a format string like %03d in the input and output filenames, but this would require a little more validation. You could shorten this based on the max number of frames if that's available, but otherwise this seems OK.
There was a problem hiding this comment.
You could shorten this based on the max number of frames if that's available, but otherwise this seems OK.
I did that at first but we decided that having a predictible number of zeros (and therefore filename) would be easier for scripts. And that we could implement the "%03d" syntax if someone ever asked for it, but it really seems like a "nice to have".
* main: (53 commits) Fix reading gray/color ICC profile from color/gray image in apps (AOMediaCodec#2675) Revert "Use SVT_LOG=0 in avifsvttest for SVT-AV1 3.0.0 (AOMediaCodec#2668)" (AOMediaCodec#2674) Fix monochrome ICC profile error message in apps (AOMediaCodec#2673) Add missing changelog for `avifdec --index all` (AOMediaCodec#2672) Specify target platform version in binary artifacts (AOMediaCodec#2652) avidec: output all frames of animations when passed `--index all` (AOMediaCodec#2670) Do not allow keeping the ICC when converting from RGB to gray (AOMediaCodec#2669) Update CHANGELOG (AOMediaCodec#2666) Use SVT_LOG=0 in avifsvttest for SVT-AV1 3.0.0 (AOMediaCodec#2668) read.c: fix empty struct initializers (AOMediaCodec#2661) avifutil.c: fix avifQueryCPUCount() empty param list (AOMediaCodec#2662) Add libyuv dependency to the README.md (AOMediaCodec#2664) fix: patch libyuv CMakeLists for compatibility with gcc 10 (AOMediaCodec#2660) Use default for tensorflow with libavm (AOMediaCodec#2624) Bump some Rust dependencies (AOMediaCodec#2658) Use C99 syntax to initialize avifFileType struct (AOMediaCodec#2657) Bump the github-actions group with 3 updates libargparse: use a git patch file instead of sed command (AOMediaCodec#2654) y4m: Remove some repeated code Remove fully static instructions (AOMediaCodec#2649) ...
…MediaCodec#2670) Also refactor avifdec.c to remove duplication between --info mode and normal mode.
…MediaCodec#2670) Also refactor avifdec.c to remove duplication between --info mode and normal mode.
Also refactor avifdec.c to remove duplication between --info mode and normal mode.
#2634