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

[James Almer]

On 5/13/2017 3:44 AM, wm4 wrote:
> 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.

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.