| Message ID | 1591692526-18839-1-git-send-email-linjie.fu@intel.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | None | expand |
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
> 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
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) >
> 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
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.
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,
> 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
> 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
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,
> 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
Fu, Linjie (12020-06-11):
> If rawvideo here means .h264, attached the output file produced by libx264:
Now, rawvideo means rawvideo.
Regards,
> 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 --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)
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(+)