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 <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
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 _______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
