Re: [FFmpeg-devel] [PATCH v3] avcodec/libvpxenc: fix potential memory leak.
On Wed, Feb 17, 2021 at 11:30 AM Wonkap Jang wrote:
>
>
> On Wed, Feb 17, 2021 at 9:50 AM Nicolas George 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(>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(_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(_ref_frame_config);
>> > codecctl_intp(avctx,
>> VP9E_SET_SVC_REF_FRAME_CONFIG, (int *)>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-requ...@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
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH v3] avcodec/libvpxenc: fix potential memory leak.
On Wed, Feb 17, 2021 at 9:50 AM Nicolas George 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(>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(_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(_ref_frame_config);
> > codecctl_intp(avctx, VP9E_SET_SVC_REF_FRAME_CONFIG,
> (int *)>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-requ...@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
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH v3] avcodec/libvpxenc: fix potential memory leak.
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(>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(_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(_ref_frame_config);
> codecctl_intp(avctx, VP9E_SET_SVC_REF_FRAME_CONFIG, (int
> *)>ref_frame_config);
> } else {
> av_log(avctx, AV_LOG_WARNING,
Regards,
--
Nicolas George
signature.asc
Description: PGP signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH v3] avcodec/libvpxenc: fix potential memory leak.
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, -- Nicolas George signature.asc Description: PGP signature ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
[FFmpeg-devel] [PATCH v3] avcodec/libvpxenc: fix potential memory leak.
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(>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(_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(_ref_frame_config);
codecctl_intp(avctx, VP9E_SET_SVC_REF_FRAME_CONFIG, (int
*)>ref_frame_config);
} else {
av_log(avctx, AV_LOG_WARNING,
--
2.30.0.478.g8a0d178c01-goog
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH v3] avcodec/libvpxenc: fix potential memory leak.
On Wed, Feb 17, 2021 at 8:52 AM Wonkap Jang wrote: > Hi Nicolas, > > On Wed, Feb 17, 2021 at 3:00 AM Nicolas George 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-requ...@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 ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH v3] avcodec/libvpxenc: fix potential memory leak.
Hi Nicolas, On Wed, Feb 17, 2021 at 3:00 AM Nicolas George 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-requ...@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 ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH v3] avcodec/libvpxenc: fix potential memory leak.
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 signature.asc Description: PGP signature ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
[FFmpeg-devel] [PATCH 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(-)
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(>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(>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(>vpx_ref_frame_config);
codecctl_intp(avctx, VP9E_SET_SVC_REF_FRAME_CONFIG, (int
*)>ref_frame_config);
} else {
av_log(avctx, AV_LOG_WARNING,
--
2.30.0.478.g8a0d178c01-goog
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
