[FFmpeg-devel] [PATCH 3/3] avcodec/h264_parser: use the keyframe heuristics flag
James Almer
jamrial at gmail.com
Sun Jul 18 03:55:30 EEST 2021
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 <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.
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.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?
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.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 at ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request at ffmpeg.org with subject "unsubscribe".
>
More information about the ffmpeg-devel
mailing list