HI, My comments are in-line. On Tue, Jan 14, 2020 at 8:55 PM James Zern <jzern-at-google....@ffmpeg.org> wrote:
> On Tue, Jan 14, 2020 at 11:07 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 | 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@ffmpeg.org > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > To unsubscribe, visit link above, or email > ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe". _______________________________________________ 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".