[FFmpeg-devel] Fixed bug encountered when decoding interlaced video

Submitted by Aaron Levinson on May 5, 2017, 6:46 a.m.

Details

Message ID 3fe3c283-3c0e-a246-8d5b-b5e86337914a@aracnet.com
State New
Headers show

Commit Message

Aaron Levinson May 5, 2017, 6:46 a.m.
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 <mpegts file> -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 <alevinsn@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@aracnet.com>
---
 ffmpeg.c | 61 +++++++++++++++++++++++++++++++++++++++++++++++++++++--------
 1 file changed, 53 insertions(+), 8 deletions(-)

Comments

wm4 May 5, 2017, 7:55 a.m.
On Thu, 4 May 2017 23:46:30 -0700
Aaron Levinson <alevinsn@aracnet.com> wrote:

> 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 <mpegts file> -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 <alevinsn@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@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);
> +
>  /**
>   * 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;
>      }

As I said on IRC, I'm skeptic against this, but I'm also not sure
whether I understand the situation.

First, your change seems a bit fragile, and almost relies on
coincidences. This could easily break in the future. (And if we add a
FATE test, it would probably hit random people trying to change
ffmpeg.c, and would cause them more work.)

Looking at the current do_video_out() function (which btw. is way too
long and also has broken indentation), I see the following line:

  mux_par->field_order = AV_FIELD_PROGRESSIVE;

(and similar assignments to field_order to set interlaced modes)

This is apparently run on every frame. This looks pretty wrong to me.
It should _never_ change the codecpar after headers were written. This
is simply invalid API use.

If this is really a parameter that can change per frame, and that the
_muxer_ needs this information (per packet I suppose), a side data
signaling the field order should be added. mpegtsenc.c doesn't seem to
read field_order at all, though.

So I remain confused.
Aaron Levinson May 5, 2017, 8:11 a.m.
On 5/5/2017 12:55 AM, wm4 wrote:
> On Thu, 4 May 2017 23:46:30 -0700
> Aaron Levinson <alevinsn@aracnet.com> wrote:
>
>> 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:
 >>>>> [...]
 >>> [...]
>> 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@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@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);
>> +
>>  /**
>>   * 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;
>>      }
>
> As I said on IRC, I'm skeptic against this, but I'm also not sure
> whether I understand the situation.
>
> First, your change seems a bit fragile, and almost relies on
> coincidences. This could easily break in the future. (And if we add a
> FATE test, it would probably hit random people trying to change
> ffmpeg.c, and would cause them more work.)

I guess I don't understand why you think this change is fragile, relies 
on coincidences, and could easily break in the future.  The existing 
code is already fragile.  As I indicated in previous e-mails on this 
topic, the behavior of the current code can change depending on timing 
when there is more than one stream.  This patch should make things more 
deterministic.  And, I do think it is appropriate to add an interlaced 
video decoding FATE test, although there is no point to doing so until 
this bug is fixed, as the current behavior is broken.

> Looking at the current do_video_out() function (which btw. is way too
> long and also has broken indentation), I see the following line:
>
>   mux_par->field_order = AV_FIELD_PROGRESSIVE;
>
> (and similar assignments to field_order to set interlaced modes)
>
> This is apparently run on every frame. This looks pretty wrong to me.
> It should _never_ change the codecpar after headers were written. This
> is simply invalid API use.
>
> If this is really a parameter that can change per frame, and that the
> _muxer_ needs this information (per packet I suppose), a side data
> signaling the field order should be added. mpegtsenc.c doesn't seem to
> read field_order at all, though.
>
> So I remain confused.

Yes, I agree that the approach of changing field_order for every frame 
is not the right way to go, but this code was already existing.  A 
future patch could undertake the task of handling interlaced video in a 
more proper way, and I wrote a TODO comment to that point.

Aaron
wm4 May 5, 2017, 9:26 a.m.
On Fri, 5 May 2017 01:11:17 -0700
Aaron Levinson <alevinsn@aracnet.com> wrote:

> > As I said on IRC, I'm skeptic against this, but I'm also not sure
> > whether I understand the situation.
> >
> > First, your change seems a bit fragile, and almost relies on
> > coincidences. This could easily break in the future. (And if we add a
> > FATE test, it would probably hit random people trying to change
> > ffmpeg.c, and would cause them more work.)  
> 
> I guess I don't understand why you think this change is fragile, relies 
> on coincidences, and could easily break in the future.

Well, the implemented and working logic is that ffmpeg.c waits until it
has encoded packets for every stream, and then writes the file headers.
That should work pretty well to let metadata naturally "flow" through
decoder, filters, and encoder. But your case doesn't fit right into it,
so you apparently have to make it somehow call do_video_out() more.
(Also a thing I don't understand - do_video_out() should have been
called at least once to encode a video packet.)

And looking at how field_order is set, the existing code doesn't make
sense in the first place.

So maybe it would be better to fix the actual problem, instead of
adding another workaround.

At least that's my thinking.

>  The existing 
> code is already fragile.

Indeed. That (and me being sleep deprived) probably contributes to that
I just don't understand your fix.

Also feel free to ignore me - I'm neither maintainer of ffmpeg.c nor an
authority on it.

>  As I indicated in previous e-mails on this 
> topic, the behavior of the current code can change depending on timing 
> when there is more than one stream.  This patch should make things more 
> deterministic.  And, I do think it is appropriate to add an interlaced 
> video decoding FATE test, although there is no point to doing so until 
> this bug is fixed, as the current behavior is broken.
> 
> > Looking at the current do_video_out() function (which btw. is way too
> > long and also has broken indentation), I see the following line:
> >
> >   mux_par->field_order = AV_FIELD_PROGRESSIVE;
> >
> > (and similar assignments to field_order to set interlaced modes)
> >
> > This is apparently run on every frame. This looks pretty wrong to me.
> > It should _never_ change the codecpar after headers were written. This
> > is simply invalid API use.
> >
> > If this is really a parameter that can change per frame, and that the
> > _muxer_ needs this information (per packet I suppose), a side data
> > signaling the field order should be added. mpegtsenc.c doesn't seem to
> > read field_order at all, though.
> >
> > So I remain confused.  
> 
> Yes, I agree that the approach of changing field_order for every frame 
> is not the right way to go, but this code was already existing.  A 
> future patch could undertake the task of handling interlaced video in a 
> more proper way, and I wrote a TODO comment to that point.

I don't understand that comment either. As I already said,
do_video_out() has to be _necessarily_ called before the muxer can be
initialized (aka writing headers).
Aaron Levinson May 6, 2017, 4:59 a.m.
On 5/5/2017 2:26 AM, wm4 wrote:
> On Fri, 5 May 2017 01:11:17 -0700
> Aaron Levinson <alevinsn@aracnet.com> wrote:
>
>>> As I said on IRC, I'm skeptic against this, but I'm also not sure
>>> whether I understand the situation.
>>>
>>> First, your change seems a bit fragile, and almost relies on
>>> coincidences. This could easily break in the future. (And if we add a
>>> FATE test, it would probably hit random people trying to change
>>> ffmpeg.c, and would cause them more work.)
>>
>> I guess I don't understand why you think this change is fragile, relies
>> on coincidences, and could easily break in the future.
>
> Well, the implemented and working logic is that ffmpeg.c waits until it
> has encoded packets for every stream, and then writes the file headers.
> That should work pretty well to let metadata naturally "flow" through
> decoder, filters, and encoder. But your case doesn't fit right into it,
> so you apparently have to make it somehow call do_video_out() more.
> (Also a thing I don't understand - do_video_out() should have been
> called at least once to encode a video packet.)

That's not how ffmpeg.c behaves today.  It writes the file headers 
before it has any encoded packets for the _last_ output stream that is 
initialized.  With my change, I make sure that the file headers aren't 
written until after it has had an opportunity to generate encoded 
packets for at least one input frame for all output streams, although 
there are some FATE test cases in which input frames aren't ready yet by 
this point, which explains why the existence of the fallback in my patch 
is necessary.

It doesn't call do_video_out() any more times with my patch than prior 
to my patch.  It just changes the order of when check_init_output_file() 
is called with respect to do_video_out() (and do_audio_out()).

> And looking at how field_order is set, the existing code doesn't make
> sense in the first place.
>
> So maybe it would be better to fix the actual problem, instead of
> adding another workaround.
>
> At least that's my thinking.
>
>>  The existing
>> code is already fragile.
>
> Indeed. That (and me being sleep deprived) probably contributes to that
> I just don't understand your fix.
>
> Also feel free to ignore me - I'm neither maintainer of ffmpeg.c nor an
> authority on it.
>
>>  As I indicated in previous e-mails on this
>> topic, the behavior of the current code can change depending on timing
>> when there is more than one stream.  This patch should make things more
>> deterministic.  And, I do think it is appropriate to add an interlaced
>> video decoding FATE test, although there is no point to doing so until
>> this bug is fixed, as the current behavior is broken.
>>
>>> Looking at the current do_video_out() function (which btw. is way too
>>> long and also has broken indentation), I see the following line:
>>>
>>>   mux_par->field_order = AV_FIELD_PROGRESSIVE;
>>>
>>> (and similar assignments to field_order to set interlaced modes)
>>>
>>> This is apparently run on every frame. This looks pretty wrong to me.
>>> It should _never_ change the codecpar after headers were written. This
>>> is simply invalid API use.
>>>
>>> If this is really a parameter that can change per frame, and that the
>>> _muxer_ needs this information (per packet I suppose), a side data
>>> signaling the field order should be added. mpegtsenc.c doesn't seem to
>>> read field_order at all, though.
>>>
>>> So I remain confused.
>>
>> Yes, I agree that the approach of changing field_order for every frame
>> is not the right way to go, but this code was already existing.  A
>> future patch could undertake the task of handling interlaced video in a
>> more proper way, and I wrote a TODO comment to that point.
>
> I don't understand that comment either. As I already said,
> do_video_out() has to be _necessarily_ called before the muxer can be
> initialized (aka writing headers).

The way the code is currently structured, do_video_out() is called by 
reap_filters() after it calls init_output_stream(), and it is 
init_output_stream() that makes the call to check_init_output_file(), 
which is where the call to avformat_write_header() is done. 
reap_filters() is the only function that calls do_video_out(), and I 
think it is pretty clear that this will be done after it calls 
init_output_stream() from looking at the source code.

My TODO has to do with setting up the field order based on header 
information, rather than waiting till it gets the first input frame. 
That's the theory at least, but I don't currently know how to do that in 
practice within ffmpeg.c.

Aaron Levinson
Michael Niedermayer May 9, 2017, 6:56 p.m.
On Thu, May 04, 2017 at 11:46:30PM -0700, Aaron Levinson wrote:
> 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 <mpegts file> -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 <alevinsn@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@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()


>  
> @@ -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.

thx

PS: confirmed that interlaced information appears with this patch
that was not there before

[...]
Aaron Levinson May 12, 2017, 8:19 p.m.
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@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@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
Michael Niedermayer May 13, 2017, 12:51 a.m.
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@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@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.

[...]
Aaron Levinson May 13, 2017, 1:22 a.m.
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@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@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

Patch hide | download patch | download mbox

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;
     }