[FFmpeg-devel] [PATCH v4] avcodec/libvpxenc: add VP9 temporal scalability encoding option

[James Zern]

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.

> +        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

> +        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.

> [...]
> +
> +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.

> [...]
> +    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.