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 <alevi...@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 <alevi...@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
[...]
@@ -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()
