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

[Wonkap Jang]

On Wed, Feb 17, 2021 at 11:30 AM Wonkap Jang <wonkap at google.com> wrote:

>
>
> 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
>

Hi Nicolas,

FYI: I have sent out the new code: you can search for subject:
"avcodec/libvpxenc: optimize parsing vpx_svc_ref_frame_config parameters"

I'll wait for your review.

Thank you,

Wonkap