[FFmpeg-devel] [PATCH] Fixed bug encountered when decoding interlaced video
Michael Niedermayer
michael at niedermayer.cc
Sat May 13 03:51:39 EEST 2017
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.
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 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.
[...]
--
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
Avoid a single point of failure, be that a person or equipment.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 181 bytes
Desc: Digital signature
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20170513/a1d112a9/attachment.sig>
More information about the ffmpeg-devel
mailing list