diff mbox

[FFmpeg-devel,1/4] ffmpeg: Flush output BSFs when encode reaches EOF

Message ID 20170618220926.24656-1-sw@jkqxz.net
State New
Headers show

Commit Message

Mark Thompson June 18, 2017, 10:09 p.m. UTC
Before this, output bitstream filters would never see EOF and
therefore would not be able to flush any delayed packets.

(cherry picked from commit f64d1100a54d12c78ce436181bb64229c56da6b3)
---
 ffmpeg.c | 30 +++++++++++++++++++-----------
 1 file changed, 19 insertions(+), 11 deletions(-)

Comments

wm4 June 19, 2017, 8:21 a.m. UTC | #1
On Sun, 18 Jun 2017 23:09:23 +0100
Mark Thompson <sw@jkqxz.net> wrote:

> Before this, output bitstream filters would never see EOF and
> therefore would not be able to flush any delayed packets.
> 
> (cherry picked from commit f64d1100a54d12c78ce436181bb64229c56da6b3)
> ---
>  ffmpeg.c | 30 +++++++++++++++++++-----------
>  1 file changed, 19 insertions(+), 11 deletions(-)
> 
> diff --git a/ffmpeg.c b/ffmpeg.c
> index 6170bd453c..f265980fdd 100644
> --- a/ffmpeg.c
> +++ b/ffmpeg.c
> @@ -814,7 +814,8 @@ static void close_output_stream(OutputStream *ost)
>      }
>  }
>  
> -static void output_packet(OutputFile *of, AVPacket *pkt, OutputStream *ost)
> +static void output_packet(OutputFile *of, AVPacket *pkt,
> +                          OutputStream *ost, int eof)
>  {
>      int ret = 0;
>  
> @@ -822,10 +823,11 @@ static void output_packet(OutputFile *of, AVPacket *pkt, OutputStream *ost)
>      if (ost->nb_bitstream_filters) {
>          int idx;
>  
> -        ret = av_bsf_send_packet(ost->bsf_ctx[0], pkt);
> +        ret = av_bsf_send_packet(ost->bsf_ctx[0], eof ? NULL : pkt);
>          if (ret < 0)
>              goto finish;
>  
> +        eof = 0;
>          idx = 1;
>          while (idx) {
>              /* get a packet from the previous filter up the chain */
> @@ -834,19 +836,24 @@ static void output_packet(OutputFile *of, AVPacket *pkt, OutputStream *ost)
>                  ret = 0;
>                  idx--;
>                  continue;
> +            } else if (ret == AVERROR_EOF) {
> +                eof = 1;
>              } else if (ret < 0)
>                  goto finish;
>  
>              /* send it to the next filter down the chain or to the muxer */
>              if (idx < ost->nb_bitstream_filters) {
> -                ret = av_bsf_send_packet(ost->bsf_ctx[idx], pkt);
> +                ret = av_bsf_send_packet(ost->bsf_ctx[idx], eof ? NULL : pkt);
>                  if (ret < 0)
>                      goto finish;
>                  idx++;
> -            } else
> +                eof = 0;
> +            } else if (eof)
> +                goto finish;
> +            else
>                  write_packet(of, pkt, ost, 0);
>          }
> -    } else
> +    } else if (!eof)
>          write_packet(of, pkt, ost, 0);
>  
>  finish:
> @@ -922,7 +929,7 @@ static void do_audio_out(OutputFile *of, OutputStream *ost,
>                     av_ts2str(pkt.dts), av_ts2timestr(pkt.dts, &enc->time_base));
>          }
>  
> -        output_packet(of, &pkt, ost);
> +        output_packet(of, &pkt, ost, 0);
>      }
>  
>      return;
> @@ -1010,7 +1017,7 @@ static void do_subtitle_out(OutputFile *of,
>                  pkt.pts += av_rescale_q(sub->end_display_time, (AVRational){ 1, 1000 }, ost->mux_timebase);
>          }
>          pkt.dts = pkt.pts;
> -        output_packet(of, &pkt, ost);
> +        output_packet(of, &pkt, ost, 0);
>      }
>  }
>  
> @@ -1196,7 +1203,7 @@ static void do_video_out(OutputFile *of,
>          pkt.pts    = av_rescale_q(in_picture->pts, enc->time_base, ost->mux_timebase);
>          pkt.flags |= AV_PKT_FLAG_KEY;
>  
> -        output_packet(of, &pkt, ost);
> +        output_packet(of, &pkt, ost, 0);
>      } else
>  #endif
>      {
> @@ -1299,7 +1306,7 @@ static void do_video_out(OutputFile *of,
>              }
>  
>              frame_size = pkt.size;
> -            output_packet(of, &pkt, ost);
> +            output_packet(of, &pkt, ost, 0);
>  
>              /* if two pass, output log */
>              if (ost->logfile && enc->stats_out) {
> @@ -1930,6 +1937,7 @@ static void flush_encoders(void)
>                      fprintf(ost->logfile, "%s", enc->stats_out);
>                  }
>                  if (ret == AVERROR_EOF) {
> +                    output_packet(of, &pkt, ost, 1);
>                      break;
>                  }
>                  if (ost->finished & MUXER_FINISHED) {
> @@ -1938,7 +1946,7 @@ static void flush_encoders(void)
>                  }
>                  av_packet_rescale_ts(&pkt, enc->time_base, ost->mux_timebase);
>                  pkt_size = pkt.size;
> -                output_packet(of, &pkt, ost);
> +                output_packet(of, &pkt, ost, 0);
>                  if (ost->enc_ctx->codec_type == AVMEDIA_TYPE_VIDEO && vstats_filename) {
>                      do_video_stats(ost, pkt_size);
>                  }
> @@ -2077,7 +2085,7 @@ static void do_streamcopy(InputStream *ist, OutputStream *ost, const AVPacket *p
>      }
>  #endif
>  
> -    output_packet(of, &opkt, ost);
> +    output_packet(of, &opkt, ost, 0);
>  }
>  
>  int guess_input_channel_layout(InputStream *ist)

Not sure why flushing can't just be done with output_packet(of, NULL,
ost)?

But if it works ok I guess (and preferable to having different code
than in Libav).
Michael Niedermayer June 19, 2017, 3:32 p.m. UTC | #2
On Sun, Jun 18, 2017 at 11:09:23PM +0100, Mark Thompson wrote:
> Before this, output bitstream filters would never see EOF and
> therefore would not be able to flush any delayed packets.
> 
> (cherry picked from commit f64d1100a54d12c78ce436181bb64229c56da6b3)
> ---
>  ffmpeg.c | 30 +++++++++++++++++++-----------
>  1 file changed, 19 insertions(+), 11 deletions(-)
> 
> diff --git a/ffmpeg.c b/ffmpeg.c
> index 6170bd453c..f265980fdd 100644
> --- a/ffmpeg.c
> +++ b/ffmpeg.c
> @@ -814,7 +814,8 @@ static void close_output_stream(OutputStream *ost)
>      }
>  }
>  
> -static void output_packet(OutputFile *of, AVPacket *pkt, OutputStream *ost)
> +static void output_packet(OutputFile *of, AVPacket *pkt,
> +                          OutputStream *ost, int eof)
>  {
>      int ret = 0;
>  

please document why pkt==NULL is not used for eof detection and what
the functionn does with pkt. The caller must be aware of it, if it is
not just used as unchanged input that is written

[...]
Mark Thompson June 19, 2017, 9:48 p.m. UTC | #3
On 19/06/17 16:32, Michael Niedermayer wrote:
> On Sun, Jun 18, 2017 at 11:09:23PM +0100, Mark Thompson wrote:
>> Before this, output bitstream filters would never see EOF and
>> therefore would not be able to flush any delayed packets.
>>
>> (cherry picked from commit f64d1100a54d12c78ce436181bb64229c56da6b3)
>> ---
>>  ffmpeg.c | 30 +++++++++++++++++++-----------
>>  1 file changed, 19 insertions(+), 11 deletions(-)
>>
>> diff --git a/ffmpeg.c b/ffmpeg.c
>> index 6170bd453c..f265980fdd 100644
>> --- a/ffmpeg.c
>> +++ b/ffmpeg.c
>> @@ -814,7 +814,8 @@ static void close_output_stream(OutputStream *ost)
>>      }
>>  }
>>  
>> -static void output_packet(OutputFile *of, AVPacket *pkt, OutputStream *ost)
>> +static void output_packet(OutputFile *of, AVPacket *pkt,
>> +                          OutputStream *ost, int eof)
>>  {
>>      int ret = 0;
>>  
> 
> please document why pkt==NULL is not used for eof detection and what
> the functionn does with pkt. The caller must be aware of it, if it is
> not just used as unchanged input that is written

"""
/*
 * Send a single packet to the output, applying any bitstream filters
 * associated with the output stream.  This may result in any number
 * of packets actually being written, depending on what bitstream
 * filters are applied.  The supplied packet is consumed and will be
 * empty when this function returns.
 *
 * If eof is set, instead indicate EOF to all bitstream filters and
 * therefore flush any delayed packets to the output.  An empty packet
 * must be supplied in this case.
 */
static void output_packet(OutputFile *of, AVPacket *pkt,
                          OutputStream *ost, int eof)
"""

?
Michael Niedermayer June 24, 2017, 10:29 a.m. UTC | #4
On Mon, Jun 19, 2017 at 10:48:15PM +0100, Mark Thompson wrote:
> On 19/06/17 16:32, Michael Niedermayer wrote:
> > On Sun, Jun 18, 2017 at 11:09:23PM +0100, Mark Thompson wrote:
> >> Before this, output bitstream filters would never see EOF and
> >> therefore would not be able to flush any delayed packets.
> >>
> >> (cherry picked from commit f64d1100a54d12c78ce436181bb64229c56da6b3)
> >> ---
> >>  ffmpeg.c | 30 +++++++++++++++++++-----------
> >>  1 file changed, 19 insertions(+), 11 deletions(-)
> >>
> >> diff --git a/ffmpeg.c b/ffmpeg.c
> >> index 6170bd453c..f265980fdd 100644
> >> --- a/ffmpeg.c
> >> +++ b/ffmpeg.c
> >> @@ -814,7 +814,8 @@ static void close_output_stream(OutputStream *ost)
> >>      }
> >>  }
> >>  
> >> -static void output_packet(OutputFile *of, AVPacket *pkt, OutputStream *ost)
> >> +static void output_packet(OutputFile *of, AVPacket *pkt,
> >> +                          OutputStream *ost, int eof)
> >>  {
> >>      int ret = 0;
> >>  
> > 
> > please document why pkt==NULL is not used for eof detection and what
> > the functionn does with pkt. The caller must be aware of it, if it is
> > not just used as unchanged input that is written
> 
> """
> /*
>  * Send a single packet to the output, applying any bitstream filters
>  * associated with the output stream.  This may result in any number
>  * of packets actually being written, depending on what bitstream
>  * filters are applied.  The supplied packet is consumed and will be
>  * empty when this function returns.
>  *
>  * If eof is set, instead indicate EOF to all bitstream filters and
>  * therefore flush any delayed packets to the output.  An empty packet
>  * must be supplied in this case.
>  */
> static void output_packet(OutputFile *of, AVPacket *pkt,
>                           OutputStream *ost, int eof)

LGTM

maybe "empty packet" could be clarified.

thx

[...]
Mark Thompson June 24, 2017, 4:44 p.m. UTC | #5
On 24/06/17 11:29, Michael Niedermayer wrote:
> On Mon, Jun 19, 2017 at 10:48:15PM +0100, Mark Thompson wrote:
>> On 19/06/17 16:32, Michael Niedermayer wrote:
>>> On Sun, Jun 18, 2017 at 11:09:23PM +0100, Mark Thompson wrote:
>>>> Before this, output bitstream filters would never see EOF and
>>>> therefore would not be able to flush any delayed packets.
>>>>
>>>> (cherry picked from commit f64d1100a54d12c78ce436181bb64229c56da6b3)
>>>> ---
>>>>  ffmpeg.c | 30 +++++++++++++++++++-----------
>>>>  1 file changed, 19 insertions(+), 11 deletions(-)
>>>>
>>>> diff --git a/ffmpeg.c b/ffmpeg.c
>>>> index 6170bd453c..f265980fdd 100644
>>>> --- a/ffmpeg.c
>>>> +++ b/ffmpeg.c
>>>> @@ -814,7 +814,8 @@ static void close_output_stream(OutputStream *ost)
>>>>      }
>>>>  }
>>>>  
>>>> -static void output_packet(OutputFile *of, AVPacket *pkt, OutputStream *ost)
>>>> +static void output_packet(OutputFile *of, AVPacket *pkt,
>>>> +                          OutputStream *ost, int eof)
>>>>  {
>>>>      int ret = 0;
>>>>  
>>>
>>> please document why pkt==NULL is not used for eof detection and what
>>> the functionn does with pkt. The caller must be aware of it, if it is
>>> not just used as unchanged input that is written
>>
>> """
>> /*
>>  * Send a single packet to the output, applying any bitstream filters
>>  * associated with the output stream.  This may result in any number
>>  * of packets actually being written, depending on what bitstream
>>  * filters are applied.  The supplied packet is consumed and will be
>>  * empty when this function returns.
>>  *
>>  * If eof is set, instead indicate EOF to all bitstream filters and
>>  * therefore flush any delayed packets to the output.  An empty packet
>>  * must be supplied in this case.
>>  */
>> static void output_packet(OutputFile *of, AVPacket *pkt,
>>                           OutputStream *ost, int eof)
> 
> LGTM
> 
> maybe "empty packet" could be clarified.

Set applied with this modified as noted on IRC.

Thanks,

- Mark
diff mbox

Patch

diff --git a/ffmpeg.c b/ffmpeg.c
index 6170bd453c..f265980fdd 100644
--- a/ffmpeg.c
+++ b/ffmpeg.c
@@ -814,7 +814,8 @@  static void close_output_stream(OutputStream *ost)
     }
 }
 
-static void output_packet(OutputFile *of, AVPacket *pkt, OutputStream *ost)
+static void output_packet(OutputFile *of, AVPacket *pkt,
+                          OutputStream *ost, int eof)
 {
     int ret = 0;
 
@@ -822,10 +823,11 @@  static void output_packet(OutputFile *of, AVPacket *pkt, OutputStream *ost)
     if (ost->nb_bitstream_filters) {
         int idx;
 
-        ret = av_bsf_send_packet(ost->bsf_ctx[0], pkt);
+        ret = av_bsf_send_packet(ost->bsf_ctx[0], eof ? NULL : pkt);
         if (ret < 0)
             goto finish;
 
+        eof = 0;
         idx = 1;
         while (idx) {
             /* get a packet from the previous filter up the chain */
@@ -834,19 +836,24 @@  static void output_packet(OutputFile *of, AVPacket *pkt, OutputStream *ost)
                 ret = 0;
                 idx--;
                 continue;
+            } else if (ret == AVERROR_EOF) {
+                eof = 1;
             } else if (ret < 0)
                 goto finish;
 
             /* send it to the next filter down the chain or to the muxer */
             if (idx < ost->nb_bitstream_filters) {
-                ret = av_bsf_send_packet(ost->bsf_ctx[idx], pkt);
+                ret = av_bsf_send_packet(ost->bsf_ctx[idx], eof ? NULL : pkt);
                 if (ret < 0)
                     goto finish;
                 idx++;
-            } else
+                eof = 0;
+            } else if (eof)
+                goto finish;
+            else
                 write_packet(of, pkt, ost, 0);
         }
-    } else
+    } else if (!eof)
         write_packet(of, pkt, ost, 0);
 
 finish:
@@ -922,7 +929,7 @@  static void do_audio_out(OutputFile *of, OutputStream *ost,
                    av_ts2str(pkt.dts), av_ts2timestr(pkt.dts, &enc->time_base));
         }
 
-        output_packet(of, &pkt, ost);
+        output_packet(of, &pkt, ost, 0);
     }
 
     return;
@@ -1010,7 +1017,7 @@  static void do_subtitle_out(OutputFile *of,
                 pkt.pts += av_rescale_q(sub->end_display_time, (AVRational){ 1, 1000 }, ost->mux_timebase);
         }
         pkt.dts = pkt.pts;
-        output_packet(of, &pkt, ost);
+        output_packet(of, &pkt, ost, 0);
     }
 }
 
@@ -1196,7 +1203,7 @@  static void do_video_out(OutputFile *of,
         pkt.pts    = av_rescale_q(in_picture->pts, enc->time_base, ost->mux_timebase);
         pkt.flags |= AV_PKT_FLAG_KEY;
 
-        output_packet(of, &pkt, ost);
+        output_packet(of, &pkt, ost, 0);
     } else
 #endif
     {
@@ -1299,7 +1306,7 @@  static void do_video_out(OutputFile *of,
             }
 
             frame_size = pkt.size;
-            output_packet(of, &pkt, ost);
+            output_packet(of, &pkt, ost, 0);
 
             /* if two pass, output log */
             if (ost->logfile && enc->stats_out) {
@@ -1930,6 +1937,7 @@  static void flush_encoders(void)
                     fprintf(ost->logfile, "%s", enc->stats_out);
                 }
                 if (ret == AVERROR_EOF) {
+                    output_packet(of, &pkt, ost, 1);
                     break;
                 }
                 if (ost->finished & MUXER_FINISHED) {
@@ -1938,7 +1946,7 @@  static void flush_encoders(void)
                 }
                 av_packet_rescale_ts(&pkt, enc->time_base, ost->mux_timebase);
                 pkt_size = pkt.size;
-                output_packet(of, &pkt, ost);
+                output_packet(of, &pkt, ost, 0);
                 if (ost->enc_ctx->codec_type == AVMEDIA_TYPE_VIDEO && vstats_filename) {
                     do_video_stats(ost, pkt_size);
                 }
@@ -2077,7 +2085,7 @@  static void do_streamcopy(InputStream *ist, OutputStream *ost, const AVPacket *p
     }
 #endif
 
-    output_packet(of, &opkt, ost);
+    output_packet(of, &opkt, ost, 0);
 }
 
 int guess_input_channel_layout(InputStream *ist)