Message ID | 20210217174238.3269771-1-wonkap@google.com |
---|---|
State | Superseded |
Headers | show |
Series | [FFmpeg-devel,v3] avcodec/libvpxenc: fix potential memory leak. | expand |
Context | Check | Description |
---|---|---|
andriy/x86_make | success | Make finished |
andriy/x86_make_fate | fail | Make fate failed |
andriy/PPC64_make | warning | Make failed |
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,
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
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 --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,