| Message ID | 20170717141926.7442-1-george@nsup.org |
|---|---|
| State | Accepted |
| Commit | 1daacba91f7c8a29858fb2de58f8695f33308fa7 |
| Headers | show |
On 7/17/2017 11:19 AM, Nicolas George wrote: > This reverts commit 04aa09c4bcf2d5a634a35da3a3ae3fc1abe30ef8. You could mention this is also reverting e5bce8b4ce7b1f3a83998febdfa86a3771df96ce. > > The fate-ffm change is caused by field_order now being set > on the output format because the first frame arrives earlier. > The fate-mxf change is assumed to be the same. > > Signed-off-by: Nicolas George <george@nsup.org> > --- > libavfilter/buffersrc.c | 25 +++++++++++++++++++++++++ > tests/ref/lavf/ffm | 2 +- > tests/ref/lavf/mxf | 6 +++--- > 3 files changed, 29 insertions(+), 4 deletions(-) > > > The field_order info seems to not be printed by any tool, and I do not know > the MXF format to check directly in the file; with FFM it was easy to see in > a hexdump. Anyway, the fact that field order was not set before de-reverting > hints at something fishy happening in the initialization code of ffmpeg. I > do not have time nor motivation to investigate that, so I will assume the > change is for the best like FFM. > > If someone wants to investigate, the change can be toggled by simply adding > "return 0" at the beginning of push_frame(), below. > > > diff --git a/libavfilter/buffersrc.c b/libavfilter/buffersrc.c > index 587b29b91a..e8f59c2de7 100644 > --- a/libavfilter/buffersrc.c > +++ b/libavfilter/buffersrc.c > @@ -173,6 +173,20 @@ int attribute_align_arg av_buffersrc_add_frame_flags(AVFilterContext *ctx, AVFra > return ret; > } > > +static int push_frame(AVFilterGraph *graph) > +{ > + int ret; > + > + while (1) { > + ret = ff_filter_graph_run_once(graph); > + if (ret == AVERROR(EAGAIN)) > + break; > + if (ret < 0) > + return ret; > + } > + return 0; > +} > + > static int av_buffersrc_add_frame_internal(AVFilterContext *ctx, > AVFrame *frame, int flags) > { > @@ -185,6 +199,11 @@ static int av_buffersrc_add_frame_internal(AVFilterContext *ctx, > if (!frame) { > s->eof = 1; > ff_avfilter_link_set_in_status(ctx->outputs[0], AVERROR_EOF, AV_NOPTS_VALUE); > + if ((flags & AV_BUFFERSRC_FLAG_PUSH)) { > + ret = push_frame(ctx->graph); > + if (ret < 0) > + return ret; > + } > return 0; > } else if (s->eof) > return AVERROR(EINVAL); > @@ -239,6 +258,12 @@ static int av_buffersrc_add_frame_internal(AVFilterContext *ctx, > if ((ret = ctx->output_pads[0].request_frame(ctx->outputs[0])) < 0) > return ret; > > + if ((flags & AV_BUFFERSRC_FLAG_PUSH)) { > + ret = push_frame(ctx->graph); > + if (ret < 0) > + return ret; > + } > + > return 0; > } > > diff --git a/tests/ref/lavf/ffm b/tests/ref/lavf/ffm > index 54c56034aa..d9fa8d52cb 100644 > --- a/tests/ref/lavf/ffm > +++ b/tests/ref/lavf/ffm > @@ -1,3 +1,3 @@ > -a0e9616f0d9a8c1029f3220b1b9175f4 *./tests/data/lavf/lavf.ffm > +ca2a450cd0d1e299514a345923b4c82a *./tests/data/lavf/lavf.ffm > 376832 ./tests/data/lavf/lavf.ffm > ./tests/data/lavf/lavf.ffm CRC=0x000e23ae > diff --git a/tests/ref/lavf/mxf b/tests/ref/lavf/mxf > index 9ab4432c63..48fe95a235 100644 > --- a/tests/ref/lavf/mxf > +++ b/tests/ref/lavf/mxf > @@ -1,9 +1,9 @@ > -dbdbb7d8677dc29b0d90eedcf418ce13 *./tests/data/lavf/lavf.mxf > +eaac3125ac1a61fe5f968c7af83fa71e *./tests/data/lavf/lavf.mxf > 525369 ./tests/data/lavf/lavf.mxf > ./tests/data/lavf/lavf.mxf CRC=0x8dddfaab > -40fcb0a898f8825a17f5754b23762f49 *./tests/data/lavf/lavf.mxf > +1562530330b13e9e70f522fe20265632 *./tests/data/lavf/lavf.mxf > 560697 ./tests/data/lavf/lavf.mxf > ./tests/data/lavf/lavf.mxf CRC=0xf21b1b48 > -9233d192af20fc2a89304f5ae93c21ee *./tests/data/lavf/lavf.mxf > +e07858715997313ae66a1cdd6fde5f66 *./tests/data/lavf/lavf.mxf > 525369 ./tests/data/lavf/lavf.mxf > ./tests/data/lavf/lavf.mxf CRC=0x8dddfaab >
Le nonidi 29 messidor, an CCXXV, James Almer a écrit : > You could mention this is also reverting > e5bce8b4ce7b1f3a83998febdfa86a3771df96ce. I had not noticed that the FATE changes were introduced in a separate commit. I added this: It also reverts e5bce8b4ce7b1f3a83998febdfa86a3771df96ce that fixed FATE refs. to the commit message, at the beginning of the second paragraph. Thanks for noticing. Regards,
On 17.07.2017 16:19, Nicolas George wrote: > This reverts commit 04aa09c4bcf2d5a634a35da3a3ae3fc1abe30ef8. > > The fate-ffm change is caused by field_order now being set > on the output format because the first frame arrives earlier. > The fate-mxf change is assumed to be the same. > > Signed-off-by: Nicolas George <george@nsup.org> > --- > libavfilter/buffersrc.c | 25 +++++++++++++++++++++++++ > tests/ref/lavf/ffm | 2 +- > tests/ref/lavf/mxf | 6 +++--- > 3 files changed, 29 insertions(+), 4 deletions(-) > > > The field_order info seems to not be printed by any tool, and I do not know > the MXF format to check directly in the file; with FFM it was easy to see in > a hexdump. Anyway, the fact that field order was not set before de-reverting > hints at something fishy happening in the initialization code of ffmpeg. I > do not have time nor motivation to investigate that, so I will assume the > change is for the best like FFM. ffprobe should be able to print the stream's field_order since commit 54350f06e11727f255e3d1829cb1afde49931d8b > > If someone wants to investigate, the change can be toggled by simply adding > "return 0" at the beginning of push_frame(), below. > > [...] Regards, Tobias
On 7/17/2017 3:19 PM, Nicolas George wrote: > This reverts commit 04aa09c4bcf2d5a634a35da3a3ae3fc1abe30ef8. > > The fate-ffm change is caused by field_order now being set > on the output format because the first frame arrives earlier. > The fate-mxf change is assumed to be the same. > > Signed-off-by: Nicolas George <george@nsup.org> > --- > libavfilter/buffersrc.c | 25 +++++++++++++++++++++++++ > tests/ref/lavf/ffm | 2 +- > tests/ref/lavf/mxf | 6 +++--- > 3 files changed, 29 insertions(+), 4 deletions(-) Doesn't say /why/ it's being reverted. Please update the commit message to do so. - Derek
Le decadi 30 messidor, an CCXXV, Derek Buitenhuis a écrit : > Doesn't say /why/ it's being reverted. Please update > the commit message to do so. The reasons are exactly the same as the commit that it restores. I will include that and the other comments you request once I can access my work tree comfortably. Regards,
On 7/18/2017 3:48 PM, Nicolas George wrote: > The reasons are exactly the same as the commit that it restores. I will > include that and the other comments you request once I can access my > work tree comfortably. OK. - Derek
On 17.07.2017 16:19, Nicolas George wrote: > This reverts commit 04aa09c4bcf2d5a634a35da3a3ae3fc1abe30ef8. > > The fate-ffm change is caused by field_order now being set > on the output format because the first frame arrives earlier. > The fate-mxf change is assumed to be the same. > > Signed-off-by: Nicolas George <george@nsup.org> > --- > libavfilter/buffersrc.c | 25 +++++++++++++++++++++++++ > tests/ref/lavf/ffm | 2 +- > tests/ref/lavf/mxf | 6 +++--- > 3 files changed, 29 insertions(+), 4 deletions(-) > > [...] This commit seems to break transcoding of some input files on machines with a lot of CPU cores. See attached script that reproduces the problem ("-threads 32" is used to simulate the situation of a multi-core CPU). Sorry for noticing late but it took me some time to track down the problem and git bisecting it. The issue seems to be triggered by the high number of audio packets in the input file (~375 packets per second). Input files that have a lower audio packet rate work fine. Regards, Tobias #!/bin/bash # ========================================================================= # Configuration BASE_PATH="./debug" INPUT_FILE="$BASE_PATH/ffmpeg-test-input.avi" OUTPUT_FILE="$BASE_PATH/ffmpeg-test-output.avi" TEMP_FILE="$BASE_PATH/ffmpeg-test-temp.wav" FFMPEG_PATH="." FFMPEG_BIN="$FFMPEG_PATH/build-linux/ffmpeg-dbg" # ========================================================================= # Create Input Files mkdir -p "$BASE_PATH" $FFMPEG_BIN \ -flags +bitexact -sws_flags +accurate_rnd+bitexact -fflags +bitexact \ -f lavfi -graph "aevalsrc=sin(440*2*PI*t):sample_rate=48000:channel_layout=7.1" -i dummy1 \ -f wav -codec:a pcm_f32le -t 30.0 -y "$TEMP_FILE" $FFMPEG_BIN \ -flags +bitexact -sws_flags +accurate_rnd+bitexact -fflags +bitexact \ -f lavfi -graph "testsrc2=size=720x576:rate=25:sar=16/15,noise=alls=40:allf=t+u" -i dummy1 \ -i "$TEMP_FILE" \ -f avi -codec:v ffvhuff -pix_fmt yuv422p -codec:a pcm_s24le -t 30.0 -y "$INPUT_FILE" # ========================================================================= # Create Output File $FFMPEG_BIN \ -flags +bitexact -sws_flags +accurate_rnd+bitexact -fflags +bitexact \ -threads 32 -probesize 50M -ss 3.0 -i "$INPUT_FILE" \ -f avi -map 0:v -map 0:a -codec:v ffvhuff -pix_fmt yuv422p \ -codec:a pcm_s24le -loglevel repeat+verbose -xerror \ -y "$OUTPUT_FILE" if [ $? -eq 0 ]; then echo "=== TEST SUCCEEDED ==="; else echo "=== TEST FAILED ==="; fi
Le sextidi 26 vendémiaire, an CCXXVI, Tobias Rapp a écrit : > This commit seems to break transcoding of some input files on machines with > a lot of CPU cores. See attached script that reproduces the problem > ("-threads 32" is used to simulate the situation of a multi-core CPU). > > Sorry for noticing late but it took me some time to track down the problem > and git bisecting it. The issue seems to be triggered by the high number of > audio packets in the input file (~375 packets per second). Input files that > have a lower audio packet rate work fine. Thanks for reporting. I fear this will be tricky to debug. I think the change you have tracked cannot be the cause of the issue, since it does not touch anything related to threading. What this change does, on the other hand, is increase the efficiency of the scheduling in lavfi. That can cause more work for filters that do use threading, and reveal a race condition there. I do not have access to a 32-core system. Can the problem be reproduced with your script with just "-threads 32" without such a system? How ofter does it manifest? Regards,
On 17.10.2017 20:12, Nicolas George wrote: > Le sextidi 26 vendémiaire, an CCXXVI, Tobias Rapp a écrit : >> This commit seems to break transcoding of some input files on machines with >> a lot of CPU cores. See attached script that reproduces the problem >> ("-threads 32" is used to simulate the situation of a multi-core CPU). >> >> Sorry for noticing late but it took me some time to track down the problem >> and git bisecting it. The issue seems to be triggered by the high number of >> audio packets in the input file (~375 packets per second). Input files that >> have a lower audio packet rate work fine. > > Thanks for reporting. I fear this will be tricky to debug. > > I think the change you have tracked cannot be the cause of the issue, > since it does not touch anything related to threading. Yes, I also don't see why this patch correlates with the number of threads. But when bypassing the push_frame() function from the patch by adding an immediate "return 0" the problem disappears. > What this change does, on the other hand, is increase the efficiency of > the scheduling in lavfi. That can cause more work for filters that do > use threading, and reveal a race condition there. > > I do not have access to a 32-core system. Can the problem be reproduced > with your script with just "-threads 32" without such a system? How > ofter does it manifest? I'm able to reproduce the issue reliably on each run of the test script on three test machines: - Windows 64bit 32-core, without "-threads 32" - Windows 64bit 2-core, with "-threads 32" - GNU/Linux 64bit 2-core, with "-threads 32" What seem to help as a work-around is adding "-max_muxing_queue_size 1000". Regards, Tobias
diff --git a/libavfilter/buffersrc.c b/libavfilter/buffersrc.c index 587b29b91a..e8f59c2de7 100644 --- a/libavfilter/buffersrc.c +++ b/libavfilter/buffersrc.c @@ -173,6 +173,20 @@ int attribute_align_arg av_buffersrc_add_frame_flags(AVFilterContext *ctx, AVFra return ret; } +static int push_frame(AVFilterGraph *graph) +{ + int ret; + + while (1) { + ret = ff_filter_graph_run_once(graph); + if (ret == AVERROR(EAGAIN)) + break; + if (ret < 0) + return ret; + } + return 0; +} + static int av_buffersrc_add_frame_internal(AVFilterContext *ctx, AVFrame *frame, int flags) { @@ -185,6 +199,11 @@ static int av_buffersrc_add_frame_internal(AVFilterContext *ctx, if (!frame) { s->eof = 1; ff_avfilter_link_set_in_status(ctx->outputs[0], AVERROR_EOF, AV_NOPTS_VALUE); + if ((flags & AV_BUFFERSRC_FLAG_PUSH)) { + ret = push_frame(ctx->graph); + if (ret < 0) + return ret; + } return 0; } else if (s->eof) return AVERROR(EINVAL); @@ -239,6 +258,12 @@ static int av_buffersrc_add_frame_internal(AVFilterContext *ctx, if ((ret = ctx->output_pads[0].request_frame(ctx->outputs[0])) < 0) return ret; + if ((flags & AV_BUFFERSRC_FLAG_PUSH)) { + ret = push_frame(ctx->graph); + if (ret < 0) + return ret; + } + return 0; } diff --git a/tests/ref/lavf/ffm b/tests/ref/lavf/ffm index 54c56034aa..d9fa8d52cb 100644 --- a/tests/ref/lavf/ffm +++ b/tests/ref/lavf/ffm @@ -1,3 +1,3 @@ -a0e9616f0d9a8c1029f3220b1b9175f4 *./tests/data/lavf/lavf.ffm +ca2a450cd0d1e299514a345923b4c82a *./tests/data/lavf/lavf.ffm 376832 ./tests/data/lavf/lavf.ffm ./tests/data/lavf/lavf.ffm CRC=0x000e23ae diff --git a/tests/ref/lavf/mxf b/tests/ref/lavf/mxf index 9ab4432c63..48fe95a235 100644 --- a/tests/ref/lavf/mxf +++ b/tests/ref/lavf/mxf @@ -1,9 +1,9 @@ -dbdbb7d8677dc29b0d90eedcf418ce13 *./tests/data/lavf/lavf.mxf +eaac3125ac1a61fe5f968c7af83fa71e *./tests/data/lavf/lavf.mxf 525369 ./tests/data/lavf/lavf.mxf ./tests/data/lavf/lavf.mxf CRC=0x8dddfaab -40fcb0a898f8825a17f5754b23762f49 *./tests/data/lavf/lavf.mxf +1562530330b13e9e70f522fe20265632 *./tests/data/lavf/lavf.mxf 560697 ./tests/data/lavf/lavf.mxf ./tests/data/lavf/lavf.mxf CRC=0xf21b1b48 -9233d192af20fc2a89304f5ae93c21ee *./tests/data/lavf/lavf.mxf +e07858715997313ae66a1cdd6fde5f66 *./tests/data/lavf/lavf.mxf 525369 ./tests/data/lavf/lavf.mxf ./tests/data/lavf/lavf.mxf CRC=0x8dddfaab
This reverts commit 04aa09c4bcf2d5a634a35da3a3ae3fc1abe30ef8. The fate-ffm change is caused by field_order now being set on the output format because the first frame arrives earlier. The fate-mxf change is assumed to be the same. Signed-off-by: Nicolas George <george@nsup.org> --- libavfilter/buffersrc.c | 25 +++++++++++++++++++++++++ tests/ref/lavf/ffm | 2 +- tests/ref/lavf/mxf | 6 +++--- 3 files changed, 29 insertions(+), 4 deletions(-) The field_order info seems to not be printed by any tool, and I do not know the MXF format to check directly in the file; with FFM it was easy to see in a hexdump. Anyway, the fact that field order was not set before de-reverting hints at something fishy happening in the initialization code of ffmpeg. I do not have time nor motivation to investigate that, so I will assume the change is for the best like FFM. If someone wants to investigate, the change can be toggled by simply adding "return 0" at the beginning of push_frame(), below.