[FFmpeg-devel] [PATCH] libavformat: fix inputs initialization in mpegts muxer with filters

[Michael Niedermayer]

On Mon, Apr 22, 2019 at 11:42:57AM +0000, Andreas Håkon wrote:
> 
> ‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐
> On Friday, 19 de April de 2019 17:08, Michael Niedermayer <michael at niedermayer.cc> wrote:
> 
> > On Fri, Apr 19, 2019 at 08:23:35AM +0000, Andreas Håkon via ffmpeg-devel wrote:
> >
> > > From 936740731c17a9757aa093bdb35d9772df1e64a8 Mon Sep 17 00:00:00 2001
> > > From: Andreas Hakon andreas.hakon at protonmail.com
> > > Date: Fri, 19 Apr 2019 09:17:32 +0100
> > > Subject: [PATCH] libavformat: input init mpegts with filters v2
> > > This patch solves the initialization of the inputs when using filters
> > > (a graph filter) with the mpegts muxer.
> > > This bug seems to be generated by a simple forgetting to copy. The same code is
> > > repeated two times, but only in one case the variable inputs_done is initialized.
> >
> > > Testcase:
> > > $ ffmpeg -f mpegts -i mpeg.ts \
> > > -filter_complex "[i:100]fps=fps=25[out]" \
> > > -map "[out]" -c:v libx264 -f mpegts out.ts
> >
> > i see no difference in the output (same md5)
> > with what input file does this show a difference?
> 
> 
> Hi Michael,
> 
> Let me to explain the problem inside the code. Here the relevant part of the
> file "/fftools/ffmpeg.c" (line #4606):
> 
> ---------------------8<----------------------
>     if (ost->filter && ost->filter->graph->graph) {
>         if (!ost->initialized) {
>             char error[1024] = {0};
>             ret = init_output_stream(ost, error, sizeof(error));
>             if (ret < 0) {
>                 av_log(NULL, AV_LOG_ERROR, "Error initializing output stream %d:%d -- %s\n",
>                        ost->file_index, ost->index, error);
>                 exit_program(1);
>             }
>         }
>         if ((ret = transcode_from_filter(ost->filter->graph, &ist)) < 0)
>             return ret;
>         if (!ist)
>             return 0;
>     } else if (ost->filter) {
>         int i;
>         for (i = 0; i < ost->filter->graph->nb_inputs; i++) {
>             InputFilter *ifilter = ost->filter->graph->inputs[i];
>             if (!ifilter->ist->got_output && !input_files[ifilter->ist->file_index]->eof_reached) {
>                 ist = ifilter->ist;
>                 break;
>             }
>         }
>         if (!ist) {
>             ost->inputs_done = 1;
>             return 0;
>         }
>     } else {
>         av_assert0(ost->source_index >= 0);
>         ist = input_streams[ost->source_index];
>     }
> ---------------------8<----------------------
> 
> Please, note the first block of the "if (ost->filter && ost->filter->graph->graph)".
> This block normally returns with "if (!ist) return 0;".
> 
> But the second block (the one "else if (ost->filter)") returns with
> "if (!ist) {ost->inputs_done = 1; return 0;"
> 
> What the second block likes to protect is when the input stream is completed.
> However, the first block doesn't do it!
> 
> One difference is that the first block uses the transcode_from_filter() function.
> But that not makes difference, as the second block iterates over the inputs, and
> the function transcode_from_filter() does it too.
> 
> Futhermore, the "ost->inputs_done" is used only in the function "choose_output()"
> (at line #3882 of /fftools/ffmpeg.c):
> 
> ---------------------8<----------------------
> static OutputStream *choose_output(void)
> {
>     int i;
>     int64_t opts_min = INT64_MAX;
>     OutputStream *ost_min = NULL;
> 
>     for (i = 0; i < nb_output_streams; i++) {
>         OutputStream *ost = output_streams[i];
>         int64_t opts = ost->st->cur_dts == AV_NOPTS_VALUE ? INT64_MIN :
>                        av_rescale_q(ost->st->cur_dts, ost->st->time_base,
>                                     AV_TIME_BASE_Q);
>         if (ost->st->cur_dts == AV_NOPTS_VALUE)
>             av_log(NULL, AV_LOG_DEBUG,
>                 "cur_dts is invalid st:%d (%d) [init:%d i_done:%d finish:%d] (this is harmless if it occurs once at the start per stream)\n",
>                 ost->st->index, ost->st->id, ost->initialized, ost->inputs_done, ost->finished);
> 
>         if (!ost->initialized && !ost->inputs_done)
>             return ost;
> 
>         if (!ost->finished && opts < opts_min) {
>             opts_min = opts;
>             ost_min  = ost->unavailable ? NULL : ost;
>         }
>     }
>     return ost_min;
> }
> ------------8<----------------------
> 
> So, the case with troubles is when the output stream is selected because
> is not initilized and no inputs are completed. And without this patch
> it's possible that inputs can be completed but not marked as done.
> 
> I assume that the error is a simple incorrect "copy&paste" of the original
> code, as the second block sets the "ost->inputs_done = 1;" but not the
> first one. And the difference between both blocks is that the first
> is when a filter graph is running.


> 
> Please, revise the code! I hope you understand it when you look at my
> descriptions.

I would prefer to have some testcase that shows a problem
what you write basically sounds like
there are 2 similar pieces of code but they differ, so you change
one assuming the other is correct. Maybe the change is correct but
the explanation is insufficient

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

When you are offended at any man's fault, turn to yourself and study your
own failings. Then you will forget your anger. -- Epictetus
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 181 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20190423/0274b3dc/attachment.sig>