> From: Devin Heitmueller <devin.heitmueller at ltnglobal.com>
> Sent: Tuesday, June 9, 2020 23:47
> To: FFmpeg development discussions and patches <ffmpeg-
> devel at ffmpeg.org>
> Cc: Fu, Linjie <linjie.fu at 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 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,
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