diff mbox series

[FFmpeg-devel,17/17] fftools/ffmpeg_opt: Apply copyinkf for all stream types

Message ID AM7PR03MB666077272D227673728A7D488F929@AM7PR03MB6660.eurprd03.prod.outlook.com
State New
Headers show
Series [FFmpeg-devel,01/17] avformat/mxfenc: Auto-insert h264_mp4toannexb BSF if needed
Related show

Checks

Context Check Description
andriy/make_x86 success Make finished
andriy/make_fate_x86 success Make fate finished
andriy/make_ppc success Make finished
andriy/make_fate_ppc success Make fate finished

Commit Message

Andreas Rheinhardt Nov. 9, 2021, 6:01 p.m. UTC
The earlier code has ignored it for all stream types except
video and subtitles, probably because audio was presumed
to only consist of keyframes. Yet this assumption is not true
for e.g. TrueHD.

Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
---
 fftools/ffmpeg_opt.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

Comments

Jan Ekström Nov. 10, 2021, 11:14 a.m. UTC | #1
On Tue, Nov 9, 2021 at 8:05 PM Andreas Rheinhardt
<andreas.rheinhardt@outlook.com> wrote:
>
> The earlier code has ignored it for all stream types except
> video and subtitles, probably because audio was presumed
> to only consist of keyframes. Yet this assumption is not true
> for e.g. TrueHD.
>
> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
> ---
>  fftools/ffmpeg_opt.c | 7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)
>
> diff --git a/fftools/ffmpeg_opt.c b/fftools/ffmpeg_opt.c
> index ab4c63a362..60ee6b16b5 100644
> --- a/fftools/ffmpeg_opt.c
> +++ b/fftools/ffmpeg_opt.c
> @@ -1655,6 +1655,9 @@ static OutputStream *new_output_stream(OptionsContext *o, AVFormatContext *oc, e
>      ost->muxing_queue = av_fifo_alloc(8 * sizeof(AVPacket));
>      if (!ost->muxing_queue)
>          exit_program(1);
> +    if (ost->stream_copy)
> +        MATCH_PER_STREAM_OPT(copy_initial_nonkeyframes, i,
> +                             ost->copy_initial_nonkeyframes, oc, st);

Could use an extra empty line after the previous if that leads to
exit_program since the two are not related/grouped.

I think we can follow the way it was done for subtitle streams
previously, and just skip the ost->stream_copy check? There is no
extra initialization done for this logic, and having the correct value
always set in "ost" should not be a bad thing (unless I've missed
something with my quick check-around)

Otherwise LGTM for me.

Jan
diff mbox series

Patch

diff --git a/fftools/ffmpeg_opt.c b/fftools/ffmpeg_opt.c
index ab4c63a362..60ee6b16b5 100644
--- a/fftools/ffmpeg_opt.c
+++ b/fftools/ffmpeg_opt.c
@@ -1655,6 +1655,9 @@  static OutputStream *new_output_stream(OptionsContext *o, AVFormatContext *oc, e
     ost->muxing_queue = av_fifo_alloc(8 * sizeof(AVPacket));
     if (!ost->muxing_queue)
         exit_program(1);
+    if (ost->stream_copy)
+        MATCH_PER_STREAM_OPT(copy_initial_nonkeyframes, i,
+                             ost->copy_initial_nonkeyframes, oc, st);
 
     return ost;
 }
@@ -1940,8 +1943,6 @@  static OutputStream *new_video_stream(OptionsContext *o, AVFormatContext *oc, in
         ost->avfilter = get_ost_filters(o, oc, ost);
         if (!ost->avfilter)
             exit_program(1);
-    } else {
-        MATCH_PER_STREAM_OPT(copy_initial_nonkeyframes, i, ost->copy_initial_nonkeyframes, oc ,st);
     }
 
     if (ost->stream_copy)
@@ -2069,8 +2070,6 @@  static OutputStream *new_subtitle_stream(OptionsContext *o, AVFormatContext *oc,
 
     subtitle_enc->codec_type = AVMEDIA_TYPE_SUBTITLE;
 
-    MATCH_PER_STREAM_OPT(copy_initial_nonkeyframes, i, ost->copy_initial_nonkeyframes, oc, st);
-
     if (!ost->stream_copy) {
         char *frame_size = NULL;