HI,
On Mon, Jan 13, 2020 at 4:44 PM James Zern <jzern-at-google....@ffmpeg.org>
wrote:
> Hi,
>
> On Fri, Jan 10, 2020 at 9:59 AM Wonkap Jang
> <wonkap-at-google....@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@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
My comments are in-line.
Wonkap
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
