diff mbox series

[FFmpeg-devel,4/5] fftools/ffmpeg: flush and recreate encoder instance if resolution changes

Message ID 1591692526-18839-1-git-send-email-linjie.fu@intel.com
State Superseded
Headers show
Series Untitled series #1450
Related show

Commit Message

Fu, Linjie June 9, 2020, 8:48 a.m. UTC
Signed-off-by: Linjie Fu <linjie.fu@intel.com>
---
Should be squashed with:
https://patchwork.ffmpeg.org/project/ffmpeg/list/?series=1434

 fftools/ffmpeg.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

Comments

Devin Heitmueller June 9, 2020, 3:47 p.m. UTC | #1
On Tue, Jun 9, 2020 at 4:53 AM Linjie Fu <linjie.fu@intel.com> wrote:
>
> Signed-off-by: Linjie Fu <linjie.fu@intel.com>
> ---
> Should be squashed with:
> https://patchwork.ffmpeg.org/project/ffmpeg/list/?series=1434
>
>  fftools/ffmpeg.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
>
> diff --git a/fftools/ffmpeg.c b/fftools/ffmpeg.c
> index 5859781..8cdd532 100644
> --- a/fftools/ffmpeg.c
> +++ b/fftools/ffmpeg.c
> @@ -130,6 +130,7 @@ static void do_video_stats(OutputStream *ost, int frame_size);
>  static BenchmarkTimeStamps get_benchmark_time_stamps(void);
>  static int64_t getmaxrss(void);
>  static int ifilter_has_all_input_formats(FilterGraph *fg);
> +static void flush_encoders(void);
>
>  static int run_as_daemon  = 0;
>  static int nb_frames_dup = 0;
> @@ -1058,11 +1059,21 @@ static void do_video_out(OutputFile *of,
>
>      if (next_picture && (enc->width != next_picture->width ||
>                           enc->height != next_picture->height)) {
> +        flush_encoders();
> +        avcodec_flush_buffers(enc);
>          if (!(enc->codec->capabilities & AV_CODEC_CAP_VARIABLE_DIMENSIONS)) {
>              av_log(NULL, AV_LOG_ERROR, "Variable dimension encoding "
>                              "is not supported by %s.\n", enc->codec->name);
>              goto error;
>          }
> +
> +        enc->width  = next_picture->width;
> +        enc->height = next_picture->height;

Perhaps from a workflow standpoint it makes more sense to move the
code which changes the encoder parameters to after where you close the
existing encoder (i.e. between the close and init calls).  I can't
think of a specific case where this might break a downstream encoder,
but it seems like a good practice to only have the parameters applied
to the new encoder instance.

> +
> +        if (enc->codec->close(enc) < 0)
> +            goto error;
> +        if (enc->codec->init(enc) < 0)
> +            goto error;
>      }
>
>      if (ost->source_index >= 0)

In general do we really think this is a safe thing to do?  Does
something also need to be propagated to the output as well?  I know
that this would break use cases like the decklink output where the
frame resolution suddenly changes in the middle of the stream without
calling the output's write_header() routine.

Devin
Fu, Linjie June 10, 2020, 3:12 a.m. UTC | #2
> From: Devin Heitmueller <devin.heitmueller@ltnglobal.com>
> Sent: Tuesday, June 9, 2020 23:47
> To: FFmpeg development discussions and patches <ffmpeg-
> devel@ffmpeg.org>
> Cc: Fu, Linjie <linjie.fu@intel.com>
> Subject: Re: [FFmpeg-devel] [PATCH 4/5] fftools/ffmpeg: flush and recreate
> encoder instance if resolution changes
> 
> On Tue, Jun 9, 2020 at 4:53 AM Linjie Fu <linjie.fu@intel.com> wrote:
> >
> > Signed-off-by: Linjie Fu <linjie.fu@intel.com>
> > ---
> > Should be squashed with:
> > https://patchwork.ffmpeg.org/project/ffmpeg/list/?series=1434
> >
> >  fftools/ffmpeg.c | 11 +++++++++++
> >  1 file changed, 11 insertions(+)
> >
> > diff --git a/fftools/ffmpeg.c b/fftools/ffmpeg.c
> > index 5859781..8cdd532 100644
> > --- a/fftools/ffmpeg.c
> > +++ b/fftools/ffmpeg.c
> > @@ -130,6 +130,7 @@ static void do_video_stats(OutputStream *ost, int
> frame_size);
> >  static BenchmarkTimeStamps get_benchmark_time_stamps(void);
> >  static int64_t getmaxrss(void);
> >  static int ifilter_has_all_input_formats(FilterGraph *fg);
> > +static void flush_encoders(void);
> >
> >  static int run_as_daemon  = 0;
> >  static int nb_frames_dup = 0;
> > @@ -1058,11 +1059,21 @@ static void do_video_out(OutputFile *of,
> >
> >      if (next_picture && (enc->width != next_picture->width ||
> >                           enc->height != next_picture->height)) {
> > +        flush_encoders();
> > +        avcodec_flush_buffers(enc);
> >          if (!(enc->codec->capabilities &
> AV_CODEC_CAP_VARIABLE_DIMENSIONS)) {
> >              av_log(NULL, AV_LOG_ERROR, "Variable dimension encoding "
> >                              "is not supported by %s.\n", enc->codec->name);
> >              goto error;
> >          }
> > +
> > +        enc->width  = next_picture->width;
> > +        enc->height = next_picture->height;
> 
> Perhaps from a workflow standpoint it makes more sense to move the
> code which changes the encoder parameters to after where you close the
> existing encoder (i.e. between the close and init calls).  I can't
> think of a specific case where this might break a downstream encoder,
That's the reason I simply set the width/height ahead.
> but it seems like a good practice to only have the parameters applied
> to the new encoder instance.
Indeed, fixed locally.

> > +
> > +        if (enc->codec->close(enc) < 0)
> > +            goto error;
> > +        if (enc->codec->init(enc) < 0)
> > +            goto error;
> >      }
> >
> >      if (ost->source_index >= 0)
> 
> In general do we really think this is a safe thing to do?  Does
> something also need to be propagated to the output as well?  I know
> that this would break use cases like the decklink output where the
> frame resolution suddenly changes in the middle of the stream without
> calling the output's write_header() routine.

Yes, noticed the constraints in sequence header and container.

Since resolution changing is allowed in single bitstream, one global header is not
enough anymore as Nicolas has pointed out in [1].

Hence as to the container level,  for the formats with AVFMT_GLOBALHEADER flag,
we should place sps/pps information in key frame instead of in extradata.
(e.g. disable AV_CODEC_FLAG_GLOBAL_HEADER)

-    if (oc->oformat->flags & AVFMT_GLOBALHEADER)
+    if (oc->oformat->flags & AVFMT_GLOBALHEADER && ost->autoscale)
         ost->enc_ctx->flags |= AV_CODEC_FLAG_GLOBAL_HEADER; 

Verified this works, at least for containers like mp4.

- Linjie

[1] https://patchwork.ffmpeg.org/project/ffmpeg/patch/1591606685-4450-1-git-send-email-linjie.fu@intel.com/#55293
James Almer June 10, 2020, 4:22 a.m. UTC | #3
On 6/9/2020 5:48 AM, Linjie Fu wrote:
> Signed-off-by: Linjie Fu <linjie.fu@intel.com>
> ---
> Should be squashed with:
> https://patchwork.ffmpeg.org/project/ffmpeg/list/?series=1434
> 
>  fftools/ffmpeg.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/fftools/ffmpeg.c b/fftools/ffmpeg.c
> index 5859781..8cdd532 100644
> --- a/fftools/ffmpeg.c
> +++ b/fftools/ffmpeg.c
> @@ -130,6 +130,7 @@ static void do_video_stats(OutputStream *ost, int frame_size);
>  static BenchmarkTimeStamps get_benchmark_time_stamps(void);
>  static int64_t getmaxrss(void);
>  static int ifilter_has_all_input_formats(FilterGraph *fg);
> +static void flush_encoders(void);
>  
>  static int run_as_daemon  = 0;
>  static int nb_frames_dup = 0;
> @@ -1058,11 +1059,21 @@ static void do_video_out(OutputFile *of,
>  
>      if (next_picture && (enc->width != next_picture->width ||
>                           enc->height != next_picture->height)) {
> +        flush_encoders();
> +        avcodec_flush_buffers(enc);

This only works for a limited amount of encoders that set the
AV_CODEC_CAP_ENCODER_FLUSH codec capability, and it's a no-op after
emitting a warning otherwise.

>          if (!(enc->codec->capabilities & AV_CODEC_CAP_VARIABLE_DIMENSIONS)) {
>              av_log(NULL, AV_LOG_ERROR, "Variable dimension encoding "
>                              "is not supported by %s.\n", enc->codec->name);
>              goto error;
>          }
> +
> +        enc->width  = next_picture->width;
> +        enc->height = next_picture->height;
> +
> +        if (enc->codec->close(enc) < 0)
> +            goto error;
> +        if (enc->codec->init(enc) < 0)
> +            goto error;
>      }
>  
>      if (ost->source_index >= 0)
>
Fu, Linjie June 10, 2020, 6:12 a.m. UTC | #4
> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of
> James Almer
> Sent: Wednesday, June 10, 2020 12:22
> To: ffmpeg-devel@ffmpeg.org
> Subject: Re: [FFmpeg-devel] [PATCH 4/5] fftools/ffmpeg: flush and recreate
> encoder instance if resolution changes
> 
> On 6/9/2020 5:48 AM, Linjie Fu wrote:
> > Signed-off-by: Linjie Fu <linjie.fu@intel.com>
> > ---
> > Should be squashed with:
> > https://patchwork.ffmpeg.org/project/ffmpeg/list/?series=1434
> >
> >  fftools/ffmpeg.c | 11 +++++++++++
> >  1 file changed, 11 insertions(+)
> >
> > diff --git a/fftools/ffmpeg.c b/fftools/ffmpeg.c
> > index 5859781..8cdd532 100644
> > --- a/fftools/ffmpeg.c
> > +++ b/fftools/ffmpeg.c
> > @@ -130,6 +130,7 @@ static void do_video_stats(OutputStream *ost, int
> frame_size);
> >  static BenchmarkTimeStamps get_benchmark_time_stamps(void);
> >  static int64_t getmaxrss(void);
> >  static int ifilter_has_all_input_formats(FilterGraph *fg);
> > +static void flush_encoders(void);
> >
> >  static int run_as_daemon  = 0;
> >  static int nb_frames_dup = 0;
> > @@ -1058,11 +1059,21 @@ static void do_video_out(OutputFile *of,
> >
> >      if (next_picture && (enc->width != next_picture->width ||
> >                           enc->height != next_picture->height)) {
> > +        flush_encoders();
> > +        avcodec_flush_buffers(enc);
> 
> This only works for a limited amount of encoders that set the
> AV_CODEC_CAP_ENCODER_FLUSH codec capability, and it's a no-op after
> emitting a warning otherwise.

Yes, hence would like to declare VARIABLE_DIMENSIONS and ENCODER_FLUSH
capability for encoders as well:
https://patchwork.ffmpeg.org/project/ffmpeg/patch/1591692568-19385-1-git-send-email-linjie.fu@intel.com/

 - Linjie
Anton Khirnov June 10, 2020, 8:31 a.m. UTC | #5
Quoting Linjie Fu (2020-06-09 10:48:46)
> Signed-off-by: Linjie Fu <linjie.fu@intel.com>
> ---
> Should be squashed with:
> https://patchwork.ffmpeg.org/project/ffmpeg/list/?series=1434
> 
>  fftools/ffmpeg.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/fftools/ffmpeg.c b/fftools/ffmpeg.c
> index 5859781..8cdd532 100644
> --- a/fftools/ffmpeg.c
> +++ b/fftools/ffmpeg.c
> @@ -130,6 +130,7 @@ static void do_video_stats(OutputStream *ost, int frame_size);
>  static BenchmarkTimeStamps get_benchmark_time_stamps(void);
>  static int64_t getmaxrss(void);
>  static int ifilter_has_all_input_formats(FilterGraph *fg);
> +static void flush_encoders(void);
>  
>  static int run_as_daemon  = 0;
>  static int nb_frames_dup = 0;
> @@ -1058,11 +1059,21 @@ static void do_video_out(OutputFile *of,
>  
>      if (next_picture && (enc->width != next_picture->width ||
>                           enc->height != next_picture->height)) {
> +        flush_encoders();
> +        avcodec_flush_buffers(enc);
>          if (!(enc->codec->capabilities & AV_CODEC_CAP_VARIABLE_DIMENSIONS)) {
>              av_log(NULL, AV_LOG_ERROR, "Variable dimension encoding "
>                              "is not supported by %s.\n", enc->codec->name);
>              goto error;
>          }
> +
> +        enc->width  = next_picture->width;
> +        enc->height = next_picture->height;
> +
> +        if (enc->codec->close(enc) < 0)
> +            goto error;
> +        if (enc->codec->init(enc) < 0)
> +            goto error;

Absolutely not.
Those are private fields, they must not be accessed by libavcodec
callers.

What I meant was freeing the encoder and creating a completely new
encoder instance.
Nicolas George June 10, 2020, 11:53 a.m. UTC | #6
Linjie Fu (12020-06-09):
> Signed-off-by: Linjie Fu <linjie.fu@intel.com>
> ---
> Should be squashed with:
> https://patchwork.ffmpeg.org/project/ffmpeg/list/?series=1434

Apart from the issue of accessing non-public fields, this needs to be
intensively tested. If it allows to create unplayable files, then it is
not acceptable. It should be tested with codecs that use extradata (x264
for example) and also with rawvideo.

Regards,
Fu, Linjie June 11, 2020, 3:51 p.m. UTC | #7
> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of
> Anton Khirnov
> Sent: Wednesday, June 10, 2020 16:31
> To: FFmpeg development discussions and patches <ffmpeg-
> devel@ffmpeg.org>
> Subject: Re: [FFmpeg-devel] [PATCH 4/5] fftools/ffmpeg: flush and recreate
> encoder instance if resolution changes
> 
> Quoting Linjie Fu (2020-06-09 10:48:46)
> > Signed-off-by: Linjie Fu <linjie.fu@intel.com>
> > ---
> > Should be squashed with:
> > https://patchwork.ffmpeg.org/project/ffmpeg/list/?series=1434
> >
> >  fftools/ffmpeg.c | 11 +++++++++++
> >  1 file changed, 11 insertions(+)
> >
> > diff --git a/fftools/ffmpeg.c b/fftools/ffmpeg.c
> > index 5859781..8cdd532 100644
> > --- a/fftools/ffmpeg.c
> > +++ b/fftools/ffmpeg.c
> > @@ -130,6 +130,7 @@ static void do_video_stats(OutputStream *ost, int
> frame_size);
> >  static BenchmarkTimeStamps get_benchmark_time_stamps(void);
> >  static int64_t getmaxrss(void);
> >  static int ifilter_has_all_input_formats(FilterGraph *fg);
> > +static void flush_encoders(void);
> >
> >  static int run_as_daemon  = 0;
> >  static int nb_frames_dup = 0;
> > @@ -1058,11 +1059,21 @@ static void do_video_out(OutputFile *of,
> >
> >      if (next_picture && (enc->width != next_picture->width ||
> >                           enc->height != next_picture->height)) {
> > +        flush_encoders();
> > +        avcodec_flush_buffers(enc);
> >          if (!(enc->codec->capabilities &
> AV_CODEC_CAP_VARIABLE_DIMENSIONS)) {
> >              av_log(NULL, AV_LOG_ERROR, "Variable dimension encoding "
> >                              "is not supported by %s.\n", enc->codec->name);
> >              goto error;
> >          }
> > +
> > +        enc->width  = next_picture->width;
> > +        enc->height = next_picture->height;
> > +
> > +        if (enc->codec->close(enc) < 0)
> > +            goto error;
> > +        if (enc->codec->init(enc) < 0)
> > +            goto error;
> 
> Absolutely not.
> Those are private fields, they must not be accessed by libavcodec
> callers.
> 
> What I meant was freeing the encoder and creating a completely new
> encoder instance.

Got it and updated, please help to comment, thx.
https://patchwork.ffmpeg.org/project/ffmpeg/list/?series=1470

- Linjie
Fu, Linjie June 11, 2020, 3:54 p.m. UTC | #8
> From: Nicolas George <george@nsup.org>
> Sent: Wednesday, June 10, 2020 19:54
> To: FFmpeg development discussions and patches <ffmpeg-
> devel@ffmpeg.org>
> Cc: Fu, Linjie <linjie.fu@intel.com>
> Subject: Re: [FFmpeg-devel] [PATCH 4/5] fftools/ffmpeg: flush and recreate
> encoder instance if resolution changes
> 
> Linjie Fu (12020-06-09):
> > Signed-off-by: Linjie Fu <linjie.fu@intel.com>
> > ---
> > Should be squashed with:
> > https://patchwork.ffmpeg.org/project/ffmpeg/list/?series=1434
> 
> Apart from the issue of accessing non-public fields, this needs to be
> intensively tested. If it allows to create unplayable files, then it is
> not acceptable. It should be tested with codecs that use extradata (x264
> for example) and also with rawvideo.
> 
Indeed, tested with .mp4 and .h264 for encoder libx264, the results are playable.
(discarding global header if resolution changing is allowed, then we can keep the
Sequence header in each Key frame and make the resolution changing noticeable)

Please help to comment, thx.
https://patchwork.ffmpeg.org/project/ffmpeg/list/?series=1470

- Linjie
Nicolas George June 11, 2020, 4:50 p.m. UTC | #9
Fu, Linjie (12020-06-11):
> Indeed, tested with .mp4 and .h264 for encoder libx264, the results are playable.
> (discarding global header if resolution changing is allowed, then we can keep the
> Sequence header in each Key frame and make the resolution changing noticeable)
> 
> Please help to comment, thx.
> https://patchwork.ffmpeg.org/project/ffmpeg/list/?series=1470

Did it produce a rawvideo stream that can be decoded? I doubt it.

Regards,
Fu, Linjie June 11, 2020, 5:12 p.m. UTC | #10
> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of
> Nicolas George
> Sent: Friday, June 12, 2020 00:50
> To: FFmpeg development discussions and patches <ffmpeg-
> devel@ffmpeg.org>
> Subject: Re: [FFmpeg-devel] [PATCH 4/5] fftools/ffmpeg: flush and recreate
> encoder instance if resolution changes
> 
> Fu, Linjie (12020-06-11):
> > Indeed, tested with .mp4 and .h264 for encoder libx264, the results are
> playable.
> > (discarding global header if resolution changing is allowed, then we can
> keep the
> > Sequence header in each Key frame and make the resolution changing
> noticeable)
> >
> > Please help to comment, thx.
> > https://patchwork.ffmpeg.org/project/ffmpeg/list/?series=1470
> 
> Did it produce a rawvideo stream that can be decoded? I doubt it.
> 
If rawvideo here means .h264, attached the output file produced by libx264:

$ ffmpeg -i reinit-large_420_8-to-small_420_8.h264 -c:v libx264 -autoscale 0 -y large_to_small_recreate_encoder_out.h264

- Linjie
Nicolas George June 11, 2020, 5:52 p.m. UTC | #11
Fu, Linjie (12020-06-11):
> If rawvideo here means .h264, attached the output file produced by libx264:

Now, rawvideo means rawvideo.

Regards,
Fu, Linjie June 12, 2020, 3:17 a.m. UTC | #12
> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of
> Nicolas George
> Sent: Friday, June 12, 2020 01:52
> To: FFmpeg development discussions and patches <ffmpeg-
> devel@ffmpeg.org>
> Subject: Re: [FFmpeg-devel] [PATCH 4/5] fftools/ffmpeg: flush and recreate
> encoder instance if resolution changes
> 
> Fu, Linjie (12020-06-11):
> > If rawvideo here means .h264, attached the output file produced by libx264:
> 
> Now, rawvideo means rawvideo.
> 
For Raw YUV video, it is not able to notify player/user about the resolution changing
since it only contains data information.

The usage for raw video is discussed previously. For now we'd like to use rawvideo encoder
To do some bit-exact compare to make sure the result of decoder is good without scaling. 
(matches the output of other media decoder component like gstreamer)

- Linjie
diff mbox series

Patch

diff --git a/fftools/ffmpeg.c b/fftools/ffmpeg.c
index 5859781..8cdd532 100644
--- a/fftools/ffmpeg.c
+++ b/fftools/ffmpeg.c
@@ -130,6 +130,7 @@  static void do_video_stats(OutputStream *ost, int frame_size);
 static BenchmarkTimeStamps get_benchmark_time_stamps(void);
 static int64_t getmaxrss(void);
 static int ifilter_has_all_input_formats(FilterGraph *fg);
+static void flush_encoders(void);
 
 static int run_as_daemon  = 0;
 static int nb_frames_dup = 0;
@@ -1058,11 +1059,21 @@  static void do_video_out(OutputFile *of,
 
     if (next_picture && (enc->width != next_picture->width ||
                          enc->height != next_picture->height)) {
+        flush_encoders();
+        avcodec_flush_buffers(enc);
         if (!(enc->codec->capabilities & AV_CODEC_CAP_VARIABLE_DIMENSIONS)) {
             av_log(NULL, AV_LOG_ERROR, "Variable dimension encoding "
                             "is not supported by %s.\n", enc->codec->name);
             goto error;
         }
+
+        enc->width  = next_picture->width;
+        enc->height = next_picture->height;
+
+        if (enc->codec->close(enc) < 0)
+            goto error;
+        if (enc->codec->init(enc) < 0)
+            goto error;
     }
 
     if (ost->source_index >= 0)