[FFmpeg-devel] [PATCH v4] avcodec/libvpxenc: add VP9 temporal scalability encoding option
Wonkap Jang
wonkap at google.com
Thu Jan 16 04:19:36 EET 2020
HI,
My comments are in-line.
On Tue, Jan 14, 2020 at 8:55 PM James Zern <jzern-at-google.com at ffmpeg.org>
wrote:
> On Tue, Jan 14, 2020 at 11:07 AM Wonkap Jang
> <wonkap-at-google.com at ffmpeg.org> wrote:
> >
> > This commit reuses the configuration options for VP8 that enables
> > temporal scalability for VP9. It also adds a way to enable three
> > preset temporal structures (refer to the documentation for more
> > detail) that can be used in offline encoding.
> > ---
> > doc/encoders.texi | 18 ++-
> > libavcodec/libvpxenc.c | 252 +++++++++++++++++++++++++++++++++++++----
> > 2 files changed, 244 insertions(+), 26 deletions(-)
> >
> > [...]
> > @@ -221,10 +229,20 @@ static av_cold void dump_enc_cfg(AVCodecContext
> *avctx,
> > width, "rc_overshoot_pct:", cfg->rc_overshoot_pct);
> > av_log(avctx, level, "temporal layering settings\n"
> > " %*s%u\n", width, "ts_number_layers:",
> cfg->ts_number_layers);
> > - av_log(avctx, level,
> > - "\n %*s", width, "ts_target_bitrate:");
> > - for (i = 0; i < VPX_TS_MAX_LAYERS; i++)
> > - av_log(avctx, level, "%u ", cfg->ts_target_bitrate[i]);
> > + if (avctx->codec_id == AV_CODEC_ID_VP8) {
> > + av_log(avctx, level,
> > + "\n %*s", width, "ts_target_bitrate:");
>
> align this with the '(' here and below.
>
[WJ] Not sure why this was sent this way, as the lines were on the same
line in the commit. Maybe lint code running?
I will make the change to get around that.
>
> > + for (i = 0; i < VPX_TS_MAX_LAYERS; i++)
> > + av_log(avctx, level, "%u ", cfg->ts_target_bitrate[i]);
> > + }
> > +#if (VPX_ENCODER_ABI_VERSION >= 12) && CONFIG_LIBVPX_VP9_ENCODER
> > + if (avctx->codec_id == AV_CODEC_ID_VP9) {
> > + av_log(avctx, level,
> > + "\n %*s", width, "layer_target_bitrate:");
>
> trailing whitespace
>
[WJ] Will do.
>
> > + for (i = 0; i < VPX_TS_MAX_LAYERS; i++)
> > + av_log(avctx, level, "%u ", cfg->layer_target_bitrate[i]);
> > + }
> > +#endif
> > av_log(avctx, level, "\n");
> > av_log(avctx, level,
> > "\n %*s", width, "ts_rate_decimator:");
> > @@ -346,6 +364,8 @@ static av_cold int vpx_free(AVCodecContext *avctx)
> > }
> > #endif
> >
> > + av_freep(&ctx->ts_layer_flags);
> > +
> > vpx_codec_destroy(&ctx->encoder);
> > if (ctx->is_alpha) {
> > vpx_codec_destroy(&ctx->encoder_alpha);
> > @@ -370,23 +390,153 @@ static void vp8_ts_parse_int_array(int *dest,
> char *value, size_t value_len, int
> > }
> > }
> >
> > -static int vp8_ts_param_parse(struct vpx_codec_enc_cfg *enccfg, char
> *key, char *value)
> > +static void set_temporal_layer_pattern(int layering_mode,
> > + vpx_codec_enc_cfg_t *cfg,
> > + int *layer_flags,
> > + int *flag_periodicity)
> > +{
> > + switch (layering_mode) {
> > + case 2: {
> > + /**
> > + * 2-layers, 2-frame period.
> > + */
> > + int ids[2] = { 0, 1 };
> > + cfg->ts_periodicity = 2;
> > + *flag_periodicity = 2;
> > + cfg->ts_number_layers = 2;
> > + cfg->ts_rate_decimator[0] = 2;
> > + cfg->ts_rate_decimator[1] = 1;
> > + memcpy(cfg->ts_layer_id, ids, sizeof(ids));
> > +
> > + layer_flags[0] =
> > + VP8_EFLAG_NO_REF_GF | VP8_EFLAG_NO_REF_ARF |
> > + VP8_EFLAG_NO_UPD_GF | VP8_EFLAG_NO_UPD_ARF;
> > + layer_flags[1] =
> > + VP8_EFLAG_NO_UPD_ARF | VP8_EFLAG_NO_UPD_GF |
> > + VP8_EFLAG_NO_UPD_LAST |
> > + VP8_EFLAG_NO_REF_ARF | VP8_EFLAG_NO_REF_GF;
> > + break;
> > + }
> > + case 3: {
> > + /**
> > + * 3-layers structure with one reference frame.
> > + * This works same as temporal_layering_mode 3.
> > + *
> > + * 3-layers, 4-frame period.
> > + */
> > + int ids[4] = { 0, 2, 1, 2 };
> > + cfg->ts_periodicity = 4;
> > + *flag_periodicity = 4;
> > + cfg->ts_number_layers = 3;
> > + cfg->ts_rate_decimator[0] = 4;
> > + cfg->ts_rate_decimator[1] = 2;
> > + cfg->ts_rate_decimator[2] = 1;
> > + memcpy(cfg->ts_layer_id, ids, sizeof(ids));
> > +
> > + /**
> > + * 0=L, 1=GF, 2=ARF,
>
> my point with the earlier comment was that you document 3 indices, but
> set 4. if [3] won't be used in this case then the assignment could be
> removed.
>
[WJ] the ids[] and layer_flags[] are indexed up to the frame# in
ts_periodicity..
so that goes from 0-3 (period of 4), whereas ts_rate_decimator[] is indexed
by the temporal layer id itself (ids[]) which goes from 0 to 2. Am I
missing something?
> > [...]
> > +
> > +static int vpx_ts_param_parse(VPxContext *ctx,
> > + struct vpx_codec_enc_cfg *enccfg,
> > + char *key, char *value,
> > + const enum AVCodecID codec_id)
>
> it's not common in this code base to mark non-pointer parameters
> const. you can merge the first and second lines and the third and
> fourth since they're not overly long.
>
> [WJ] my mistake. will make the changes.
> > [...]
> > + if (ts_layering_mode) {
> > + // make sure the ts_layering_mode comes at the end of the
> ts_parameter string to ensure that
> > + // correct configuration is done.
> > + ctx->ts_layer_flags = av_malloc(sizeof(*ctx->ts_layer_flags) *
> VPX_TS_MAX_PERIODICITY);
> > + set_temporal_layer_pattern(ts_layering_mode, enccfg,
> ctx->ts_layer_flags, &enccfg->ts_periodicity);
>
> indent is incorrect, it should be 4 spaces.
>
[WJ] will make the change.
Thank you,
Wonkap
_______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request at ffmpeg.org with subject "unsubscribe".
More information about the ffmpeg-devel
mailing list