[FFmpeg-devel] [PATCH v3] avcodec/libvpxenc: fix potential memory leak.

[Wonkap Jang]

On Wed, Feb 17, 2021 at 9:50 AM Nicolas George <george at nsup.org> wrote:

> Wonkap Jang (12021-02-17):
> > While parsing ref_frame_config, AVdictionary needs to be manually
> > deallocated.
> > ---
> >  libavcodec/libvpxenc.c | 21 +++++++++++++--------
> >  1 file changed, 13 insertions(+), 8 deletions(-)
> >
> > diff --git a/libavcodec/libvpxenc.c b/libavcodec/libvpxenc.c
> > index 284cb9a108..56a1b5aafe 100644
> > --- a/libavcodec/libvpxenc.c
> > +++ b/libavcodec/libvpxenc.c
> > @@ -127,7 +127,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;
> >
> > @@ -1595,15 +1594,21 @@ 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);
>
> > +                    AVDictionary* vpx_ref_frame_config = NULL;
> > +
> > +                    if (av_dict_parse_string(&vpx_ref_frame_config,
> en->value, "=", ":", 0) != 0) {
>
> > +                        av_log(avctx, AV_LOG_WARNING,
> > +                           "Error in parsing ref-frame-config. \n");
>
> I forgot this the first time: the error must be forwarded.
>
> > +                    } else {
> > +                        while ((en2 = av_dict_get(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);
> > +                        }
>
> See my other mail: it should not be in a dictionary at all, just passing
> the string and using the values immediately.
>
> >                      }
> >
> > +                    av_dict_free(&vpx_ref_frame_config);
> >                      codecctl_intp(avctx, VP9E_SET_SVC_REF_FRAME_CONFIG,
> (int *)&ctx->ref_frame_config);
> >                  } else {
> >                      av_log(avctx, AV_LOG_WARNING,
>
> Regards,
>
> --
>   Nicolas George
> _______________________________________________
> 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".


Hi Nicolas,

Thank you for the detailed information/recommendation. I do appreciate it.
I always tend to use the available (already-tested) tools when it comes to
string parsing due to potential errors that can easily pop up when dealing
with string manipulations in C. And, if I were the owner of a project, I'd
always recommend using exactly that even at the expense of losing a small
(I think this is very small in my opinion) efficiency. I feel the things
you will be saving would be some flag checking, and the part that is
iterating through the parsed key-value pairs at the expense of losing the
robustness of the code. And besides, the cost of the savings would be
negligible compared to the whole encoding pipeline. I tend to forgive small
loss of efficiency in complexity in encoding (external to codec) when it is
not at the block (macroblock, CTU... etc.) level for the sake of robustness.

That said, it is true that you will save some memory on top of the
complexity, and the tiny inefficiencies do pile on to become unbearable
sometimes. So, I will give it a shot. I'll look for the lower-level helper
functions that are available as you suggested.

Thank you so much,

Wonkap