[FFmpeg-devel] [PATCH 3/3] avcodec/h264_parser: use the keyframe heuristics flag
Andreas Rheinhardt
andreas.rheinhardt at outlook.com
Sun Jul 18 03:30:48 EEST 2021
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 <jamrial at 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
>
More information about the ffmpeg-devel
mailing list