From patchwork Fri May 5 06:46:30 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Aaron Levinson X-Patchwork-Id: 3574 Delivered-To: ffmpegpatchwork@gmail.com Received: by 10.103.3.129 with SMTP id 123csp67924vsd; Thu, 4 May 2017 23:46:43 -0700 (PDT) X-Received: by 10.28.7.18 with SMTP id 18mr4187673wmh.80.1493966803795; Thu, 04 May 2017 23:46:43 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1493966803; cv=none; d=google.com; s=arc-20160816; b=dH6R2L5BLTV/QeYLPD8UCswMyOfIFVd5qr+riJQbFL/Mw/1pLZxHfD3FuXqLTlceQ7 Fk1Rw4/6uVVRzIw+3v7GC0vGtd4GdqqBEKummaYMz3Deq32o08xatC8ROTlEI2NwIFzL TmHGDWvyn6OQ8ULElXgJSVvm8dvBWDLlr4tWAwnaJK00sFVZ/pIsBopG8rE+o0US8g4Q QcHd0PlocVOq67iiDpehUbZ3e3eWXLyvoRGC4rZSiVy27AnBid1SRutr+k/szO4di7yN +PDpwGcnPwdkGJaEWTG8BoHlSYd6MMBOjm0gL+IxMbOqP6k9GqNR23GVVm6x8rHRNNU8 B3/Q== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=sender:errors-to:content-transfer-encoding:reply-to:list-subscribe :list-help:list-post:list-archive:list-unsubscribe:list-id :precedence:subject:in-reply-to:mime-version:user-agent:date :message-id:from:to:references:delivered-to :arc-authentication-results; bh=zvRI8X4And6CorI9pfU7yp6WhmWAHYFhdCtEVQXJE3A=; b=WLVzJJhC3aV7e8VajwgPFAZc9d/r111S2xJnjRsZc9014Z89YALFqQBgc1b1/QdnTq hyJpxi+N9kMRmMywjG1PAQuNW7+Fum4OfDzcNE8EzabHZJ2H6mAgfbG48VCpdO/MKjhl zJfU1mPBVahhCyYWVa6JxuPzxBwGEGKtBwxyE6u70kQloGpZGyHVbQzL9wOCa3X9LMJI 6VmZePPQR7HDMg6AfMjSQN1u72pUthQUazcAMoHbVIPMFKucVEo94PMr1Za6nWk54R54 dXQ7PZ6I6XYTC7c/7oYNSDDuIWdacKiErneThG1U12leS62YLvoPCyjJ0iHiAZN4gjSQ RPcA== ARC-Authentication-Results: i=1; mx.google.com; spf=pass (google.com: domain of ffmpeg-devel-bounces@ffmpeg.org designates 79.124.17.100 as permitted sender) smtp.mailfrom=ffmpeg-devel-bounces@ffmpeg.org Return-Path: Received: from ffbox0-bg.mplayerhq.hu (ffbox0-bg.ffmpeg.org. [79.124.17.100]) by mx.google.com with ESMTP id 91si4833251wrn.189.2017.05.04.23.46.43; Thu, 04 May 2017 23:46:43 -0700 (PDT) Received-SPF: pass (google.com: domain of ffmpeg-devel-bounces@ffmpeg.org designates 79.124.17.100 as permitted sender) client-ip=79.124.17.100; Authentication-Results: mx.google.com; spf=pass (google.com: domain of ffmpeg-devel-bounces@ffmpeg.org designates 79.124.17.100 as permitted sender) smtp.mailfrom=ffmpeg-devel-bounces@ffmpeg.org Received: from [127.0.1.1] (localhost [127.0.0.1]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTP id 46AB6680CCC; Fri, 5 May 2017 09:46:35 +0300 (EEST) X-Original-To: ffmpeg-devel@ffmpeg.org Delivered-To: ffmpeg-devel@ffmpeg.org Received: from white.spiritone.com (white.spiritone.com [216.99.193.38]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTP id A5D8A680656 for ; Fri, 5 May 2017 09:46:28 +0300 (EEST) Received: from [192.168.3.101] (184-100-204-251.ptld.qwest.net [184.100.204.251]) by white.spiritone.com (Postfix) with ESMTPSA id A66387340552 for ; Thu, 4 May 2017 23:46:31 -0700 (PDT) References: <6ba95ea3-746d-aba8-0986-7fe79d3002ef@aracnet.com> <1a7558a5-d736-3f8f-6c72-185ab9e92466@fem.tu-ilmenau.de> <32df01a4-4612-0881-429c-cd4be53b6eea@aracnet.com> To: FFmpeg development discussions and patches From: Aaron Levinson Message-ID: <3fe3c283-3c0e-a246-8d5b-b5e86337914a@aracnet.com> Date: Thu, 4 May 2017 23:46:30 -0700 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:45.0) Gecko/20100101 Thunderbird/45.8.0 MIME-Version: 1.0 In-Reply-To: Subject: Re: [FFmpeg-devel] [PATCH] Fixed bug encountered when decoding interlaced video X-BeenThere: ffmpeg-devel@ffmpeg.org X-Mailman-Version: 2.1.20 Precedence: list List-Id: FFmpeg development discussions and patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Reply-To: FFmpeg development discussions and patches Errors-To: ffmpeg-devel-bounces@ffmpeg.org Sender: "ffmpeg-devel" On 4/12/2017 6:08 PM, Aaron Levinson wrote: > On 3/26/2017 10:34 AM, Aaron Levinson wrote: >> On 3/26/2017 4:41 AM, Matthias Hunstock wrote: >>> Am 26.03.2017 um 11:50 schrieb Aaron Levinson: >>>> When using the following command to play back either file: >>>> ffmpeg -i -f decklink -pix_fmt uyvy422 "DeckLink SDI >>>> 4K", I noticed that with the mpegts file with the AAC audio stream, >>>> it would correctly select an interlaced video mode for the video >>>> output stream, but with the mpegts file with the Opus audio stream, >>>> it would use a progressive video mode (1080p29.97) for the video >>>> output stream. >>> >>> Which FFmpeg version did you test this with? >>> >>> There was a change related to this just short time ago. >>> >>> Does it happen with current git HEAD? >>> >>> Matthias >> >> This issue occurs with the current git HEAD. I'm aware of the >> Blackmagic improvement that was added in February to add support for >> interlaced video modes on output, and actually that's one of the reasons >> why I'm using the latest git sources, as opposed to, say, 3.2.4. This >> particular issue has nothing to do with Blackmagic, and I only used >> Blackmagic in the example that reproduces the bug because it is >> something that can be reproduced on both Windows and Linux (and >> presumably also on OS/X). The issue also occurs if I do something like >> -f rawvideo out.avi on Windows, and I'm sure that there are plenty of >> other examples. >> >> Aaron Levinson > > Has anyone had a chance to review this patch yet, which I submitted on March 26th? To demonstrate the impact of this patch, here's some output of before and after for an .h264 file with interlaced 1080i59.94 video content: > > Command-line: ffmpeg -i test8_1080i.h264 -c:v mpeg2video test8_1080i_mp2.ts > > Before patch: > > -------------------------------------- > > 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 > Side data: > cpb: bitrate max/min/avg: 0/0/200000 buffer size: 0 vbv_delay: -1 > > -------------------------------------- > > After patch: > > -------------------------------------- > > 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(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 > Metadata: > encoder : Lavc57.92.100 mpeg2video > Side data: > cpb: bitrate max/min/avg: 0/0/200000 buffer size: 0 vbv_delay: -1 > > -------------------------------------- > > As can be seen, before the patch, after decoding the .h264 file and then re-encoding it as mpeg2video in an mpegts container, the interlaced aspect of the video has been lost in the output, and it is now effectively 1080p29.97, although the video hasn't actually been converted to progressive. ffmpeg simply thinks that the video is progressive when it is not. With the patch, the interlaced aspect is not lost and propagates to the output. So, this conclusively demonstrates that the issue has nothing to do with Blackmagic and is a more general issue with interlaced video and decoding. > > I can make the input file available if that would be helpful. > > Anyway, it would be great if this bug fix could make it into ffmpeg. > > Thanks, > Aaron Levinson 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 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 --- 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); + /** * Get and encode new output from any of the filtergraphs, without causing * activity. @@ -1433,6 +1435,7 @@ static int reap_filters(int flush) AVFilterContext *filter; AVCodecContext *enc = ost->enc_ctx; int ret = 0; + int did_init_output_stream = 0; if (!ost->filter || !ost->filter->graph->graph) continue; @@ -1440,12 +1443,14 @@ static int reap_filters(int flush) if (!ost->initialized) { char error[1024] = ""; - ret = init_output_stream(ost, error, sizeof(error)); + ret = init_output_stream(ost, error, sizeof(error), 0); if (ret < 0) { av_log(NULL, AV_LOG_ERROR, "Error initializing output stream %d:%d -- %s\n", ost->file_index, ost->index, error); exit_program(1); } + + did_init_output_stream = 1; } if (!ost->filtered_frame && !(ost->filtered_frame = av_frame_alloc())) { @@ -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; } } @@ -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; } @@ -3633,7 +3678,7 @@ static int transcode_init(void) if (output_streams[i]->filter) continue; - ret = init_output_stream(output_streams[i], error, sizeof(error)); + ret = init_output_stream(output_streams[i], error, sizeof(error), 1); if (ret < 0) goto dump_format; }