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

[Wonkap Jang]

HI,

On Mon, Jan 13, 2020 at 4:44 PM James Zern <jzern-at-google.com at ffmpeg.org>
wrote:

> Hi,
>
> On Fri, Jan 10, 2020 at 9:59 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 | 251 +++++++++++++++++++++++++++++++++++++----
> >  2 files changed, 243 insertions(+), 26 deletions(-)
> >
> > [...]
> >  #endif
> > @@ -223,8 +231,16 @@ static av_cold void dump_enc_cfg(AVCodecContext
> *avctx,
> >             "  %*s%u\n", width, "ts_number_layers:",
> cfg->ts_number_layers);
> >      av_log(avctx, level,
> >             "\n  %*s", width, "ts_target_bitrate:");
>
> This should move to the vp8 branch and a new heading added for vp9's
> layer_target_bitrate.
>
> > [...]
> > -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 };
>
> these can be static const.
>
[WJ] OK.

>
> > +        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;
> > +
>
> this line can be removed.
>
[WJ] OK.

>
> > +        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,
>
> what about [3]?
>
[WJ] decimator is for indicating the framerate decimation per temporal
layer. There are three temporal layers in this case, no need for [3].


>
> > +         * Intra-layer prediction disabled.
> > +         */
> > +        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[2] =
>
> can you reorder this to be in the natural 0...3 order?
>
[WJ] this is in the order of temporal layer id, just like it is written in
vpx_temporal_svc_encoder in libvpx/examples, but I'll make the change as
suggested.


>
> > [...]
> > +        /**
> > +         * 0=L, 1=GF, 2=ARF, Intra-layer prediction disabled.
> > +         */
> > +        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[2] =
>
> same comment here about order.
>
> > [...]
> > +
> > +#if (VPX_ENCODER_ABI_VERSION >= 12) && CONFIG_LIBVPX_VP9_ENCODER
> > +    enccfg->temporal_layering_mode = 1; // only bypass mode is being
> supported for now.
>
> ...mode is supported...
>
[WJ] gotit.

>
> > +    enccfg->ss_number_layers = 1; // making sure the spatial
> scalability is off. Will support this in future.
>
> this can be TODO: add spatial scalability support.

[WJ] gotit.

>


> > +#endif
> > +    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(int) *
> VPX_TS_MAX_PERIODICITY);
>
> prefer sizeof(var).
>
[WJ] OK.

>
> > [...]
> >
> > -    if(!avctx->bit_rate)
> > -        if(avctx->rc_max_rate || avctx->rc_buffer_size ||
> avctx->rc_initial_buffer_occupancy) {
> > +    if (!avctx->bit_rate)
> > +        if (avctx->rc_max_rate || avctx->rc_buffer_size ||
> avctx->rc_initial_buffer_occupancy) {
>
> this is unrelated to the change, please remove it.
>
[WJ] ok.

>
> > [...]
> > +#if VPX_ENCODER_ABI_VERSION >= 12
> > +        codecctl_int(avctx, VP9E_SET_SVC, 1);
> > +        codecctl_intp(avctx, VP9E_SET_SVC_PARAMETERS, (int *)
> &svc_params);
>
> drop the space after the cast.
>
[WJ] Ok.

>
> > [...]
> > +#if CONFIG_LIBVPX_VP9_ENCODER && VPX_ENCODER_ABI_VERSION >= 12
> > +        else if (avctx->codec_id == AV_CODEC_ID_VP9) {
> > +            codecctl_intp(avctx, VP9E_SET_SVC_LAYER_ID, (int *)
> &layer_id);
>
> drop the space after the cast.
>
[WJ] ok.


> _______________________________________________
> 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".



My comments are in-line.

Wonkap