On 7/17/2021 9:30 PM, Andreas Rheinhardt wrote:
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.
It was an assumption considering the Matroska h264 encapsulation is taken verbatim from ISOBMFF. But you're right, MKVToolnix does mark those as key.
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.
I worked around this for the mpegts muxer as Kieran requested, but if Matroska is the same then the situation changes.
I can add a user settable demuxer option for the autoinserted parser instead of hardcoding the behavior, and leave the current behavior as the default.
(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.shindex 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-apngfate-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?
When i tested i recall it was needed because one command in lavf_container_fate() remuxes the input file to create the output, and the second remuxed the newly created output with the framecrc pseudo muxer.
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.makindex 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.matroskaThe 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".
_______________________________________________ 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".
