[FFmpeg-devel,2/2] ffmpeg: Made execution of reap_filters() more deterministic with respect to when headers are written

Submitted by Aaron Levinson on May 12, 2017, 8:28 p.m.

Details

Message ID 72a8bacf-861b-b349-fc77-55f63fceee86@aracnet.com
State New
Headers show

Commit Message

Aaron Levinson May 12, 2017, 8:28 p.m.
Purpose: Made execution of reap_filters() more deterministic with
respect to when headers are written in relationship with the
initialization of output streams and the processing of input audio
and/or video frames.  This change fixes a bug in ffmpeg encountered
when decoding interlaced video.  In some cases, ffmpeg would treat it
as progressive.

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 stream 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 is then immediately followed by a call to
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.

Signed-off-by: Aaron Levinson <alevinsn@aracnet.com>
---
 ffmpeg.c | 43 ++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 40 insertions(+), 3 deletions(-)

Comments

wm4 May 13, 2017, 6:44 a.m.
On Fri, 12 May 2017 13:28:14 -0700
Aaron Levinson <alevinsn@aracnet.com> wrote:

> Purpose: Made execution of reap_filters() more deterministic with
> respect to when headers are written in relationship with the
> initialization of output streams and the processing of input audio
> and/or video frames.  This change fixes a bug in ffmpeg encountered
> when decoding interlaced video.  In some cases, ffmpeg would treat it
> as progressive.
> 
> 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 stream 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 is then immediately followed by a call to
> 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.
> 
> Signed-off-by: Aaron Levinson <alevinsn@aracnet.com>
> ---
>  ffmpeg.c | 43 ++++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 40 insertions(+), 3 deletions(-)
> 
> diff --git a/ffmpeg.c b/ffmpeg.c
> index 3cd45ba665..7b044b068c 100644
> --- a/ffmpeg.c
> +++ b/ffmpeg.c
> @@ -1434,6 +1434,7 @@ static int reap_filters(int flush)
>          AVFilterContext *filter;
>          AVCodecContext *enc = ost->enc_ctx;
>          int ret = 0;
> +        int do_check_init_output_file = 0;
>  
>          if (!ost->filter || !ost->filter->graph->graph)
>              continue;
> @@ -1448,9 +1449,7 @@ static int reap_filters(int flush)
>                  exit_program(1);
>              }
>  
> -            ret = check_init_output_file(of, ost->file_index);
> -            if (ret < 0)
> -                exit_program(1);
> +            do_check_init_output_file = 1;
>          }
>  
>          if (!ost->filtered_frame && !(ost->filtered_frame = av_frame_alloc())) {
> @@ -1526,6 +1525,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 (do_check_init_output_file) {
> +                ret = check_init_output_file(of, ost->file_index);
> +                if (ret < 0)
> +                    exit_program(1);
> +                do_check_init_output_file = 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 (do_check_init_output_file) {
> +            ret = check_init_output_file(of, ost->file_index);
> +            if (ret < 0)
> +                exit_program(1);
>          }
>      }
>  

Doesn't look good to me. The big comment compared with the small
amount of code is indicative that this approach is way too complex and
fragile.
James Almer May 26, 2017, 2:48 p.m.
On 5/13/2017 3:44 AM, wm4 wrote:
> On Fri, 12 May 2017 13:28:14 -0700
> Aaron Levinson <alevinsn@aracnet.com> wrote:
> 
>> Purpose: Made execution of reap_filters() more deterministic with
>> respect to when headers are written in relationship with the
>> initialization of output streams and the processing of input audio
>> and/or video frames.  This change fixes a bug in ffmpeg encountered
>> when decoding interlaced video.  In some cases, ffmpeg would treat it
>> as progressive.
>>
>> 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 stream 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 is then immediately followed by a call to
>> 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.
>>
>> Signed-off-by: Aaron Levinson <alevinsn@aracnet.com>
>> ---
>>  ffmpeg.c | 43 ++++++++++++++++++++++++++++++++++++++++---
>>  1 file changed, 40 insertions(+), 3 deletions(-)
>>
>> diff --git a/ffmpeg.c b/ffmpeg.c
>> index 3cd45ba665..7b044b068c 100644
>> --- a/ffmpeg.c
>> +++ b/ffmpeg.c
>> @@ -1434,6 +1434,7 @@ static int reap_filters(int flush)
>>          AVFilterContext *filter;
>>          AVCodecContext *enc = ost->enc_ctx;
>>          int ret = 0;
>> +        int do_check_init_output_file = 0;
>>  
>>          if (!ost->filter || !ost->filter->graph->graph)
>>              continue;
>> @@ -1448,9 +1449,7 @@ static int reap_filters(int flush)
>>                  exit_program(1);
>>              }
>>  
>> -            ret = check_init_output_file(of, ost->file_index);
>> -            if (ret < 0)
>> -                exit_program(1);
>> +            do_check_init_output_file = 1;
>>          }
>>  
>>          if (!ost->filtered_frame && !(ost->filtered_frame = av_frame_alloc())) {
>> @@ -1526,6 +1525,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 (do_check_init_output_file) {
>> +                ret = check_init_output_file(of, ost->file_index);
>> +                if (ret < 0)
>> +                    exit_program(1);
>> +                do_check_init_output_file = 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 (do_check_init_output_file) {
>> +            ret = check_init_output_file(of, ost->file_index);
>> +            if (ret < 0)
>> +                exit_program(1);
>>          }
>>      }
>>  
> 
> Doesn't look good to me. The big comment compared with the small
> amount of code is indicative that this approach is way too complex and
> fragile.

This patch actually prevents a regression a patch dealing with delayed
header writing i'm intending to post would introduce.

If you think this approach is not optimal then please suggest or write
another with the same results, because this is  header writing issue
that needs fixing.
Michael Niedermayer May 26, 2017, 6:53 p.m.
On Fri, May 12, 2017 at 01:28:14PM -0700, Aaron Levinson wrote:
> Purpose: Made execution of reap_filters() more deterministic with
> respect to when headers are written in relationship with the
> initialization of output streams and the processing of input audio
> and/or video frames.  This change fixes a bug in ffmpeg encountered
> when decoding interlaced video.  In some cases, ffmpeg would treat it
> as progressive.
> 
> 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 stream 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 is then immediately followed by a call to
> 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.
> 
> Signed-off-by: Aaron Levinson <alevinsn@aracnet.com>
> ---
>  ffmpeg.c | 43 ++++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 40 insertions(+), 3 deletions(-)

This breaks

./ffmpeg -itsoffset 2 -re -i matrixbench_mpeg2.mpg -vcodec libx264 -an -t 3 -f rtp rtp://127.0.0.1:17755 > h264.sdp 2> log1 &
sleep 2
./ffmpeg -protocol_whitelist file,rtp,udp -i h264.sdp -y -t 0.9 out-h264.avi

(h264.sdp: Invalid data found when processing input)

for the input see:
http://samples.ffmpeg.org/benchmark/testsuite1/matrixbench_mpeg2.mpg

also increasing sleep to 2.5 or 3 does not make it work, it fails
differently (h264 decoder errors)

[...]

Patch hide | download patch | download mbox

diff --git a/ffmpeg.c b/ffmpeg.c
index 3cd45ba665..7b044b068c 100644
--- a/ffmpeg.c
+++ b/ffmpeg.c
@@ -1434,6 +1434,7 @@  static int reap_filters(int flush)
         AVFilterContext *filter;
         AVCodecContext *enc = ost->enc_ctx;
         int ret = 0;
+        int do_check_init_output_file = 0;
 
         if (!ost->filter || !ost->filter->graph->graph)
             continue;
@@ -1448,9 +1449,7 @@  static int reap_filters(int flush)
                 exit_program(1);
             }
 
-            ret = check_init_output_file(of, ost->file_index);
-            if (ret < 0)
-                exit_program(1);
+            do_check_init_output_file = 1;
         }
 
         if (!ost->filtered_frame && !(ost->filtered_frame = av_frame_alloc())) {
@@ -1526,6 +1525,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 (do_check_init_output_file) {
+                ret = check_init_output_file(of, ost->file_index);
+                if (ret < 0)
+                    exit_program(1);
+                do_check_init_output_file = 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 (do_check_init_output_file) {
+            ret = check_init_output_file(of, ost->file_index);
+            if (ret < 0)
+                exit_program(1);
         }
     }