[FFmpeg-devel] [PATCH] Fixed bug encountered when decoding interlaced video

Aaron Levinson alevinsn at aracnet.com
Fri May 12 23:19:43 EEST 2017


On 5/9/2017 11:56 AM, Michael Niedermayer wrote:
> On Thu, May 04, 2017 at 11:46:30PM -0700, Aaron Levinson wrote:
>>
>> I've provided a new version of the patch.  When I created the first version of the patch on March 26th, this was the first patch that I submitted to ffmpeg, and some aspects were rough.  I had indicated that the patch passed regression tests, but all I did was run "make fate", instead of "make fate SAMPLES=fate-suite/", and once I understood that I should use fate-suite, I discovered that some of the FATE tests failed with this patch.  I fixed the issues with the patch, adjusted some comments, and adjusted the patch description text.  I've confirmed that this patch passes all fate-suite tests for 64-bit MSVC on Windows and 64-bit gcc on Linux.
>>
>> Thanks,
>> Aaron Levinson
>>
>> ------------------------------------------------------------------------
>>
>> From 85aa383f5753795652bae015f4fe91b016f7387e Mon Sep 17 00:00:00 2001
>> From: Aaron Levinson <alevinsn at aracnet.com>
>> Date: Thu, 4 May 2017 22:54:31 -0700
>> Subject: [PATCH] ffmpeg:  Fixed bug with decoding interlaced video
>>
>> Purpose: Fixed bug in ffmpeg encountered when decoding interlaced
>> video.  In some cases, ffmpeg would treat it as progressive.  As a
>> result of the change, the issues with interlaced video are fixed.
>>
>> 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 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 in turn calls 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.
>>
>> Comments:
>>
>> -- ffmpeg.c:
>> a) Enhanced init_output_stream() to take an additional input
>>    indicating whether or not check_init_output_file() should be
>>    called.  There are other places that call init_output_stream(), and
>>    it was important to make sure that these continued to work
>>    properly.
>> b) Adjusted reap_filters() such that, in the case that
>>    init_output_stream() is called, it indicates that it should not
>>    call check_init_output_file() in init_output_stream().  Instead, it
>>    makes the call to check_init_output_file() directly, but after it
>>    does the filtered frame setup and processing.
>>
>> Signed-off-by: Aaron Levinson <alevinsn at aracnet.com>
>> ---
>>  ffmpeg.c | 61 +++++++++++++++++++++++++++++++++++++++++++++++++++++--------
>>  1 file changed, 53 insertions(+), 8 deletions(-)
>>
>> diff --git a/ffmpeg.c b/ffmpeg.c
>> index e798d92277..45dbfafc04 100644
>> --- a/ffmpeg.c
>> +++ b/ffmpeg.c
>
>> @@ -1400,7 +1400,7 @@ static void do_video_stats(OutputStream *ost, int frame_size)
>>      }
>>  }
>>
>> -static int init_output_stream(OutputStream *ost, char *error, int error_len);
>> +static int init_output_stream(OutputStream *ost, char *error, int error_len, int do_check_output_file);
>>
>>  static void finish_output_stream(OutputStream *ost)
>>  {
>
>> @@ -1415,6 +1415,8 @@ static void finish_output_stream(OutputStream *ost)
>>      }
>>  }
>>
>> +static int check_init_output_file(OutputFile *of, int file_index);
>
> should be close to the file top, together with similar prototypes

Will do.

> [...]
>> @@ -1521,6 +1526,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 (did_init_output_stream) {
>> +                ret = check_init_output_file(of, ost->file_index);
>> +                if (ret < 0)
>> +                    return ret;
>> +                did_init_output_stream = 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 (did_init_output_stream) {
>> +            ret = check_init_output_file(of, ost->file_index);
>> +            if (ret < 0)
>> +                return ret;
>>          }
>>      }
>
> you can possibly avoid did_init_output_stream by checking
> of->header_written in check_init_output_file()

I could do that, but then it would call check_init_output_file() 
potentially multiple times per stream.  Depending on when an output 
stream is ready, it can go through reap_filters() multiple times for the 
same output stream.  For example, if there are two output streams, and 
output stream #2 is slow to be setup, it might go through reap_filters() 
multiple times for output stream #1 before it ever hits output stream 
#2.  If of->header_written is used to determine this, then it will call 
check_init_output_file() each time.  There is no benefit to calling 
check_init_output_file() multiple times per stream, although it doesn't 
hurt to do so as well.  Also, doing this perhaps makes some assumptions 
about the behavior of check_init_output_file().

>> @@ -1888,7 +1931,7 @@ static void flush_encoders(void)
>>                  finish_output_stream(ost);
>>              }
>>
>> -            ret = init_output_stream(ost, error, sizeof(error));
>> +            ret = init_output_stream(ost, error, sizeof(error), 1);
>>              if (ret < 0) {
>>                  av_log(NULL, AV_LOG_ERROR, "Error initializing output stream %d:%d -- %s\n",
>>                         ost->file_index, ost->index, error);
>> @@ -3396,7 +3439,7 @@ static int init_output_stream_encode(OutputStream *ost)
>>      return 0;
>>  }
>>
>> -static int init_output_stream(OutputStream *ost, char *error, int error_len)
>> +static int init_output_stream(OutputStream *ost, char *error, int error_len, int do_check_output_file)
>>  {
>>      int ret = 0;
>>
>
>> @@ -3564,9 +3607,11 @@ static int init_output_stream(OutputStream *ost, char *error, int error_len)
>>
>>      ost->initialized = 1;
>>
>> -    ret = check_init_output_file(output_files[ost->file_index], ost->file_index);
>> -    if (ret < 0)
>> -        return ret;
>> +    if (do_check_output_file) {
>> +        ret = check_init_output_file(output_files[ost->file_index], ost->file_index);
>> +        if (ret < 0)
>> +            return ret;
>> +    }
>>
>>      return ret;
>>  }
>
> It should be cleaner to not call check_init_output_file() from
> init_output_stream() which avoids the new argument.

As discussed over IRC, I'll make that change as a separate "cosmetic" 
patch and submit an additional patch for the bug fix.  The two patches 
should follow shortly after this e-mail.  Also, I confirmed that FATE 
passes on both Windows and Linux with both patches.

>
> thx
>
> PS: confirmed that interlaced information appears with this patch
> that was not there before
>
> [...]

Aaron Levinson


More information about the ffmpeg-devel mailing list