| Message ID | rGeDEUUXca51Mu71tqKzyOlNCpotnJMf7MP5K0_0xWwdeWevofS_z8ItbR5RpyoPRa-p_bZ0uAhABRFYztuTfESMr6a5zqIX3jrHIrw00bk=@protonmail.com |
|---|---|
| State | New |
| Headers | show |
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? [...]
‐‐‐‐‐‐‐ 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. ---
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 [...]
‐‐‐‐‐‐‐ 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. ---
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 [...]
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 [...]
‐‐‐‐‐‐‐ 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. ---
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++) {