[FFmpeg-devel] [PATCH] Fixed bug encountered when decoding interlaced video
Aaron Levinson
alevinsn at aracnet.com
Sat May 13 04:22:19 EEST 2017
On 5/12/2017 5:51 PM, Michael Niedermayer wrote:
> On Fri, May 12, 2017 at 01:19:43PM -0700, Aaron Levinson wrote:
>> 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().
>
> I dont understand your concern
> check_init_output_file() will set header_written as soon as it does
> anything and not run again then.
No, check_init_output_file() will only set header_written if all the
output streams have been initialized by the time it has been called. If
all the output streams haven't been initialized by the time it is
called, it returns immediately. That is the case I'm talking about.
> If you are concernd about the speed, the loop in check_init_output_file()
> which determines if all streams have been initialized can be replaced
> by a simple varaiable counting the number of uninitialized streams
> as in
> if (of->count_uinitialized_streams > 0 || of->header_written)
> return 0;
>
> or even
> if (of->count_uinitialized_streams == 0 && !of->header_written) {
> ret = check_init_output_file(of, ost->file_index);
> }
I'm not concerned about the speed, but if header_written is used as the
check, then there are definitely cases in which check_init_output_file()
will be called multiple times for the same stream, and this behavior
would be different from that currently in the code base, which calls
check_init_output_file() only once per output stream, in
init_output_stream(). init_output_stream() is also only called once per
output stream, but in this case, that behavior is controlled by
ost->initialized, which is set to 1 by init_output_stream(). I guess I
could add another boolean to indicate whether or not
check_init_output_file() has been called for an output stream, but as
this is only relevant for reap_filters() currently, I think my approach
of using a local variable is preferable.
More precisely, it can go through the loop in reap_filters() multiple
times for the same output stream before it does anything with the second
output stream, if there is a second output stream. This is due to the
following code in reap_filters() (shortened for discussion purposes):
/* Reap all buffers present in the buffer sinks */
for (i = 0; i < nb_output_streams; i++) {
OutputStream *ost = output_streams[i];
if (!ost->filter || !ost->filter->graph->graph)
continue;
In some cases, it takes longer for ost->filter to be setup for one
output stream compared to another, and in that case, it will frequently
iterate through the loop multiple times for the output stream that was
setup more quickly.
> I dont like did_init_output_stream because it somehow creates a
> 2nd layer check at a different location making this more complex
> than needed.
> I think all the checks that check if check_init_output_file() should
> run or not should be at the same place.
See above. But, if you want a consistent way of checking whether or not
check_init_output_file() has been called for the current output stream,
as opposed to a local variable, then I think a boolean will need to be
added to OutputStream, similar to the already existing initialized
member variable.
Aaron Levinson
More information about the ffmpeg-devel
mailing list