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

While parsing ref_frame_config, AVdictionary needs to be manually
deallocated.
---
 libavcodec/libvpxenc.c | 19 ++++++++++++-------
 1 file changed, 12 insertions(+), 7 deletions(-)

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,