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.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-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".

Reply via email to