| 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 | expand |
| 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 |
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
Jan Ekström: > 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. > Applied with these changes. - Andreas
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;
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(-)