| Message ID | 72a8bacf-861b-b349-fc77-55f63fceee86@aracnet.com |
|---|---|
| State | New |
| Headers | show |
On Fri, 12 May 2017 13:28:14 -0700 Aaron Levinson <alevinsn@aracnet.com> wrote: > Purpose: Made execution of reap_filters() more deterministic with > respect to when headers are written in relationship with the > initialization of output streams and the processing of input audio > and/or video frames. This change fixes a bug in ffmpeg encountered > when decoding interlaced video. In some cases, ffmpeg would treat it > as progressive. > > Detailed description of problem: Run the following command: "ffmpeg -i > test8_1080i.h264 -c:v mpeg2video test8_1080i_mp2.ts". In this case, > test8_1080i.h264 is an H.264-encoded file with 1080i59.94 > (interlaced). Prior to the patch, the following output is generated: > > Input #0, h264, from 'test8_1080i.h264': > Duration: N/A, bitrate: N/A > Stream #0:0: Video: h264 (High), yuv420p(top first), 1920x1080 [SAR 1:1 DAR 16:9], 29.97 fps, 29.97 tbr, 1200k tbn, 59.94 tbc > Stream mapping: > Stream #0:0 -> #0:0 (h264 (native) -> mpeg2video (native)) > Press [q] to stop, [?] for help > Output #0, mpegts, to 'test8_1080i_mp2_2.ts': > Metadata: > encoder : Lavf57.72.100 > Stream #0:0: Video: mpeg2video (Main), yuv420p, 1920x1080 [SAR 1:1 DAR 16:9], q=2-31, 200 kb/s, 29.97 fps, 90k tbn, 29.97 tbc > Metadata: > encoder : Lavc57.92.100 mpeg2video > > which demonstrates the bug. The output stream should instead look like: > > Stream #0:0: Video: mpeg2video (Main), yuv420p(top coded first (swapped)), 1920x1080 [SAR 1:1 DAR 16:9], q=2-31, 200 kb/s, 29.97 fps, 90k tbn, 29.97 tbc > > The bug is caused by the fact that reap_filters() calls > init_output_stream(), which is then immediately followed by a call to > check_init_output_file(), and this is all done prior to the first call > to do_video_out(). An initial call to do_video_out() is necessary to > populate the interlaced video information to the output stream's > codecpar (mux_par->field_order in do_video_out()). However, > check_init_output_file() calls avformat_write_header() prior to the > initial call to do_video_out(), so field_order is populated too late, > and the header is written with the default field_order value, > progressive. > > Signed-off-by: Aaron Levinson <alevinsn@aracnet.com> > --- > ffmpeg.c | 43 ++++++++++++++++++++++++++++++++++++++++--- > 1 file changed, 40 insertions(+), 3 deletions(-) > > diff --git a/ffmpeg.c b/ffmpeg.c > index 3cd45ba665..7b044b068c 100644 > --- a/ffmpeg.c > +++ b/ffmpeg.c > @@ -1434,6 +1434,7 @@ static int reap_filters(int flush) > AVFilterContext *filter; > AVCodecContext *enc = ost->enc_ctx; > int ret = 0; > + int do_check_init_output_file = 0; > > if (!ost->filter || !ost->filter->graph->graph) > continue; > @@ -1448,9 +1449,7 @@ static int reap_filters(int flush) > exit_program(1); > } > > - ret = check_init_output_file(of, ost->file_index); > - if (ret < 0) > - exit_program(1); > + do_check_init_output_file = 1; > } > > if (!ost->filtered_frame && !(ost->filtered_frame = av_frame_alloc())) { > @@ -1526,6 +1525,44 @@ static int reap_filters(int flush) > } > > av_frame_unref(filtered_frame); > + > + /* > + * It is important to call check_init_output_file() here, after > + * do_video_out() was called, instead of in init_output_stream(), > + * as was done previously. > + * If called from init_output_stream(), it is possible that not > + * everything will have been fully initialized by the time that > + * check_init_output_file() is called, and if > + * check_init_output_file() determines that all output streams > + * have been initialized, it will write the header. An example > + * of initialization that depends on do_video_out() being called > + * at least once is the specification of interlaced video, which > + * happens in do_video_out(). This is particularly relevant when > + * decoding--without processing a video frame, the interlaced > + * video setting is not propagated before the header is written, > + * and that causes problems. > + * TODO: should probably handle interlaced video differently > + * and not depend on it being setup in do_video_out(). Another > + * solution would be to set it up once by examining the input > + * header. > + */ > + if (do_check_init_output_file) { > + ret = check_init_output_file(of, ost->file_index); > + if (ret < 0) > + exit_program(1); > + do_check_init_output_file = 0; > + } > + } > + > + /* > + * Can't wait too long to call check_init_output_file(). > + * Otherwise, bad things start to occur. > + * If didn't do it earlier, do it by the time it gets here. > + */ > + if (do_check_init_output_file) { > + ret = check_init_output_file(of, ost->file_index); > + if (ret < 0) > + exit_program(1); > } > } > Doesn't look good to me. The big comment compared with the small amount of code is indicative that this approach is way too complex and fragile.
On 5/13/2017 3:44 AM, wm4 wrote: > On Fri, 12 May 2017 13:28:14 -0700 > Aaron Levinson <alevinsn@aracnet.com> wrote: > >> Purpose: Made execution of reap_filters() more deterministic with >> respect to when headers are written in relationship with the >> initialization of output streams and the processing of input audio >> and/or video frames. This change fixes a bug in ffmpeg encountered >> when decoding interlaced video. In some cases, ffmpeg would treat it >> as progressive. >> >> Detailed description of problem: Run the following command: "ffmpeg -i >> test8_1080i.h264 -c:v mpeg2video test8_1080i_mp2.ts". In this case, >> test8_1080i.h264 is an H.264-encoded file with 1080i59.94 >> (interlaced). Prior to the patch, the following output is generated: >> >> Input #0, h264, from 'test8_1080i.h264': >> Duration: N/A, bitrate: N/A >> Stream #0:0: Video: h264 (High), yuv420p(top first), 1920x1080 [SAR 1:1 DAR 16:9], 29.97 fps, 29.97 tbr, 1200k tbn, 59.94 tbc >> Stream mapping: >> Stream #0:0 -> #0:0 (h264 (native) -> mpeg2video (native)) >> Press [q] to stop, [?] for help >> Output #0, mpegts, to 'test8_1080i_mp2_2.ts': >> Metadata: >> encoder : Lavf57.72.100 >> Stream #0:0: Video: mpeg2video (Main), yuv420p, 1920x1080 [SAR 1:1 DAR 16:9], q=2-31, 200 kb/s, 29.97 fps, 90k tbn, 29.97 tbc >> Metadata: >> encoder : Lavc57.92.100 mpeg2video >> >> which demonstrates the bug. The output stream should instead look like: >> >> Stream #0:0: Video: mpeg2video (Main), yuv420p(top coded first (swapped)), 1920x1080 [SAR 1:1 DAR 16:9], q=2-31, 200 kb/s, 29.97 fps, 90k tbn, 29.97 tbc >> >> The bug is caused by the fact that reap_filters() calls >> init_output_stream(), which is then immediately followed by a call to >> check_init_output_file(), and this is all done prior to the first call >> to do_video_out(). An initial call to do_video_out() is necessary to >> populate the interlaced video information to the output stream's >> codecpar (mux_par->field_order in do_video_out()). However, >> check_init_output_file() calls avformat_write_header() prior to the >> initial call to do_video_out(), so field_order is populated too late, >> and the header is written with the default field_order value, >> progressive. >> >> Signed-off-by: Aaron Levinson <alevinsn@aracnet.com> >> --- >> ffmpeg.c | 43 ++++++++++++++++++++++++++++++++++++++++--- >> 1 file changed, 40 insertions(+), 3 deletions(-) >> >> diff --git a/ffmpeg.c b/ffmpeg.c >> index 3cd45ba665..7b044b068c 100644 >> --- a/ffmpeg.c >> +++ b/ffmpeg.c >> @@ -1434,6 +1434,7 @@ static int reap_filters(int flush) >> AVFilterContext *filter; >> AVCodecContext *enc = ost->enc_ctx; >> int ret = 0; >> + int do_check_init_output_file = 0; >> >> if (!ost->filter || !ost->filter->graph->graph) >> continue; >> @@ -1448,9 +1449,7 @@ static int reap_filters(int flush) >> exit_program(1); >> } >> >> - ret = check_init_output_file(of, ost->file_index); >> - if (ret < 0) >> - exit_program(1); >> + do_check_init_output_file = 1; >> } >> >> if (!ost->filtered_frame && !(ost->filtered_frame = av_frame_alloc())) { >> @@ -1526,6 +1525,44 @@ static int reap_filters(int flush) >> } >> >> av_frame_unref(filtered_frame); >> + >> + /* >> + * It is important to call check_init_output_file() here, after >> + * do_video_out() was called, instead of in init_output_stream(), >> + * as was done previously. >> + * If called from init_output_stream(), it is possible that not >> + * everything will have been fully initialized by the time that >> + * check_init_output_file() is called, and if >> + * check_init_output_file() determines that all output streams >> + * have been initialized, it will write the header. An example >> + * of initialization that depends on do_video_out() being called >> + * at least once is the specification of interlaced video, which >> + * happens in do_video_out(). This is particularly relevant when >> + * decoding--without processing a video frame, the interlaced >> + * video setting is not propagated before the header is written, >> + * and that causes problems. >> + * TODO: should probably handle interlaced video differently >> + * and not depend on it being setup in do_video_out(). Another >> + * solution would be to set it up once by examining the input >> + * header. >> + */ >> + if (do_check_init_output_file) { >> + ret = check_init_output_file(of, ost->file_index); >> + if (ret < 0) >> + exit_program(1); >> + do_check_init_output_file = 0; >> + } >> + } >> + >> + /* >> + * Can't wait too long to call check_init_output_file(). >> + * Otherwise, bad things start to occur. >> + * If didn't do it earlier, do it by the time it gets here. >> + */ >> + if (do_check_init_output_file) { >> + ret = check_init_output_file(of, ost->file_index); >> + if (ret < 0) >> + exit_program(1); >> } >> } >> > > Doesn't look good to me. The big comment compared with the small > amount of code is indicative that this approach is way too complex and > fragile. This patch actually prevents a regression a patch dealing with delayed header writing i'm intending to post would introduce. If you think this approach is not optimal then please suggest or write another with the same results, because this is header writing issue that needs fixing.
On Fri, May 12, 2017 at 01:28:14PM -0700, Aaron Levinson wrote: > Purpose: Made execution of reap_filters() more deterministic with > respect to when headers are written in relationship with the > initialization of output streams and the processing of input audio > and/or video frames. This change fixes a bug in ffmpeg encountered > when decoding interlaced video. In some cases, ffmpeg would treat it > as progressive. > > Detailed description of problem: Run the following command: "ffmpeg -i > test8_1080i.h264 -c:v mpeg2video test8_1080i_mp2.ts". In this case, > test8_1080i.h264 is an H.264-encoded file with 1080i59.94 > (interlaced). Prior to the patch, the following output is generated: > > Input #0, h264, from 'test8_1080i.h264': > Duration: N/A, bitrate: N/A > Stream #0:0: Video: h264 (High), yuv420p(top first), 1920x1080 [SAR 1:1 DAR 16:9], 29.97 fps, 29.97 tbr, 1200k tbn, 59.94 tbc > Stream mapping: > Stream #0:0 -> #0:0 (h264 (native) -> mpeg2video (native)) > Press [q] to stop, [?] for help > Output #0, mpegts, to 'test8_1080i_mp2_2.ts': > Metadata: > encoder : Lavf57.72.100 > Stream #0:0: Video: mpeg2video (Main), yuv420p, 1920x1080 [SAR 1:1 DAR 16:9], q=2-31, 200 kb/s, 29.97 fps, 90k tbn, 29.97 tbc > Metadata: > encoder : Lavc57.92.100 mpeg2video > > which demonstrates the bug. The output stream should instead look like: > > Stream #0:0: Video: mpeg2video (Main), yuv420p(top coded first (swapped)), 1920x1080 [SAR 1:1 DAR 16:9], q=2-31, 200 kb/s, 29.97 fps, 90k tbn, 29.97 tbc > > The bug is caused by the fact that reap_filters() calls > init_output_stream(), which is then immediately followed by a call to > check_init_output_file(), and this is all done prior to the first call > to do_video_out(). An initial call to do_video_out() is necessary to > populate the interlaced video information to the output stream's > codecpar (mux_par->field_order in do_video_out()). However, > check_init_output_file() calls avformat_write_header() prior to the > initial call to do_video_out(), so field_order is populated too late, > and the header is written with the default field_order value, > progressive. > > Signed-off-by: Aaron Levinson <alevinsn@aracnet.com> > --- > ffmpeg.c | 43 ++++++++++++++++++++++++++++++++++++++++--- > 1 file changed, 40 insertions(+), 3 deletions(-) This breaks ./ffmpeg -itsoffset 2 -re -i matrixbench_mpeg2.mpg -vcodec libx264 -an -t 3 -f rtp rtp://127.0.0.1:17755 > h264.sdp 2> log1 & sleep 2 ./ffmpeg -protocol_whitelist file,rtp,udp -i h264.sdp -y -t 0.9 out-h264.avi (h264.sdp: Invalid data found when processing input) for the input see: http://samples.ffmpeg.org/benchmark/testsuite1/matrixbench_mpeg2.mpg also increasing sleep to 2.5 or 3 does not make it work, it fails differently (h264 decoder errors) [...]
diff --git a/ffmpeg.c b/ffmpeg.c index 3cd45ba665..7b044b068c 100644 --- a/ffmpeg.c +++ b/ffmpeg.c @@ -1434,6 +1434,7 @@ static int reap_filters(int flush) AVFilterContext *filter; AVCodecContext *enc = ost->enc_ctx; int ret = 0; + int do_check_init_output_file = 0; if (!ost->filter || !ost->filter->graph->graph) continue; @@ -1448,9 +1449,7 @@ static int reap_filters(int flush) exit_program(1); } - ret = check_init_output_file(of, ost->file_index); - if (ret < 0) - exit_program(1); + do_check_init_output_file = 1; } if (!ost->filtered_frame && !(ost->filtered_frame = av_frame_alloc())) { @@ -1526,6 +1525,44 @@ static int reap_filters(int flush) } av_frame_unref(filtered_frame); + + /* + * It is important to call check_init_output_file() here, after + * do_video_out() was called, instead of in init_output_stream(), + * as was done previously. + * If called from init_output_stream(), it is possible that not + * everything will have been fully initialized by the time that + * check_init_output_file() is called, and if + * check_init_output_file() determines that all output streams + * have been initialized, it will write the header. An example + * of initialization that depends on do_video_out() being called + * at least once is the specification of interlaced video, which + * happens in do_video_out(). This is particularly relevant when + * decoding--without processing a video frame, the interlaced + * video setting is not propagated before the header is written, + * and that causes problems. + * TODO: should probably handle interlaced video differently + * and not depend on it being setup in do_video_out(). Another + * solution would be to set it up once by examining the input + * header. + */ + if (do_check_init_output_file) { + ret = check_init_output_file(of, ost->file_index); + if (ret < 0) + exit_program(1); + do_check_init_output_file = 0; + } + } + + /* + * Can't wait too long to call check_init_output_file(). + * Otherwise, bad things start to occur. + * If didn't do it earlier, do it by the time it gets here. + */ + if (do_check_init_output_file) { + ret = check_init_output_file(of, ost->file_index); + if (ret < 0) + exit_program(1); } }
Purpose: Made execution of reap_filters() more deterministic with respect to when headers are written in relationship with the initialization of output streams and the processing of input audio and/or video frames. This change fixes a bug in ffmpeg encountered when decoding interlaced video. In some cases, ffmpeg would treat it as progressive. Detailed description of problem: Run the following command: "ffmpeg -i test8_1080i.h264 -c:v mpeg2video test8_1080i_mp2.ts". In this case, test8_1080i.h264 is an H.264-encoded file with 1080i59.94 (interlaced). Prior to the patch, the following output is generated: Input #0, h264, from 'test8_1080i.h264': Duration: N/A, bitrate: N/A Stream #0:0: Video: h264 (High), yuv420p(top first), 1920x1080 [SAR 1:1 DAR 16:9], 29.97 fps, 29.97 tbr, 1200k tbn, 59.94 tbc Stream mapping: Stream #0:0 -> #0:0 (h264 (native) -> mpeg2video (native)) Press [q] to stop, [?] for help Output #0, mpegts, to 'test8_1080i_mp2_2.ts': Metadata: encoder : Lavf57.72.100 Stream #0:0: Video: mpeg2video (Main), yuv420p, 1920x1080 [SAR 1:1 DAR 16:9], q=2-31, 200 kb/s, 29.97 fps, 90k tbn, 29.97 tbc Metadata: encoder : Lavc57.92.100 mpeg2video which demonstrates the bug. The output stream should instead look like: Stream #0:0: Video: mpeg2video (Main), yuv420p(top coded first (swapped)), 1920x1080 [SAR 1:1 DAR 16:9], q=2-31, 200 kb/s, 29.97 fps, 90k tbn, 29.97 tbc The bug is caused by the fact that reap_filters() calls init_output_stream(), which is then immediately followed by a call to check_init_output_file(), and this is all done prior to the first call to do_video_out(). An initial call to do_video_out() is necessary to populate the interlaced video information to the output stream's codecpar (mux_par->field_order in do_video_out()). However, check_init_output_file() calls avformat_write_header() prior to the initial call to do_video_out(), so field_order is populated too late, and the header is written with the default field_order value, progressive. Signed-off-by: Aaron Levinson <alevinsn@aracnet.com> --- ffmpeg.c | 43 ++++++++++++++++++++++++++++++++++++++++--- 1 file changed, 40 insertions(+), 3 deletions(-)