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

[Wonkap Jang]

Hi James,

My answers are in-line.

On Wed, Jan 29, 2020 at 8:36 AM James Zern <jzern-at-google.com at ffmpeg.org>
wrote:

> On Fri, Jan 17, 2020 at 1:56 PM Wonkap Jang
> <wonkap-at-google.com at ffmpeg.org> wrote:
> >
> > Hi James,
> >
> > On Fri, Jan 17, 2020 at 1:50 PM Wonkap Jang <wonkap at google.com> 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 | 250 +++++++++++++++++++++++++++++++++++++----
> > >  2 files changed, 242 insertions(+), 26 deletions(-)
> > >
>
> I realized I missed a couple things previously.
>
> > > [...]
> > > +static int vpx_ts_param_parse(VPxContext *ctx, struct
> vpx_codec_enc_cfg
> > > *enccfg,
> > > +                              char *key, char *value, enum AVCodecID
> > > codec_id)
> > >  {
> > >      size_t value_len = strlen(value);
> > > +    int ts_layering_mode = 0;
> > >
> > >      if (!value_len)
> > >          return -1;
> > >
> > >      if (!strcmp(key, "ts_number_layers"))
>
> There isn't much validation here, libvpx should provide that, but can you
> try
> some incompatible input? ts_layering_mode is defined up to 3, so
> ts_number_layers > 3 should fail on init.
>
[WJ] actually.. it does not fail on init, since if ts_layering_mode is set
to 3, then it will override that parameter to be 3.
However, there are cases that are not correctly caught... such as if
ts_number_layers is set to 3 but if you put 4 parameters for bitrate and
such..
I do not think those cases are critical though.

>
> > > [...]
> > > +
> > > +#if (VPX_ENCODER_ABI_VERSION >= 12) && CONFIG_LIBVPX_VP9_ENCODER
> > > +    enccfg->temporal_layering_mode = 1; // only bypass mode is
> supported
>
> There's VP9E_TEMPORAL_LAYERING_MODE_BYPASS for this.
>
[WJ] will make the change.


>
> > > for now.
> > > +    enccfg->ss_number_layers = 1; // TODO: add spatial scalability
> > > support.
> > > +#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(*ctx->ts_layer_flags) *
> > > VPX_TS_MAX_PERIODICITY);
>
> This would be better written as av_malloc_array. Looking at the use of the
> array, it doesn't look like it needs to be zeroed, but there's
> av_mallocz_array
> for that.
>
[WJ] will do.

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


 Thank you,

Wonkap