Message ID | 20210217031229.3124071-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 | success | Make fate finished |
andriy/PPC64_make | success | Make finished |
andriy/PPC64_make_fate | success | Make fate finished |
Wonkap Jang (12021-02-16): > While parsing ref_frame_config, AVdictionary needs to be manually > deallocated. > --- > libavcodec/libvpxenc.c | 19 ++++++++++++------- > 1 file changed, 12 insertions(+), 7 deletions(-) NAK. This code is all wrong, it looks like Java or Python, it should not be using a dictionary in the first place, even less a dictionary stored in the context. Just iterate over the key=value pairs of the string without allocating all of them longer than necessary. Regards,
Hi Nicolas, On Wed, Feb 17, 2021 at 3:00 AM Nicolas George <george@nsup.org> wrote: > Wonkap Jang (12021-02-16): > > While parsing ref_frame_config, AVdictionary needs to be manually > > deallocated. > > --- > > libavcodec/libvpxenc.c | 19 ++++++++++++------- > > 1 file changed, 12 insertions(+), 7 deletions(-) > > NAK. > > This code is all wrong, it looks like Java or Python, it should not be > using a dictionary in the first place, even less a dictionary stored in > the context. Just iterate over the key=value pairs of the string without > allocating all of them longer than necessary. > > 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". I'll change it to use a local variable. But, not sure what you mean by "not using the dictionary in the first place".. Could you explain how to do this? I need to parse multiple variables that have multiple values that have variable number of entries depending on th enumber of spatial layers. I appreciate your help. Thank you, Wonkap
On Wed, Feb 17, 2021 at 8:52 AM Wonkap Jang <wonkap@google.com> wrote: > Hi Nicolas, > > On Wed, Feb 17, 2021 at 3:00 AM Nicolas George <george@nsup.org> wrote: > >> Wonkap Jang (12021-02-16): >> > While parsing ref_frame_config, AVdictionary needs to be manually >> > deallocated. >> > --- >> > libavcodec/libvpxenc.c | 19 ++++++++++++------- >> > 1 file changed, 12 insertions(+), 7 deletions(-) >> >> NAK. >> >> This code is all wrong, it looks like Java or Python, it should not be >> using a dictionary in the first place, even less a dictionary stored in >> the context. Just iterate over the key=value pairs of the string without >> allocating all of them longer than necessary. >> >> 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". > > > I'll change it to use a local variable. > > But, not sure what you mean by "not using the dictionary in the first > place".. > Could you explain how to do this? I need to parse multiple variables that > have multiple values that have variable number of entries depending on th > enumber of spatial layers. > > I appreciate your help. > > Thank you, > > Wonkap > > Or are you saying after getting the string with en->value, I should just parse through the string without dictionary? Not quite familiar with using AVDictionary.. My purpose here is not to envoke av_dict_set for each parameter, but rather get all parameters in one call (using "ref-frame-config"), and parse through each parameter internally. And, in order to do that, I had to parse the whole string into a key-value pairs, and then recurse through them. Thanks, -Wonkap
Wonkap Jang (12021-02-17): > Or are you saying after getting the string with en->value, I should just > parse through the string without dictionary? Yes, exactly. You would use a dictionary if you need to keep all the values for a later use. But in this case, there is no later use, you only iterate on the values once, and immediately. > My purpose here is not to envoke av_dict_set for each parameter, but rather > get all parameters in one call (using "ref-frame-config"), and parse > through each parameter internally. And, in order to do that, I had to parse > the whole string into a key-value pairs, and then recurse through them. Yes, that is the "simple" way to do it, when you use a high-level language and do not worry about optimizations: use a high-level function, store into a high-level data structure, then use a high-level construct to exploit that data structure. But FFmpeg is low-level and cares about optimization. So you use a low-level parsing function, and do the strict necessary to achieve the result. The code should look like: loop parse one pair check for error and return stop if it's the end do with the pair free the pair end loop I suggest you look at the code for av_dict_parse_string() if this does not seem easy to you (it should!) or if you are not familiar with the other helper function available. Your code will be similar, but instead of av_dict_set(), you do the libvpx stuff. Regards,
diff --git a/libavcodec/libvpxenc.c b/libavcodec/libvpxenc.c index 284cb9a108..b552590b7b 100644 --- a/libavcodec/libvpxenc.c +++ b/libavcodec/libvpxenc.c @@ -1595,15 +1595,20 @@ 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); + ctx->vpx_ref_frame_config = NULL; + + if (av_dict_parse_string(&ctx->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(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); } + av_dict_free(&ctx->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,