On Fri, 12 May 2017 13:28:14 -0700
Aaron Levinson <alevi...@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 <alevi...@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.
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
