diff mbox series

[FFmpeg-devel,3/3] avcodec/h264_parser: use the keyframe heuristics flag

Message ID 20210714143401.890-3-jamrial@gmail.com
State New
Headers show
Series [FFmpeg-devel,1/3] avcodec: add a parser flag to enable keyframe tagging heuristics
Related show

Checks

Context Check Description
andriy/x86_make success Make finished
andriy/x86_make_fate success Make fate finished
andriy/PPC64_make success Make finished
andriy/PPC64_make_fate success Make fate finished

Commit Message

James Almer July 14, 2021, 2:34 p.m. UTC
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.

Signed-off-by: James Almer <jamrial@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(-)

Comments

Andreas Rheinhardt July 18, 2021, 12:30 a.m. UTC | #1
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@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
>
James Almer July 18, 2021, 12:55 a.m. UTC | #2
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@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@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
>
Andreas Rheinhardt July 18, 2021, 2:19 a.m. UTC | #3
James Almer:
> 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.
> 

It's not taken verbatim -- Matroska doesn't have an analog to the sync
samples table; actually, it is not even guaranteed that cues always
reference keyframes (MKVToolNix even allows to reference all blocks!),
although (lacking an alternative) demuxers operate under this assumption.

>>
>>> Signed-off-by: James Almer <jamrial@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.
> 

So the mp4 muxer would create invalid files by default and in case this
flag is set, it instead creates crippled files (where open gop random
access points are not marked as such)? Does not sound good to me.
An actual fix is to make the muxer aware of the difference between IDR
and ordinary keyframes (there is already a MOV_PARTIAL_SYNC_SAMPLE for
the latter; and there is already some parsing in case of MPEG-2).

(As much as I like to reuse the parsers for this, I don't really know
what exactly the parser should additionally return. Should this be
solved, we might need to add another field to AVPacket (given that the
new parsing API is supposed to be packet-based, said information should
be stored there; alternatively, one could also use more of AV_PKT_FLAG_*).
We do not even have to wait for the new parser API to get this
information out of the parser: We can just add new fields to
AVCodecParserContext; but we need to agree on whether this information
should be part of AVPacket and if so, how.)

- Andreas
James Almer July 18, 2021, 3:01 a.m. UTC | #4
On 7/17/2021 11:19 PM, Andreas Rheinhardt wrote:
> James Almer:
>> 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.
>>
> 
> It's not taken verbatim -- Matroska doesn't have an analog to the sync
> samples table; actually, it is not even guaranteed that cues always
> reference keyframes (MKVToolNix even allows to reference all blocks!),
> although (lacking an alternative) demuxers operate under this assumption.
> 
>>>
>>>> Signed-off-by: James Almer <jamrial@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.
>>
> 
> So the mp4 muxer would create invalid files by default and in case this
> flag is set, it instead creates crippled files (where open gop random
> access points are not marked as such)? Does not sound good to me.
> An actual fix is to make the muxer aware of the difference between IDR
> and ordinary keyframes (there is already a MOV_PARTIAL_SYNC_SAMPLE for
> the latter; and there is already some parsing in case of MPEG-2).

The current code uses this flag to write stps atoms, and at least for 
h264 that's not apparently what should be done (We need roll sample 
groups instead).

> 
> (As much as I like to reuse the parsers for this, I don't really know
> what exactly the parser should additionally return. Should this be
> solved, we might need to add another field to AVPacket (given that the
> new parsing API is supposed to be packet-based, said information should
> be stored there; alternatively, one could also use more of AV_PKT_FLAG_*).

A packet flag would work to signal that the packet contains an ordinary 
key frame, but not to propagate values like recovery_frame_cnt (to write 
roll distance). That would probably require a side data struct.

> We do not even have to wait for the new parser API to get this
> information out of the parser: We can just add new fields to
> AVCodecParserContext; but we need to agree on whether this information
> should be part of AVPacket and if so, how.)

It would need to be part of AVPacket if we do it as something 
AVCodecParsers and demuxers can output and propagate. Even a bsfs 
inserted by the muxer would require whatever we get out of it to be 
attached to the packet. But we could maybe avoid this with a new bsfs 
based parser API where for example reduced scope CBS-style 
codec-specific structs could be offered, and used by muxers in a similar 
way some of them are currently using bsfs.

> 
> - Andreas
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
>
diff mbox series

Patch

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;
+                }
             }
             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"
 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
 #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