[FFmpeg-devel,v4] avcodec/libvpxenc: optimize parsing vpx_svc_ref_frame_config parameters

Getting rid of unnecessary use of AVDictionary object in parsing
vpx_svc_ref_frame_config.
---
 libavcodec/libvpxenc.c | 76 ++++++++++++++++++++++++++++++++----------
 1 file changed, 58 insertions(+), 18 deletions(-)

On Wed, Feb 17, 2021 at 4:41 PM Wonkap Jang <wonkap@google.com> wrote:

> Getting rid of unnecessary use of AVDictionary object in parsing
> vpx_svc_ref_frame_config.
> ---
>  libavcodec/libvpxenc.c | 76 ++++++++++++++++++++++++++++++++----------
>  1 file changed, 58 insertions(+), 18 deletions(-)
>
> diff --git a/libavcodec/libvpxenc.c b/libavcodec/libvpxenc.c
> index 284cb9a108..abfd0032c1 100644
> --- a/libavcodec/libvpxenc.c
> +++ b/libavcodec/libvpxenc.c
> @@ -42,6 +42,7 @@
>  #include "libavutil/mathematics.h"
>  #include "libavutil/opt.h"
>  #include "libavutil/pixdesc.h"
> +#include "libavutil/avstring.h"
>
>  /**
>   * Portion of struct vpx_codec_cx_pkt from vpx_encoder.h.
> @@ -127,7 +128,6 @@ typedef struct VPxEncoderContext {
>      int roi_warned;
>  #if CONFIG_LIBVPX_VP9_ENCODER &&
> defined(VPX_CTRL_VP9E_SET_MAX_INTER_BITRATE_PCT)
>      vpx_svc_ref_frame_config_t ref_frame_config;
> -    AVDictionary *vpx_ref_frame_config;
>  #endif
>  } VPxContext;
>
> @@ -561,18 +561,13 @@ static int vpx_ts_param_parse(VPxContext *ctx,
> struct vpx_codec_enc_cfg *enccfg,
>  }
>
>  #if CONFIG_LIBVPX_VP9_ENCODER &&
> defined(VPX_CTRL_VP9E_SET_MAX_INTER_BITRATE_PCT)
> -static int vpx_ref_frame_config_parse(VPxContext *ctx, const struct
> vpx_codec_enc_cfg *enccfg,
> -                                      char *key, char *value, enum
> AVCodecID codec_id)
> +static int vpx_ref_frame_config_set_value(vpx_svc_ref_frame_config_t
> *ref_frame_config,
> +                                          int ss_number_layers, char
> *key, char *value)
>  {
>      size_t value_len = strlen(value);
> -    int ss_number_layers = enccfg->ss_number_layers;
> -    vpx_svc_ref_frame_config_t *ref_frame_config = &ctx->ref_frame_config;
>
>      if (!value_len)
> -        return -1;
> -
> -    if (codec_id != AV_CODEC_ID_VP9)
> -        return -1;
> +        return AVERROR(EINVAL);
>
>      if (!strcmp(key, "rfc_update_buffer_slot")) {
>          vp8_ts_parse_int_array(ref_frame_config->update_buffer_slot,
> value, value_len, ss_number_layers);
> @@ -600,6 +595,52 @@ static int vpx_ref_frame_config_parse(VPxContext
> *ctx, const struct vpx_codec_en
>
>      return 0;
>  }
> +
> +static int vpx_parse_ref_frame_config_element(vpx_svc_ref_frame_config_t
> *ref_frame_config, int ss_number_layers,
> +                                              const char **buf, const
> char *key_val_sep, const char *pairs_sep)
> +{
> +    char *key = av_get_token(buf, key_val_sep);
> +    char *val = NULL;
> +    int ret;
> +
> +    if (key && *key && strspn(*buf, key_val_sep)) {
> +        (*buf)++;
> +        val = av_get_token(buf, pairs_sep);
> +    }
> +
> +    if (key && *key && val && *val)
> +        ret = vpx_ref_frame_config_set_value(ref_frame_config,
> ss_number_layers, key, val);
> +    else
> +        ret = AVERROR(EINVAL);
> +
> +    av_freep(&key);
> +    av_freep(&val);
> +
> +    return ret;
> +}
> +
> +static int vpx_parse_ref_frame_config(vpx_svc_ref_frame_config_t
> *ref_frame_config, int ss_number_layers,
> +                                      const char *str, const char
> *key_val_sep, const char *pairs_sep)
> +{
> +    int ret = 0;
> +
> +    if (!ref_frame_config)
> +        return AVERROR(EINVAL);
> +
> +    if (!str)
> +        return AVERROR(EINVAL);
> +
> +    while (*str) {
> +        if ((ret = vpx_parse_ref_frame_config_element(ref_frame_config,
> ss_number_layers,
> +                                                      &str, key_val_sep,
> pairs_sep)) !=0)
> +            return ret;
> +
> +        if (*str)
> +            str++;
> +    }
> +
> +    return ret;
> +}
>  #endif
>
>  #if CONFIG_LIBVPX_VP9_ENCODER
> @@ -1594,20 +1635,19 @@ static int vpx_encode(AVCodecContext *avctx,
> AVPacket *pkt,
>
>              if (en) {
>                  if (avctx->codec_id == AV_CODEC_ID_VP9) {
> -                    AVDictionaryEntry* en2 = NULL;
> -                    av_dict_parse_string(&ctx->vpx_ref_frame_config,
> en->value, "=", ":", 0);
> -
> -                    while ((en2 = av_dict_get(ctx->vpx_ref_frame_config,
> "", en2, AV_DICT_IGNORE_SUFFIX))) {
> -                        if (vpx_ref_frame_config_parse(ctx, enccfg,
> en2->key, en2->value, avctx->codec_id) < 0)
> -                            av_log(avctx, AV_LOG_WARNING,
> -                                   "Error parsing option '%s = %s'.\n",
> -                                   en2->key, en2->value);
> +                    int ret;
> +                    ret =
> vpx_parse_ref_frame_config(&ctx->ref_frame_config,
> +
>  enccfg->ss_number_layers, en->value,  "=", ":");
> +                    if (ret != 0) {
> +                        av_log(avctx, AV_LOG_WARNING,
> +                               "Error parsing ref_frame_config option
> %s.\n", en->value);
> +                        return ret;
>                      }
>
>                      codecctl_intp(avctx, VP9E_SET_SVC_REF_FRAME_CONFIG,
> (int *)&ctx->ref_frame_config);
>                  } else {
>                      av_log(avctx, AV_LOG_WARNING,
> -                           "Error using option ref-frame-config for a
> non-VP9 codec\n");
> +                           "Using option ref-frame-config for a non-VP9
> codec\n");
>                  }
>              }
>  #endif
> --
> 2.30.0.478.g8a0d178c01-goog
>
> Hi Nicolas,

Just in case you missed my email indicating that this is the change
responding to your review.

Thank you so much for your time.

-Wonkap

Wonkap Jang (12021-02-17):
> Getting rid of unnecessary use of AVDictionary object in parsing
> vpx_svc_ref_frame_config.
> ---
>  libavcodec/libvpxenc.c | 76 ++++++++++++++++++++++++++++++++----------
>  1 file changed, 58 insertions(+), 18 deletions(-)

Thanks for the updated patch. It looks much better now. There are a few
minor enhancement still.

> 
> diff --git a/libavcodec/libvpxenc.c b/libavcodec/libvpxenc.c
> index 284cb9a108..abfd0032c1 100644
> --- a/libavcodec/libvpxenc.c
> +++ b/libavcodec/libvpxenc.c
> @@ -42,6 +42,7 @@
>  #include "libavutil/mathematics.h"
>  #include "libavutil/opt.h"
>  #include "libavutil/pixdesc.h"
> +#include "libavutil/avstring.h"
>  
>  /**
>   * Portion of struct vpx_codec_cx_pkt from vpx_encoder.h.
> @@ -127,7 +128,6 @@ typedef struct VPxEncoderContext {
>      int roi_warned;
>  #if CONFIG_LIBVPX_VP9_ENCODER && defined(VPX_CTRL_VP9E_SET_MAX_INTER_BITRATE_PCT)
>      vpx_svc_ref_frame_config_t ref_frame_config;
> -    AVDictionary *vpx_ref_frame_config;
>  #endif
>  } VPxContext;
>  
> @@ -561,18 +561,13 @@ static int vpx_ts_param_parse(VPxContext *ctx, struct vpx_codec_enc_cfg *enccfg,
>  }
>  
>  #if CONFIG_LIBVPX_VP9_ENCODER && defined(VPX_CTRL_VP9E_SET_MAX_INTER_BITRATE_PCT)
> -static int vpx_ref_frame_config_parse(VPxContext *ctx, const struct vpx_codec_enc_cfg *enccfg,
> -                                      char *key, char *value, enum AVCodecID codec_id)
> +static int vpx_ref_frame_config_set_value(vpx_svc_ref_frame_config_t *ref_frame_config,
> +                                          int ss_number_layers, char *key, char *value)
>  {
>      size_t value_len = strlen(value);
> -    int ss_number_layers = enccfg->ss_number_layers;
> -    vpx_svc_ref_frame_config_t *ref_frame_config = &ctx->ref_frame_config;
>  
>      if (!value_len)
> -        return -1;
> -
> -    if (codec_id != AV_CODEC_ID_VP9)
> -        return -1;
> +        return AVERROR(EINVAL);
>  
>      if (!strcmp(key, "rfc_update_buffer_slot")) {
>          vp8_ts_parse_int_array(ref_frame_config->update_buffer_slot, value, value_len, ss_number_layers);
> @@ -600,6 +595,52 @@ static int vpx_ref_frame_config_parse(VPxContext *ctx, const struct vpx_codec_en
>  
>      return 0;
>  }
> +

> +static int vpx_parse_ref_frame_config_element(vpx_svc_ref_frame_config_t *ref_frame_config, int ss_number_layers,
> +                                              const char **buf, const char *key_val_sep, const char *pairs_sep)

You do not need to carry the key_val_sep and pairs_sep around, just
hard-code them when they are used instead of hard-coding them in the
initial call.

> +{
> +    char *key = av_get_token(buf, key_val_sep);
> +    char *val = NULL;
> +    int ret;
> +
> +    if (key && *key && strspn(*buf, key_val_sep)) {
> +        (*buf)++;
> +        val = av_get_token(buf, pairs_sep);
> +    }
> +
> +    if (key && *key && val && *val)
> +        ret = vpx_ref_frame_config_set_value(ref_frame_config, ss_number_layers, key, val);
> +    else
> +        ret = AVERROR(EINVAL);
> +
> +    av_freep(&key);
> +    av_freep(&val);
> +
> +    return ret;
> +}
> +
> +static int vpx_parse_ref_frame_config(vpx_svc_ref_frame_config_t *ref_frame_config, int ss_number_layers,
> +                                      const char *str, const char *key_val_sep, const char *pairs_sep)
> +{
> +    int ret = 0;
> +

> +    if (!ref_frame_config)
> +        return AVERROR(EINVAL);
> +
> +    if (!str)
> +        return AVERROR(EINVAL);

Dictionaries entries cannot be NULL, and ref_frame_config can defintely
not be NULL here, so these tests are redundant, and misleading about the
proper use of this function.

> +
> +    while (*str) {

> +        if ((ret = vpx_parse_ref_frame_config_element(ref_frame_config, ss_number_layers,
> +                                                      &str, key_val_sep, pairs_sep)) !=0)

Please do not merge assignment and comparison when it does not make the
code more readable.

And the usual idiom is "if (ret < 0)".

> +            return ret;
> +
> +        if (*str)
> +            str++;
> +    }
> +
> +    return ret;
> +}
>  #endif
>  
>  #if CONFIG_LIBVPX_VP9_ENCODER
> @@ -1594,20 +1635,19 @@ static int vpx_encode(AVCodecContext *avctx, AVPacket *pkt,
>  
>              if (en) {
>                  if (avctx->codec_id == AV_CODEC_ID_VP9) {
> -                    AVDictionaryEntry* en2 = NULL;
> -                    av_dict_parse_string(&ctx->vpx_ref_frame_config, en->value, "=", ":", 0);
> -
> -                    while ((en2 = av_dict_get(ctx->vpx_ref_frame_config, "", en2, AV_DICT_IGNORE_SUFFIX))) {
> -                        if (vpx_ref_frame_config_parse(ctx, enccfg, en2->key, en2->value, avctx->codec_id) < 0)
> -                            av_log(avctx, AV_LOG_WARNING,
> -                                   "Error parsing option '%s = %s'.\n",
> -                                   en2->key, en2->value);
> +                    int ret;
> +                    ret = vpx_parse_ref_frame_config(&ctx->ref_frame_config,
> +                                                     enccfg->ss_number_layers, en->value,  "=", ":");

> +                    if (ret != 0) {

ret < 0 here too.

> +                        av_log(avctx, AV_LOG_WARNING,
> +                               "Error parsing ref_frame_config option %s.\n", en->value);
> +                        return ret;
>                      }
>  
>                      codecctl_intp(avctx, VP9E_SET_SVC_REF_FRAME_CONFIG, (int *)&ctx->ref_frame_config);
>                  } else {
>                      av_log(avctx, AV_LOG_WARNING,
> -                           "Error using option ref-frame-config for a non-VP9 codec\n");
> +                           "Using option ref-frame-config for a non-VP9 codec\n");
>                  }
>              }
>  #endif

Regards,