diff mbox series

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

Message ID 1591890182-22085-1-git-send-email-linjie.fu@intel.com
State New
Headers show
Series Untitled series #1470
Related show

Commit Message

Fu, Linjie June 11, 2020, 3:43 p.m. UTC
Add recreate_encoder_instance() function.

If resolution changing is allowed, discard AV_CODEC_FLAG_GLOBAL_HEADER
even if the avformat/container declares AVFMT_GLOBALHEADER flag. Place
header information in every keyframe instead of single global header.

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     | 40 ++++++++++++++++++++++++++++++++++++++++
 fftools/ffmpeg_opt.c |  2 +-
 2 files changed, 41 insertions(+), 1 deletion(-)

Comments

Nicolas George June 11, 2020, 4:49 p.m. UTC | #1
Linjie Fu (12020-06-11):
> Add recreate_encoder_instance() function.
> 
> If resolution changing is allowed, discard AV_CODEC_FLAG_GLOBAL_HEADER
> even if the avformat/container declares AVFMT_GLOBALHEADER flag. Place
> header information in every keyframe instead of single global header.

Why? How is it valid? If the codec requires global header, we cannot
just ignore them. And if the codec can change resolution, there is no
point in recreating it.

Regards,
Fu, Linjie June 12, 2020, 2:38 a.m. UTC | #2
> From: Nicolas George <george@nsup.org>
> Sent: Friday, June 12, 2020 00:49
> To: FFmpeg development discussions and patches <ffmpeg-
> devel@ffmpeg.org>
> Cc: Fu, Linjie <linjie.fu@intel.com>
> Subject: Re: [FFmpeg-devel] [PATCH v2 4/5] fftools/ffmpeg: flush and
> recreate encoder instance if resolution changes
> 
> Linjie Fu (12020-06-11):
> > Add recreate_encoder_instance() function.
> >
> > If resolution changing is allowed, discard
> AV_CODEC_FLAG_GLOBAL_HEADER
> > even if the avformat/container declares AVFMT_GLOBALHEADER flag.
> Place
> > header information in every keyframe instead of single global header.
> 
> Why? How is it valid? If the codec requires global header, we cannot
> just ignore them. 

IIRC, the global header in extra data is optional in codec level. By storing the
parameter set in global header, it allows reusing of the shared information
and hence is bitrate efficient. Also it's loss robust since it allows parameter
set content to be carried in a more reliable way.(or repeated frequently)

Hence IMHO fallback to store the sequence header in key frame may lose
the advantage of bitrate efficient and loss robust, but it seems to be correct
since it's more fundamental. (And the test result shows it works)

(Please correct if my understanding is not correct)

> And if the codec can change resolution, there is no  point in recreating it.

Agree with this, for these codecs I'd prefer to implement corresponding method
In specific encoder (like libvpx-vp9):
https://patchwork.ffmpeg.org/project/ffmpeg/patch/1565688544-9043-1-git-send-email-linjie.fu@intel.com/

- Linjie
Fu, Linjie June 14, 2020, 4:02 p.m. UTC | #3
> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of Fu,
> Linjie
> Sent: Friday, June 12, 2020 10:39
> To: ffmpeg-devel@ffmpeg.org
> Subject: Re: [FFmpeg-devel] [PATCH v2 4/5] fftools/ffmpeg: flush and
> recreate encoder instance if resolution changes
> 
> > From: Nicolas George <george@nsup.org>
> > Sent: Friday, June 12, 2020 00:49
> > To: FFmpeg development discussions and patches <ffmpeg-
> > devel@ffmpeg.org>
> > Cc: Fu, Linjie <linjie.fu@intel.com>
> > Subject: Re: [FFmpeg-devel] [PATCH v2 4/5] fftools/ffmpeg: flush and
> > recreate encoder instance if resolution changes
> >
> > Linjie Fu (12020-06-11):
> > > Add recreate_encoder_instance() function.
> > >
> > > If resolution changing is allowed, discard
> > AV_CODEC_FLAG_GLOBAL_HEADER
> > > even if the avformat/container declares AVFMT_GLOBALHEADER flag.
> > Place
> > > header information in every keyframe instead of single global header.
> >
> > Why? How is it valid? If the codec requires global header, we cannot
> > just ignore them.
> 
> IIRC, the global header in extra data is optional in codec level. By storing the
> parameter set in global header, it allows reusing of the shared information
> and hence is bitrate efficient. Also it's loss robust since it allows parameter
> set content to be carried in a more reliable way.(or repeated frequently)
> 
> Hence IMHO fallback to store the sequence header in key frame may lose
> the advantage of bitrate efficient and loss robust, but it seems to be correct
> since it's more fundamental. (And the test result shows it works)
> 
> (Please correct if my understanding is not correct)
> 
> > And if the codec can change resolution, there is no  point in recreating it.
> 
> Agree with this, for these codecs I'd prefer to implement corresponding
> method
> In specific encoder (like libvpx-vp9):
> https://patchwork.ffmpeg.org/project/ffmpeg/patch/1565688544-9043-1-
> git-send-email-linjie.fu@intel.com/
> 
Ping for this.

The patch set in series[1] would be the first step to settle down the whole solution,
which supports raw video only.

Then we'd focus on the common solutions(like recreating encoder instance) for
the encoders who declare such capabilities.

- Linjie

[1] https://patchwork.ffmpeg.org/project/ffmpeg/list/?series=1434
Nicolas George June 16, 2020, 10:55 a.m. UTC | #4
Fu, Linjie (12020-06-12):
> IIRC, the global header in extra data is optional in codec level.

Where did you take that?

The way I understand it, people who design codec will decide if they use
global extradata or not, but if they decide to, it is necessary to
decode the data. Otherwise, it would not be there!

Regards,
Fu, Linjie June 16, 2020, 1:33 p.m. UTC | #5
> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of
> Nicolas George
> Sent: Tuesday, June 16, 2020 18:55
> To: FFmpeg development discussions and patches <ffmpeg-
> devel@ffmpeg.org>
> Subject: Re: [FFmpeg-devel] [PATCH v2 4/5] fftools/ffmpeg: flush and
> recreate encoder instance if resolution changes
> 
> Fu, Linjie (12020-06-12):
> > IIRC, the global header in extra data is optional in codec level.
> 
> Where did you take that?

Chapter 2.3 Parameter Sets in < High Efficiency Video Coding (HEVC)
Algorithms and Architectures> [1]:

"Reusing parameter sets is bit rate efficient because it avoids the
necessity to send shared information multiple times. It is also loss
robust because it allows parameter set content to be carried by some
more reliable external communication link or to be repeated frequently
within the bitstream to ensure that it will not get lost."

> The way I understand it, people who design codec will decide if they use
> global extradata or not, but if they decide to, it is necessary to
> decode the data. Otherwise, it would not be there!

Indeed, the definition in spec is just the conformance, and how an encoder is
implemented in Libavcodec (and external library if any) is the thing really matters.

For encoders like libx264[2], libx265[3], libopenh264[4], if AV_CODEC_FLAG_GLOBAL_HEADER
flag is declared, the parameter Sets would be copied to extra data as the global header.
If not, parameter sets would be kept in the original bitstream, since it's fundamentally
supported in the encoder.
(BTW, librav1e seems to be identical, but I didn't check all the encoder and details for now)

And for encoders like libvpx-vp9, they don't implement the global header support, neither does
the libavformat(container) like ivf or webm.

(Please correct me if I missed anything or understood something wrongly, thx)

- Linjie

[1] https://link.springer.com/content/pdf/10.1007%2F978-3-319-06895-4.pdf
[2] https://github.com/FFmpeg/FFmpeg/blob/master/libavcodec/libx264.c#L924
[3] https://github.com/FFmpeg/FFmpeg/blob/master/libavcodec/libx265.c#L384
[4] https://github.com/FFmpeg/FFmpeg/blob/master/libavcodec/libopenh264enc.c#L338
Nicolas George June 16, 2020, 1:40 p.m. UTC | #6
Fu, Linjie (12020-06-16):
> Chapter 2.3 Parameter Sets in < High Efficiency Video Coding (HEVC)
> Algorithms and Architectures> [1]:

That is ONE codec. Not all of them.

> Indeed, the definition in spec is just the conformance, and how an encoder is
> implemented in Libavcodec (and external library if any) is the thing really matters.
> 
> For encoders like libx264[2], libx265[3], libopenh264[4], if AV_CODEC_FLAG_GLOBAL_HEADER
> flag is declared, the parameter Sets would be copied to extra data as the global header.
> If not, parameter sets would be kept in the original bitstream, since it's fundamentally
> supported in the encoder.

IIRC, encapsulation standards mandate one or the other for their
respective formats. We cannot choose.

Fact is, you cannot concatenate two packet streams and expect it to
work.

Regards,
Fu, Linjie June 16, 2020, 2:21 p.m. UTC | #7
> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of
> Nicolas George
> Sent: Tuesday, June 16, 2020 21:40
> To: FFmpeg development discussions and patches <ffmpeg-
> devel@ffmpeg.org>
> Subject: Re: [FFmpeg-devel] [PATCH v2 4/5] fftools/ffmpeg: flush and
> recreate encoder instance if resolution changes
> 
> Fu, Linjie (12020-06-16):
> > Chapter 2.3 Parameter Sets in < High Efficiency Video Coding (HEVC)
> > Algorithms and Architectures> [1]:
> 
> That is ONE codec. Not all of them.
> 
> > Indeed, the definition in spec is just the conformance, and how an encoder
> is
> > implemented in Libavcodec (and external library if any) is the thing really
> matters.
> >
> > For encoders like libx264[2], libx265[3], libopenh264[4], if
> AV_CODEC_FLAG_GLOBAL_HEADER
> > flag is declared, the parameter Sets would be copied to extra data as the
> global header.
> > If not, parameter sets would be kept in the original bitstream, since it's
> fundamentally
> > supported in the encoder.
> 
> IIRC, encapsulation standards mandate one or the other for their
> respective formats. We cannot choose.
> 
> Fact is, you cannot concatenate two packet streams and expect it to
> work.
> 
Okay, there are also some discussions on IRC yesterday about this, and one
Idea from Anton is to provide some bigger-picture solutions for concatenating
streams, maybe a bitstream filter to accommodate streamcopy.

Hence, I'd like to keep it open for now until we reach the concensus.

And I'd like to apply the patch set [1] soon for auto scale if no objections, as the first step
and touches raw video only for restricted usage for now (which is documented clearly).
This is already being waited for some time.

- Linjie

[1] https://patchwork.ffmpeg.org/project/ffmpeg/list/?series=1434
diff mbox series

Patch

diff --git a/fftools/ffmpeg.c b/fftools/ffmpeg.c
index 5859781..86562c9 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;
@@ -1039,6 +1040,37 @@  static void do_subtitle_out(OutputFile *of,
     }
 }
 
+static int recreate_encoder_instance(OutputStream *ost,
+                                     int width, int height)
+{
+    AVCodec *encoder = ost->enc_ctx->codec;
+    AVRational time_base = ost->enc_ctx->time_base;
+    int ret;
+
+    avcodec_free_context(&ost->enc_ctx);
+
+    ost->enc_ctx = avcodec_alloc_context3(encoder);
+    if (!ost->enc_ctx)
+        return AVERROR(ENOMEM);
+
+    ost->st->codecpar->width  = width;
+    ost->st->codecpar->height = height;
+
+    if (ret = avcodec_parameters_to_context(ost->enc_ctx, ost->st->codecpar) < 0)
+        return ret;
+
+    ost->enc_ctx->time_base = time_base;
+
+    if (ret = avcodec_open2(ost->enc_ctx, encoder, &ost->encoder_opts) < 0) {
+        av_log(NULL, AV_LOG_ERROR, "Error while re-opening encoder for output stream #%d:%d - "
+               "maybe incorrect parameters such as bit_rate, rate, width or height.\n",
+               ost->file_index, ost->index);
+        return ret;
+    }
+
+    return 0;
+}
+
 static void do_video_out(OutputFile *of,
                          OutputStream *ost,
                          AVFrame *next_picture,
@@ -1058,11 +1090,19 @@  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;
         }
+
+        if (recreate_encoder_instance(ost, next_picture->width, next_picture->height) < 0)
+            goto error;
+
+        enc = ost->enc_ctx;
     }
 
     if (ost->source_index >= 0)
diff --git a/fftools/ffmpeg_opt.c b/fftools/ffmpeg_opt.c
index 9d1489c..334c6ec 100644
--- a/fftools/ffmpeg_opt.c
+++ b/fftools/ffmpeg_opt.c
@@ -1562,7 +1562,7 @@  static OutputStream *new_output_stream(OptionsContext *o, AVFormatContext *oc, e
     MATCH_PER_STREAM_OPT(max_muxing_queue_size, i, ost->max_muxing_queue_size, oc, st);
     ost->max_muxing_queue_size *= sizeof(AVPacket);
 
-    if (oc->oformat->flags & AVFMT_GLOBALHEADER)
+    if (oc->oformat->flags & AVFMT_GLOBALHEADER && ost->autoscale)
         ost->enc_ctx->flags |= AV_CODEC_FLAG_GLOBAL_HEADER;
 
     av_dict_copy(&ost->sws_dict, o->g->sws_dict, 0);