PR #25297 opened by Marti2203 URL: https://code.ffmpeg.org/FFmpeg/FFmpeg/pulls/25297 Patch URL: https://code.ffmpeg.org/FFmpeg/FFmpeg/pulls/25297.patch
This fixes two problems in `rd_frame()` in the Cinepak encoder. **1. Keyframes the encoder picks itself don't decode as keyframes.** `rd_frame()` copies the best candidate from `s->frame_buf` into the packet as soon as it finds it. When a frame ends up with no skipped MBs, it is flagged as a keyframe, and the strip ids are then rewritten from 0x11 (inter) to 0x10 (intra). That rewrite went to `s->frame_buf`, after the copy, so it never reached the packet. Such frames, typically at a scene cut, are marked K in the container but still have strip id 0x11, so FFmpeg's own decoder reports `key_frame=0` for them. The first commit writes the rewrite to the output buffer instead. ``` ffmpeg -f lavfi -i "testsrc=s=64x64:r=10:d=0.5[a];smptebars=s=64x64:r=10:d=0.5[b];[a][b]concat=n=2" -c:v cinepak -g 100 cut.avi ffprobe -show_entries packet=flags cut.avi / ffprobe -show_entries frame=key_frame cut.avi ``` Frame 5 (the scene cut), before: packet `K__`, `key_frame=0`. After: packet `K__`, `key_frame=1`. All other frames are unchanged. The existing `vsynth1-cinepak` test already hits this case: frames 21 and 41 were flagged K but decoded as non-key. Its reference md5 changes only because of the corrected strip id bytes. The file size and the decoded output stay the same. **2. min_strips larger than height / 4 uses uninitialised variables.** The loop over strip counts stops at `height / 4`. If `-min_strips` is larger than that, the loop never runs, and `best_nstrips`, `best_strip_offsets` and `best_size` are used uninitialised. On master: ``` ffmpeg -f lavfi -i testsrc=s=320x96:d=0.1 -c:v cinepak -min_strips 32 -max_strips 32 -f null - ``` This segfaults in 3 of 5 runs. ASan reports `SEGV in write_strip_keyframe cinepakenc.c:843` from `rd_frame cinepakenc.c:1138`. With `testsrc=s=4x4:r=10:d=0.3 -min_strips 2` it exits 0 but writes a single 32629-byte packet, and valgrind reports 13 uses of uninitialised values. The second commit rejects such a value in init, like the existing min_strips > max_strips check: ``` [cinepak @ ...] minimum number of strips must not exceed height / 4 (got 32 and 24) ``` I could also clamp min_strips to height / 4 with a warning if that is preferred. Testing: the third commit adds `fate-cinepak-scene-cut`, which needs no samples. It encodes a lavfi testsrc to red cut with `-g 100` and checks packet flags and the decoded `key_frame`. It fails on master and passes with the fix. `fate-vsynth1-cinepak` (updated reference), `fate-vsynth2-cinepak` and `fate-vsynth_lena-cinepak` pass. `repro.sh` in the drafts runs all of the above against a given ffmpeg/ffprobe. Caveat: #25195 / #23216 (chunk alignment for the Sega Saturn) also change `tests/ref/vsynth/*-cinepak`. Whichever lands second will need its vsynth1 reference regenerated. This issue was found by an LLM-Enhanced Static Analysis Engine and verified manually. 🤖 Generated with [Claude Code](https://claude.com/claude-code) From 133febd4cc085e0d98db18a05d1797e0fe63be60 Mon Sep 17 00:00:00 2001 From: Martin Mirchev <[email protected]> Date: Wed, 21 Oct 2026 18:21:58 +0800 Subject: [PATCH 1/3] avcodec/cinepakenc: mark the strips of unforced keyframes in the packet rd_frame() copies the best candidate from s->frame_buf to the packet as soon as it is found, but the final strip id fix-up for frames that turned out to have no skipped MBs was applied to s->frame_buf afterwards and so never reached the packet. Such frames, e.g. at a scene cut, were flagged as keyframes by the encoder but kept the inter strip id 0x11, so the decoder did not mark them as keyframes and a player seeking to them could not start decoding there. Write the fix-up to the output buffer instead. In vsynth1-cinepak this changes frames 21 and 41, which were flagged as keyframes but did not decode as such. Signed-off-by: Martin Mirchev <[email protected]> --- libavcodec/cinepakenc.c | 2 +- tests/ref/vsynth/vsynth1-cinepak | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/libavcodec/cinepakenc.c b/libavcodec/cinepakenc.c index 104a9f485ea..8963ad60f43 100644 --- a/libavcodec/cinepakenc.c +++ b/libavcodec/cinepakenc.c @@ -1135,7 +1135,7 @@ static int rd_frame(CinepakEncContext *s, const AVFrame *frame, // update strip headers for (i = 0; i < best_nstrips; i++) { - write_strip_keyframe(s->frame_buf + best_strip_offsets[i], *got_keyframe); + write_strip_keyframe(buf + best_strip_offsets[i], *got_keyframe); } // let the number of strips slowly adapt to the changes in the contents, diff --git a/tests/ref/vsynth/vsynth1-cinepak b/tests/ref/vsynth/vsynth1-cinepak index 7cb33220e74..9e4d3ceb5fb 100644 --- a/tests/ref/vsynth/vsynth1-cinepak +++ b/tests/ref/vsynth/vsynth1-cinepak @@ -1,4 +1,4 @@ -6b9b4d2b32854fd8b0f84ee4d76b1fed *tests/data/fate/vsynth1-cinepak.avi +bdada851736078a8c4f83c31f808fb92 *tests/data/fate/vsynth1-cinepak.avi 408910 tests/data/fate/vsynth1-cinepak.avi 4acdcc3e664d12facbcefc389079cfa6 *tests/data/fate/vsynth1-cinepak.out.rawvideo stddev: 61.38 PSNR: 12.37 MAXDIFF: 223 bytes: 7603200/ 921600 -- 2.54.0 From 68105978e252ec68a60ba7cb898457774bf28311 Mon Sep 17 00:00:00 2001 From: Martin Mirchev <[email protected]> Date: Wed, 21 Oct 2026 18:22:27 +0800 Subject: [PATCH 2/3] avcodec/cinepakenc: reject min_strips larger than height / 4 rd_frame() only tries strip counts up to height / 4, the number of macroblock rows. If min_strips is larger than that (e.g. 320x96 with -min_strips 32, or 4x4 with -min_strips 2), the loop over strip counts never runs and best_nstrips, best_strip_offsets and best_size are used uninitialized, which crashes in write_strip_keyframe() or returns a garbage packet size. Check the option against the frame height at init time, like the existing min_strips > max_strips check. Signed-off-by: Martin Mirchev <[email protected]> --- libavcodec/cinepakenc.c | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/libavcodec/cinepakenc.c b/libavcodec/cinepakenc.c index 8963ad60f43..86814400c99 100644 --- a/libavcodec/cinepakenc.c +++ b/libavcodec/cinepakenc.c @@ -169,6 +169,12 @@ static av_cold int cinepak_encode_init(AVCodecContext *avctx) return AVERROR(EINVAL); } + if (s->min_min_strips > avctx->height / MB_SIZE) { + av_log(avctx, AV_LOG_ERROR, "minimum number of strips must not exceed height / 4 (got %i and %i)\n", + s->min_min_strips, avctx->height / MB_SIZE); + return AVERROR(EINVAL); + } + if (!(s->last_frame = av_frame_alloc())) return AVERROR(ENOMEM); if (!(s->best_frame = av_frame_alloc())) -- 2.54.0 From 574ce150fbf77ed2f2a46327715baba7beda217d Mon Sep 17 00:00:00 2001 From: Martin Mirchev <[email protected]> Date: Wed, 21 Oct 2026 18:22:27 +0800 Subject: [PATCH 3/3] tests/fate/vcodec: add a cinepak test for keyframes at a scene cut Encode a scene cut with a large GOP so that the encoder decides on its own to code the frame after the cut as a keyframe, and check that the decoder also reports it as a keyframe. Signed-off-by: Martin Mirchev <[email protected]> --- tests/fate/vcodec.mak | 11 ++++++++++ tests/ref/fate/cinepak-scene-cut | 37 ++++++++++++++++++++++++++++++++ 2 files changed, 48 insertions(+) create mode 100644 tests/ref/fate/cinepak-scene-cut diff --git a/tests/fate/vcodec.mak b/tests/fate/vcodec.mak index a5abace91fc..2a2a6eba811 100644 --- a/tests/fate/vcodec.mak +++ b/tests/fate/vcodec.mak @@ -545,3 +545,14 @@ fate-vsynth2: $(FATE_VSYNTH2) fate-vsynth_lena: $(FATE_VSYNTH_LENA) fate-vsynth3: $(FATE_VSYNTH3) fate-vcodec: fate-vsynth1 fate-vsynth_lena fate-vsynth2 fate-vsynth3 + +# The encoder turns the frame after the scene cut into a keyframe on its own +# (-g is large); its strips must then be coded as intra strips as well, so +# that the decoder also treats it as a keyframe. +FATE_CINEPAK-$(call TRANSCODE, CINEPAK, AVI, LAVFI_INDEV TESTSRC_FILTER COLOR_FILTER CONCAT_FILTER) += fate-cinepak-scene-cut +fate-cinepak-scene-cut: CMD = transcode lavfi \ + "testsrc=s=64x64:r=10:d=0.5[a];color=c=red:s=64x64:r=10:d=0.5,format=rgb24[b];[a][b]concat=n=2" \ + avi "-c:v cinepak -g 100" "-c copy" "-show_entries packet=flags:frame=key_frame -of compact" + +FATE_FFMPEG += $(FATE_CINEPAK-yes) +fate-cinepak: $(FATE_CINEPAK-yes) diff --git a/tests/ref/fate/cinepak-scene-cut b/tests/ref/fate/cinepak-scene-cut new file mode 100644 index 00000000000..56ee49f6d3c --- /dev/null +++ b/tests/ref/fate/cinepak-scene-cut @@ -0,0 +1,37 @@ +1c07cf9b43e1f8c86b3ff7ecc466e30d *tests/data/fate/cinepak-scene-cut.avi +11426 tests/data/fate/cinepak-scene-cut.avi +#tb 0: 1/10 +#media_type 0: video +#codec_id 0: cinepak +#dimensions 0: 64x64 +#sar 0: 1/1 +0, 0, 0, 1, 2126, 0x3ad207fd +0, 1, 1, 1, 672, 0xff30cb2e, F=0x0 +0, 2, 2, 1, 682, 0x6dc2e531, F=0x0 +0, 3, 3, 1, 680, 0xfaffdc5f, F=0x0 +0, 4, 4, 1, 708, 0xeae4ef03, F=0x0 +0, 5, 5, 1, 326, 0x80de099d +0, 6, 6, 1, 66, 0x6c99022c, F=0x0 +0, 7, 7, 1, 66, 0x6c99022c, F=0x0 +0, 8, 8, 1, 66, 0x6c99022c, F=0x0 +0, 9, 9, 1, 66, 0x6c99022c, F=0x0 +packet|flags=K__ +frame|key_frame=1 +packet|flags=___ +frame|key_frame=0 +packet|flags=___ +frame|key_frame=0 +packet|flags=___ +frame|key_frame=0 +packet|flags=___ +frame|key_frame=0 +packet|flags=K__ +frame|key_frame=1 +packet|flags=___ +frame|key_frame=0 +packet|flags=___ +frame|key_frame=0 +packet|flags=___ +frame|key_frame=0 +packet|flags=___ +frame|key_frame=0 -- 2.54.0 _______________________________________________ ffmpeg-devel mailing list -- [email protected] To unsubscribe send an email to [email protected]
