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

Submitted by Oliver Collyer via ffmpeg-devel on April 19, 2019, 8:23 a.m.

Details

Message ID rGeDEUUXca51Mu71tqKzyOlNCpotnJMf7MP5K0_0xWwdeWevofS_z8ItbR5RpyoPRa-p_bZ0uAhABRFYztuTfESMr6a5zqIX3jrHIrw00bk=@protonmail.com
State New
Headers show

Commit Message

Oliver Collyer via ffmpeg-devel April 19, 2019, 8:23 a.m.
‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐
On Thursday, 18 de April de 2019 22:07, Michael Niedermayer <michael@niedermayer.cc> wrote:

>
> fails to apply cleanly
>
> Applying: libavformat: fix inputs initialization in mpegts muxer with filters
> Using index info to reconstruct a base tree...
> error: patch failed: fftools/ffmpeg.c:4613
> error: fftools/ffmpeg.c: patch does not apply
> error: Did you hand edit your patch?
> It does not apply to blobs recorded in its index.
> Patch failed at 0001 libavformat: fix inputs initialization in mpegts muxer with filters
>

Sorry Michael another time!

Here a new version from a clean copy.

Regards.
A.H.


---
From 936740731c17a9757aa093bdb35d9772df1e64a8 Mon Sep 17 00:00:00 2001
From: Andreas Hakon <andreas.hakon@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

Signed-off-by: Andreas Hakon <andreas.hakon@protonmail.com>
---
 fftools/ffmpeg.c |    4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

Comments

Michael Niedermayer April 19, 2019, 3:08 p.m.
On Fri, Apr 19, 2019 at 08:23:35AM +0000, Andreas Håkon via ffmpeg-devel wrote:
> 
> ‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐
> On Thursday, 18 de April de 2019 22:07, Michael Niedermayer <michael@niedermayer.cc> wrote:
> 
> >
> > fails to apply cleanly
> >
> > Applying: libavformat: fix inputs initialization in mpegts muxer with filters
> > Using index info to reconstruct a base tree...
> > error: patch failed: fftools/ffmpeg.c:4613
> > error: fftools/ffmpeg.c: patch does not apply
> > error: Did you hand edit your patch?
> > It does not apply to blobs recorded in its index.
> > Patch failed at 0001 libavformat: fix inputs initialization in mpegts muxer with filters
> >
> 
> Sorry Michael another time!
> 
> Here a new version from a clean copy.
> 
> Regards.
> A.H.
> 
> 
> ---

> From 936740731c17a9757aa093bdb35d9772df1e64a8 Mon Sep 17 00:00:00 2001
> From: Andreas Hakon <andreas.hakon@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?

[...]
Andreas Håkon April 22, 2019, 11:42 a.m.
‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐
On Friday, 19 de April de 2019 17:08, Michael Niedermayer <michael@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@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.

Regards,
A.H.

---
Michael Niedermayer April 22, 2019, 11:07 p.m.
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@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@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

[...]
Andreas Håkon April 23, 2019, 7:16 a.m.
‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐
On Tuesday, 23 de April de 2019 1:07, Michael Niedermayer <michael@niedermayer.cc> wrote:

> On Mon, Apr 22, 2019 at 11:42:57AM +0000, Andreas Håkon wrote:
>
>
> > 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
>

Hi Michael,

I agree that a testcase is preferable. However, I can use the Mathematical Demonstration
by reduction to the absurd:

- The only difference between block A and B (the two "if () ... else if () ...") is that the
first one is executed when a filter graph is running.
- In the second block (block B) the "ost->inputs_done = 1;" is executed prior to return.
- In the first block (bloc A) is not executed.

Why is then executed in the Block B?

If the only difference is the filter graph, and nothing related to inputs, then only one
of these two can be true:

1) Or the Block B doesn't needs the code "ost->inputs_done = 1;"
2) Or the Block A needs it too.

And I don't think the option 1 is true.

So by reduction to the absurd is demonstrated.
Regards.
A.H.

---
Michael Niedermayer April 23, 2019, 9:55 p.m.
On Tue, Apr 23, 2019 at 07:16:07AM +0000, Andreas Håkon wrote:
> 
> ‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐
> On Tuesday, 23 de April de 2019 1:07, Michael Niedermayer <michael@niedermayer.cc> wrote:
> 
> > On Mon, Apr 22, 2019 at 11:42:57AM +0000, Andreas Håkon wrote:
> >
> >
> > > 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
> >
> 
> Hi Michael,
> 
> I agree that a testcase is preferable. However, I can use the Mathematical Demonstration
> by reduction to the absurd:
> 
> - The only difference between block A and B (the two "if () ... else if () ...") is that the
> first one is executed when a filter graph is running.
> - In the second block (block B) the "ost->inputs_done = 1;" is executed prior to return.
> - In the first block (bloc A) is not executed.
> 
> Why is then executed in the Block B?
> 
> If the only difference is the filter graph, and nothing related to inputs, then only one
> of these two can be true:
> 
> 1) Or the Block B doesn't needs the code "ost->inputs_done = 1;"
> 2) Or the Block A needs it too.
> 
> And I don't think the option 1 is true.
> 
> So by reduction to the absurd is demonstrated.
> Regards.

nothing of what you write here has any resemblance of a Mathematical proof,
not that it matters but you claimed that. 
A mathematical proof requires clear definitions and also requires the proposition
of what one proofs to be well existing at least.

I think a discussion of mathematical proofs will not help this matter 

I think a more accurate description of the situation is
1. we both see that there are 2 pieces of code that differ in a way that looks
   odd
2. we both do not know why.
3. you are convinced that remooving this difference in some random way that
   looks sensible is a good idea while i think its not a good idea to make
   such change without better understanding first.

thanks

[...]
Michael Niedermayer April 23, 2019, 10:58 p.m.
On Tue, Apr 23, 2019 at 11:55:59PM +0200, Michael Niedermayer wrote:
> On Tue, Apr 23, 2019 at 07:16:07AM +0000, Andreas Håkon wrote:
> > 
> > ‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐
> > On Tuesday, 23 de April de 2019 1:07, Michael Niedermayer <michael@niedermayer.cc> wrote:
> > 
> > > On Mon, Apr 22, 2019 at 11:42:57AM +0000, Andreas Håkon wrote:
> > >
> > >
> > > > 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
> > >
> > 
> > Hi Michael,
> > 
> > I agree that a testcase is preferable. However, I can use the Mathematical Demonstration
> > by reduction to the absurd:
> > 
> > - The only difference between block A and B (the two "if () ... else if () ...") is that the
> > first one is executed when a filter graph is running.
> > - In the second block (block B) the "ost->inputs_done = 1;" is executed prior to return.
> > - In the first block (bloc A) is not executed.
> > 
> > Why is then executed in the Block B?
> > 
> > If the only difference is the filter graph, and nothing related to inputs, then only one
> > of these two can be true:
> > 
> > 1) Or the Block B doesn't needs the code "ost->inputs_done = 1;"
> > 2) Or the Block A needs it too.
> > 
> > And I don't think the option 1 is true.
> > 
> > So by reduction to the absurd is demonstrated.
> > Regards.
> 
> nothing of what you write here has any resemblance of a Mathematical proof,
> not that it matters but you claimed that. 
> A mathematical proof requires clear definitions and also requires the proposition
> of what one proofs to be well existing at least.
> 
> I think a discussion of mathematical proofs will not help this matter 
> 
> I think a more accurate description of the situation is
> 1. we both see that there are 2 pieces of code that differ in a way that looks
>    odd
> 2. we both do not know why.
> 3. you are convinced that remooving this difference in some random way that
>    looks sensible is a good idea while i think its not a good idea to make
>    such change without better understanding first.

some analysis of the change of this patch:

The code part that is changed in the patch first checks

        if (!ost->initialized) {

and if true runs

            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);

init_output_stream() on failure exits the program, on success
it does 
      
      ost->initialized = 1;
      
Thus we know ost->initialized must be non zero after this
and there is no code that resets it to 0 that i can see

and the only use (read) case of ost->inputs_done is
      
      if (!ost->initialized && !ost->inputs_done)

so the code you want to add is dead code and will have no effect.

Thanks

[...]
Andreas Håkon April 24, 2019, 7:46 a.m.
‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐
On Wednesday, 24 de April de 2019 0:58, Michael Niedermayer <michael@niedermayer.cc> wrote:

> On Tue, Apr 23, 2019 at 11:55:59PM +0200, Michael Niedermayer wrote:
>
> > On Tue, Apr 23, 2019 at 07:16:07AM +0000, Andreas Håkon wrote:
> >
> > > ‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐
> > >
> > > 1.  Or the Block B doesn't needs the code "ost->inputs_done = 1;"
> > > 2.  Or the Block A needs it too.
> > >
> >
>
> some analysis of the change of this patch:
>
> ...
>
> so the code you want to add is dead code and will have no effect.
>
> Thanks
>

Hi Michael,

Good analysis! You're right and I'm wrong.
So, please close this patch. Not needed!

Regards.
A.H.


---

Patch hide | download patch | download mbox

diff --git a/fftools/ffmpeg.c b/fftools/ffmpeg.c
index 0f157d6..7399a19 100644
--- a/fftools/ffmpeg.c
+++ b/fftools/ffmpeg.c
@@ -4613,8 +4613,10 @@  static int transcode_step(void)
         }
         if ((ret = transcode_from_filter(ost->filter->graph, &ist)) < 0)
             return ret;
-        if (!ist)
+        if (!ist) {
+            ost->inputs_done = 1;
             return 0;
+        }
     } else if (ost->filter) {
         int i;
         for (i = 0; i < ost->filter->graph->nb_inputs; i++) {