James Almer: > Tag only packets containing with IDR pictures as keyframes by default, same as > the decoder. > Fixes MP4 and Matroska muxers marking incorrect samples as Sync Samples in > scenarios where this AVParser is used. >
1. Could you please explain where you got the info that Matroska keyframes need to be ISOBMFF Sync Samples from? Because it's actually undefined what it exactly is; in case of AV1 (the only codec with a detailed codec mapping) said mapping allows delayed random access points to be marked as keyframes (without providing anything to actually signal that these are delayed random access points), so a key frame is simply a random access point. And that is how it is de-facto handled with all other codecs as well. IMO it is ISOBMFF which is the outlier here. > Signed-off-by: James Almer <jamr...@gmail.com> > --- > libavcodec/h264_parser.c | 16 ++++++++++------ > tests/fate-run.sh | 4 ++-- > tests/fate/ffmpeg.mak | 2 +- > tests/fate/lavf-container.mak | 12 ++++++------ > tests/fate/matroska.mak | 2 +- > tests/ref/fate/copy-trac2211-avi | 2 +- > tests/ref/fate/matroska-h264-remux | 4 ++-- > tests/ref/fate/segment-mp4-to-ts | 10 +++++----- > tests/ref/lavf-fate/h264.mp4 | 4 ++-- > 9 files changed, 30 insertions(+), 26 deletions(-) > > diff --git a/libavcodec/h264_parser.c b/libavcodec/h264_parser.c > index d3c56cc188..532dc462b0 100644 > --- a/libavcodec/h264_parser.c > +++ b/libavcodec/h264_parser.c > @@ -344,9 +344,11 @@ static inline int parse_nal_units(AVCodecParserContext > *s, > get_ue_golomb_long(&nal.gb); // skip first_mb_in_slice > slice_type = get_ue_golomb_31(&nal.gb); > s->pict_type = ff_h264_golomb_to_pict_type[slice_type % 5]; > - if (p->sei.recovery_point.recovery_frame_cnt >= 0) { > - /* key frame, since recovery_frame_cnt is set */ > - s->key_frame = 1; > + if (s->flags & PARSER_FLAG_USE_KEYFRAME_HEURISTICS) { > + if (p->sei.recovery_point.recovery_frame_cnt >= 0) { > + /* key frame, since recovery_frame_cnt is set */ > + s->key_frame = 1; > + } 2. This change means that files that don't use IDR pictures, but only recovery point SEIs won't have packets marked as keyframes at all any more; this affects every open-gop video. This is way worse than an incorrect sync sample list created by the mp4 muxer. (When using x264 with open-gop, it even means that every stream so encoded will only have one keyframe (the IDR frame at the beginning), because for some reason x264 never uses IDR frames in open-gop mode even at scenecuts (where the gop is closed and where using an IDR frame instead of a recovery point SEI would actually save a few bytes).) > } > pps_id = get_ue_golomb(&nal.gb); > if (pps_id >= MAX_PPS_COUNT) { > @@ -370,9 +372,11 @@ static inline int parse_nal_units(AVCodecParserContext > *s, > p->ps.sps = p->ps.pps->sps; > sps = p->ps.sps; > > - // heuristic to detect non marked keyframes > - if (p->ps.sps->ref_frame_count <= 1 && p->ps.pps->ref_count[0] > <= 1 && s->pict_type == AV_PICTURE_TYPE_I) > - s->key_frame = 1; > + if (s->flags & PARSER_FLAG_USE_KEYFRAME_HEURISTICS) { > + // heuristic to detect non marked keyframes > + if (p->ps.sps->ref_frame_count <= 1 && > p->ps.pps->ref_count[0] <= 1 && s->pict_type == AV_PICTURE_TYPE_I) > + s->key_frame = 1; > + } > > p->poc.frame_num = get_bits(&nal.gb, sps->log2_max_frame_num); > > diff --git a/tests/fate-run.sh b/tests/fate-run.sh > index ba437dfbb8..8680e35524 100755 > --- a/tests/fate-run.sh > +++ b/tests/fate-run.sh > @@ -339,8 +339,8 @@ lavf_container_fate() > outdir="tests/data/lavf-fate" > file=${outdir}/lavf.$t > input="${target_samples}/$1" > - do_avconv $file -auto_conversion_filters $DEC_OPTS $2 -i "$input" > "$ENC_OPTS -metadata title=lavftest" -vcodec copy -acodec copy > - do_avconv_crc $file -auto_conversion_filters $DEC_OPTS -i > $target_path/$file $3 > + do_avconv $file -auto_conversion_filters $DEC_OPTS $2 -i "$input" > "$ENC_OPTS -metadata title=lavftest" -vcodec copy -acodec copy $3 > + do_avconv_crc $file -auto_conversion_filters $DEC_OPTS -i > $target_path/$file $4 > } > > lavf_image(){ > diff --git a/tests/fate/ffmpeg.mak b/tests/fate/ffmpeg.mak > index 4dfb77d250..57d16fba6f 100644 > --- a/tests/fate/ffmpeg.mak > +++ b/tests/fate/ffmpeg.mak > @@ -110,7 +110,7 @@ fate-copy-trac4914-avi: CMD = transcode mpegts > $(TARGET_SAMPLES)/mpeg2/xdcam8mp2 > FATE_STREAMCOPY-$(call ALLYES, H264_DEMUXER AVI_MUXER) += > fate-copy-trac2211-avi > fate-copy-trac2211-avi: $(SAMPLES)/h264/bbc2.sample.h264 > fate-copy-trac2211-avi: CMD = transcode "h264 -r 14" > $(TARGET_SAMPLES)/h264/bbc2.sample.h264\ > - avi "-c:a copy -c:v copy" > + avi "-c:a copy -c:v copy -copyinkf" > > FATE_STREAMCOPY-$(call ENCDEC, APNG, APNG) += fate-copy-apng > fate-copy-apng: fate-lavf-apng > diff --git a/tests/fate/lavf-container.mak b/tests/fate/lavf-container.mak > index 9e0eed4851..40250badc1 100644 > --- a/tests/fate/lavf-container.mak > +++ b/tests/fate/lavf-container.mak > @@ -71,13 +71,13 @@ FATE_LAVF_CONTAINER_FATE = > $(FATE_LAVF_CONTAINER_FATE-yes:%=fate-lavf-fate-%) > $(FATE_LAVF_CONTAINER_FATE): REF = > $(SRC_PATH)/tests/ref/lavf-fate/$(@:fate-lavf-fate-%=%) > $(FATE_LAVF_CONTAINER_FATE): $(AREF) $(VREF) > > -fate-lavf-fate-av1.mp4: CMD = lavf_container_fate > "av1-test-vectors/av1-1-b8-05-mv.ivf" "" "-c:v copy" > -fate-lavf-fate-av1.mkv: CMD = lavf_container_fate > "av1-test-vectors/av1-1-b8-05-mv.ivf" "" "-c:v copy" > -fate-lavf-fate-h264.mp4: CMD = lavf_container_fate "h264/intra_refresh.h264" > "" "-c:v copy" > +fate-lavf-fate-av1.mp4: CMD = lavf_container_fate > "av1-test-vectors/av1-1-b8-05-mv.ivf" "" "" "-c:v copy" > +fate-lavf-fate-av1.mkv: CMD = lavf_container_fate > "av1-test-vectors/av1-1-b8-05-mv.ivf" "" "" "-c:v copy" > +fate-lavf-fate-h264.mp4: CMD = lavf_container_fate "h264/intra_refresh.h264" > "" "-copyinkf" "-c:v copy -copyinkf" 3. You are adding copyinkf twice here; are you sure that the new parameter to lavf_container_fate is even necessary? > fate-lavf-fate-vp3.ogg: CMD = lavf_container_fate "vp3/coeff_level64.mkv" > "-idct auto" > -fate-lavf-fate-vp8.ogg: CMD = lavf_container_fate "vp8/RRSF49-short.webm" "" > "-acodec copy" > -fate-lavf-fate-latm: CMD = lavf_container_fate "aac/al04_44.mp4" "" "-acodec > copy" > -fate-lavf-fate-mp3: CMD = lavf_container_fate "mp3-conformance/he_32khz.bit" > "" "-acodec copy" > +fate-lavf-fate-vp8.ogg: CMD = lavf_container_fate "vp8/RRSF49-short.webm" "" > "" "-acodec copy" > +fate-lavf-fate-latm: CMD = lavf_container_fate "aac/al04_44.mp4" "" "" > "-acodec copy" > +fate-lavf-fate-mp3: CMD = lavf_container_fate "mp3-conformance/he_32khz.bit" > "" "" "-acodec copy" > fate-lavf-fate-qtrle_mace6.mov: CMD = lavf_container_fate > "qtrle/Animation-16Greys.mov" "-idct auto" > fate-lavf-fate-cram.avi: CMD = lavf_container_fate "cram/toon.avi" "-idct > auto" > > diff --git a/tests/fate/matroska.mak b/tests/fate/matroska.mak > index ca7193a055..545a0d1d50 100644 > --- a/tests/fate/matroska.mak > +++ b/tests/fate/matroska.mak > @@ -105,7 +105,7 @@ FATE_MATROSKA_FFMPEG_FFPROBE-$(call ALLYES, FILE_PROTOCOL > MPEGTS_DEMUXER \ > MATROSKA_DEMUXER H264_DECODER > \ > FRAMECRC_MUXER PIPE_PROTOCOL) > \ > += fate-matroska-h264-remux > -fate-matroska-h264-remux: CMD = transcode mpegts > $(TARGET_SAMPLES)/h264/h264_intra_first-small.ts matroska "-map 0:0 -map 0 > -c:v copy -sar:0 3:4 -bsf:v:1 h264_metadata=aud=remove:delete_filler=1 > -disposition:v +hearing_impaired -af aresample -c:a:0 pcm_s32le -c:a:1 > pcm_s32be -disposition:a:0 original -metadata:s:a:0 title=swedish_silence > -metadata:s:a:1 title=norwegian_silence -disposition:a:1 dub" "-map 0:v" "" > "-show_entries stream=index,codec_name:stream_tags=title,language" > +fate-matroska-h264-remux: CMD = transcode mpegts > $(TARGET_SAMPLES)/h264/h264_intra_first-small.ts matroska "-map 0:0 -map 0 > -c:v copy -copyinkf -sar:0 3:4 -bsf:v:1 > h264_metadata=aud=remove:delete_filler=1 -disposition:v +hearing_impaired -af > aresample -c:a:0 pcm_s32le -c:a:1 pcm_s32be -disposition:a:0 original > -metadata:s:a:0 title=swedish_silence -metadata:s:a:1 title=norwegian_silence > -disposition:a:1 dub" "-map 0:v" "" "-show_entries > stream=index,codec_name:stream_tags=title,language" > > # Tests writing BlockAdditional and BlockGroups with ReferenceBlock elements; > # it also tests setting a track as suitable for hearing impaired. > diff --git a/tests/ref/fate/copy-trac2211-avi > b/tests/ref/fate/copy-trac2211-avi > index 06d81e537d..1f71ae65f2 100644 > --- a/tests/ref/fate/copy-trac2211-avi > +++ b/tests/ref/fate/copy-trac2211-avi > @@ -1,4 +1,4 @@ > -0920978f3f8196413c43f0033b55a5b6 *tests/data/fate/copy-trac2211-avi.avi > +ee1e66eac40569ae3cf9552286900900 *tests/data/fate/copy-trac2211-avi.avi > 1777956 tests/data/fate/copy-trac2211-avi.avi > #tb 0: 1/14 > #media_type 0: video > diff --git a/tests/ref/fate/matroska-h264-remux > b/tests/ref/fate/matroska-h264-remux > index 14e6758fa0..7b852f8266 100644 > --- a/tests/ref/fate/matroska-h264-remux > +++ b/tests/ref/fate/matroska-h264-remux > @@ -1,5 +1,5 @@ > -ded6da7e46ce7df1232b116afb0b2f0a > *tests/data/fate/matroska-h264-remux.matroska > -2036083 tests/data/fate/matroska-h264-remux.matroska > +d5fc08094380fc8aba485c09b596ceee > *tests/data/fate/matroska-h264-remux.matroska > +2371935 tests/data/fate/matroska-h264-remux.matroska The increase in filesize shown here is due to your copyinkf (which you had to add because without it no video packets would be in the output at all): with it, the undecodable frames before the actual keyframes are not dropped. And the keyframes are not marked as such. So copyinkf can't even be used as a workaround. > #tb 0: 1/25 > #media_type 0: video > #codec_id 0: rawvideo > diff --git a/tests/ref/fate/segment-mp4-to-ts > b/tests/ref/fate/segment-mp4-to-ts > index 8b0746fa92..5c456cd0bc 100644 > --- a/tests/ref/fate/segment-mp4-to-ts > +++ b/tests/ref/fate/segment-mp4-to-ts > @@ -25,7 +25,7 @@ > 0, 57600, 64800, 3600, 1182, 0xbe1a4847, F=0x0, S=1, > 1 > 0, 61200, 61200, 3600, 809, 0x8d948a4e, F=0x0, S=1, > 1 > 0, 64800, 68400, 3600, 656, 0x4fa03c2b, F=0x0, S=1, > 1 > -0, 68400, 86400, 3600, 26555, 0x5629b584, S=1, 1 > +0, 68400, 86400, 3600, 26555, 0x5629b584, F=0x0, S=1, > 1 > 0, 72000, 79200, 3600, 1141, 0x761b31e8, F=0x0, S=1, > 1 > 0, 75600, 75600, 3600, 717, 0x57746351, F=0x0, S=1, > 1 > 0, 79200, 82800, 3600, 693, 0x78b24263, F=0x0, S=1, > 1 > @@ -49,7 +49,7 @@ > 0, 144000, 151200, 3600, 1271, 0x46006870, F=0x0, S=1, > 1 > 0, 147600, 147600, 3600, 849, 0x94dc99c7, F=0x0, S=1, > 1 > 0, 151200, 154800, 3600, 753, 0xf4236cab, F=0x0, S=1, > 1 > -0, 154800, 172800, 3600, 25825, 0xd5464dee, S=1, 1 > +0, 154800, 172800, 3600, 25825, 0xd5464dee, F=0x0, S=1, > 1 > 0, 158400, 165600, 3600, 1206, 0x8ce84344, F=0x0, S=1, > 1 > 0, 162000, 162000, 3600, 867, 0x312fa07d, F=0x0, S=1, > 1 > 0, 165600, 169200, 3600, 719, 0x810666d1, F=0x0, S=1, > 1 > @@ -73,7 +73,7 @@ > 0, 230400, 237600, 3600, 1545, 0x0099fc98, F=0x0, S=1, > 1 > 0, 234000, 234000, 3600, 929, 0xfd72d049, F=0x0, S=1, > 1 > 0, 237600, 241200, 3600, 829, 0xcfda9e96, F=0x0, S=1, > 1 > -0, 241200, 259200, 3600, 24220, 0x5ca21d71, S=1, 1 > +0, 241200, 259200, 3600, 24220, 0x5ca21d71, F=0x0, S=1, > 1 > 0, 244800, 252000, 3600, 1422, 0xcde6cc34, F=0x0, S=1, > 1 > 0, 248400, 248400, 3600, 883, 0xedacbe25, F=0x0, S=1, > 1 > 0, 252000, 255600, 3600, 768, 0x89d774bc, F=0x0, S=1, > 1 > @@ -97,7 +97,7 @@ > 0, 316800, 324000, 3600, 1501, 0xb3b8f001, F=0x0, S=1, > 1 > 0, 320400, 320400, 3600, 941, 0x92b0cb18, F=0x0, S=1, > 1 > 0, 324000, 327600, 3600, 823, 0x3d548355, F=0x0, S=1, > 1 > -0, 327600, 345600, 3600, 24042, 0x441e94fb, S=1, 1 > +0, 327600, 345600, 3600, 24042, 0x441e94fb, F=0x0, S=1, > 1 > 0, 331200, 338400, 3600, 1582, 0x4f5d1049, F=0x0, S=1, > 1 > 0, 334800, 334800, 3600, 945, 0x4f3cc9e8, F=0x0, S=1, > 1 > 0, 338400, 342000, 3600, 815, 0x0ca790a4, F=0x0, S=1, > 1 > @@ -121,7 +121,7 @@ > 0, 403200, 410400, 3600, 359, 0x11bdae52, F=0x0, S=1, > 1 > 0, 406800, 406800, 3600, 235, 0xbec26964, F=0x0, S=1, > 1 > 0, 410400, 414000, 3600, 221, 0x8380682c, F=0x0, S=1, > 1 > -0, 414000, 432000, 3600, 22588, 0xf0ecf072, S=1, 1 > +0, 414000, 432000, 3600, 22588, 0xf0ecf072, F=0x0, S=1, > 1 > 0, 417600, 424800, 3600, 383, 0x4f3bb571, F=0x0, S=1, > 1 > 0, 421200, 421200, 3600, 257, 0x22e87802, F=0x0, S=1, > 1 > 0, 424800, 428400, 3600, 261, 0xdb988134, F=0x0, S=1, > 1 > diff --git a/tests/ref/lavf-fate/h264.mp4 b/tests/ref/lavf-fate/h264.mp4 > index a9c3823c2c..54d8c407d2 100644 > --- a/tests/ref/lavf-fate/h264.mp4 > +++ b/tests/ref/lavf-fate/h264.mp4 > @@ -1,3 +1,3 @@ > -fe299ea5205b71a48281f917b1256a5d *tests/data/lavf-fate/lavf.h264.mp4 > -547928 tests/data/lavf-fate/lavf.h264.mp4 > +badb54efedaf0c7f725158b85339a8f4 *tests/data/lavf-fate/lavf.h264.mp4 > +548177 tests/data/lavf-fate/lavf.h264.mp4 > tests/data/lavf-fate/lavf.h264.mp4 CRC=0x9da2c999 > _______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".