On Wed, Feb 17, 2021 at 11:30 AM Wonkap Jang <won...@google.com> wrote:
> > > On Wed, Feb 17, 2021 at 9:50 AM Nicolas George <geo...@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-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".