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

[Devin Heitmueller]

On Tue, Jun 9, 2020 at 4:53 AM Linjie Fu <linjie.fu at intel.com> wrote:
>
> Signed-off-by: Linjie Fu <linjie.fu at 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

-- 
Devin Heitmueller, Senior Software Engineer
LTN Global Communications
o: +1 (301) 363-1001
w: https://ltnglobal.com  e: devin.heitmueller at ltnglobal.com