diff mbox series

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

Message ID 20210217174238.3269771-1-wonkap@google.com
State Superseded
Headers show
Series [FFmpeg-devel,v3] avcodec/libvpxenc: fix potential memory leak. | expand

Checks

Context Check Description
andriy/x86_make success Make finished
andriy/x86_make_fate fail Make fate failed
andriy/PPC64_make warning Make failed

Commit Message

Wonkap Jang Feb. 17, 2021, 5:42 p.m. UTC 
While parsing ref_frame_config, AVdictionary needs to be manually
deallocated.
---
 libavcodec/libvpxenc.c | 21 +++++++++++++--------
 1 file changed, 13 insertions(+), 8 deletions(-)

Comments

Nicolas George Feb. 17, 2021, 5:50 p.m. UTC | #1 
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,
Wonkap Jang Feb. 17, 2021, 7:30 p.m. UTC | #2 
On Wed, Feb 17, 2021 at 9:50 AM Nicolas George <george@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@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@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
Wonkap Jang Feb. 18, 2021, 4:07 a.m. UTC | #3 
On Wed, Feb 17, 2021 at 11:30 AM Wonkap Jang <wonkap@google.com> wrote:

>
>
> On Wed, Feb 17, 2021 at 9:50 AM Nicolas George <george@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@ffmpeg.org
>> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>>
>> To unsubscribe, visit link above, or email
>> ffmpeg-devel-request@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
diff mbox series

Patch

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");
+                    } 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);
+                        }
                     }
 
+                    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,