[FFmpeg-devel] [PATCH 2/2] ffmpeg: Made execution of reap_filters() more deterministic with respect to when headers are written

[wm4]

On Fri, 12 May 2017 13:28:14 -0700
Aaron Levinson <alevinsn at 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 at 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.