| Message ID | 1591890182-22085-1-git-send-email-linjie.fu@intel.com |
|---|---|
| State | New |
| Headers | show |
| Series | None | expand |
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,
> 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
> 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
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,
> 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
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,
> 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 --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);
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(-)