Re: [FFmpeg-devel] [PATCH 5/6] avcodec/lib*, avformat/tee: Simplify iterating over AVDictionary

[epirat07@gmail.com]

On 17 May 2024, at 17:44, Andreas Rheinhardt wrote:

> epirat07@gmail.com:
>>
>>
>> On 17 May 2024, at 17:25, Andreas Rheinhardt wrote:
>>
>>> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
>>> ---
>>>  libavcodec/libaomenc.c  | 4 ++--
>>>  libavcodec/libkvazaar.c | 4 ++--
>>>  libavcodec/libsvtav1.c  | 6 +++---
>>>  libavcodec/libx264.c    | 4 ++--
>>>  libavcodec/libx265.c    | 4 ++--
>>>  libavformat/tee.c       | 4 ++--
>>>  6 files changed, 13 insertions(+), 13 deletions(-)
>>>
>>> diff --git a/libavcodec/libaomenc.c b/libavcodec/libaomenc.c
>>> index c39853c20f..dec74ebecd 100644
>>> --- a/libavcodec/libaomenc.c
>>> +++ b/libavcodec/libaomenc.c
>>> @@ -970,9 +970,9 @@ static av_cold int aom_init(AVCodecContext *avctx,
>>>
>>>  #if AOM_ENCODER_ABI_VERSION >= 23
>>>      {
>>> -        AVDictionaryEntry *en = NULL;
>>> +        const AVDictionaryEntry *en = NULL;
>>>
>>> -        while ((en = av_dict_get(ctx->aom_params, "", en, AV_DICT_IGNORE_SUFFIX))) {
>>> +        while ((en = av_dict_iterate(ctx->aom_params, en))) {
>>>              int ret = aom_codec_set_option(&ctx->encoder, en->key, en->value);
>>>              if (ret != AOM_CODEC_OK) {
>>>                  log_encoder_error(avctx, en->key);
>>> diff --git a/libavcodec/libkvazaar.c b/libavcodec/libkvazaar.c
>>> index 0711d9ab38..cd731ae9d0 100644
>>> --- a/libavcodec/libkvazaar.c
>>> +++ b/libavcodec/libkvazaar.c
>>> @@ -111,8 +111,8 @@ FF_ENABLE_DEPRECATION_WARNINGS
>>>      if (ctx->kvz_params) {
>>>          AVDictionary *dict = NULL;
>>>          if (!av_dict_parse_string(&dict, ctx->kvz_params, "=", ",", 0)) {
>>> -            AVDictionaryEntry *entry = NULL;
>>> -            while ((entry = av_dict_get(dict, "", entry, AV_DICT_IGNORE_SUFFIX))) {
>>> +            const AVDictionaryEntry *entry = NULL;
>>> +            while ((entry = av_dict_iterate(dict, entry))) {
>>>                  if (!api->config_parse(cfg, entry->key, entry->value)) {
>>>                      av_log(avctx, AV_LOG_WARNING, "Invalid option: %s=%s.\n",
>>>                             entry->key, entry->value);
>>> diff --git a/libavcodec/libsvtav1.c b/libavcodec/libsvtav1.c
>>> index 9bc165f0cf..2fef8c8971 100644
>>> --- a/libavcodec/libsvtav1.c
>>> +++ b/libavcodec/libsvtav1.c
>>> @@ -210,7 +210,7 @@ static int config_enc_params(EbSvtAv1EncConfiguration *param,
>>>  {
>>>      SvtContext *svt_enc = avctx->priv_data;
>>>      const AVPixFmtDescriptor *desc;
>>> -    AVDictionaryEntry *en = NULL;
>>> +    const AVDictionaryEntry av_unused *en = NULL;
>>>
>>>      // Update param from options
>>>      if (svt_enc->enc_mode >= -1)
>>> @@ -326,7 +326,7 @@ FF_ENABLE_DEPRECATION_WARNINGS
>>>      handle_side_data(avctx, param);
>>>
>>>  #if SVT_AV1_CHECK_VERSION(0, 9, 1)
>>> -    while ((en = av_dict_get(svt_enc->svtav1_opts, "", en, AV_DICT_IGNORE_SUFFIX))) {
>>> +    while ((en = av_dict_iterate(svt_enc->svtav1_opts, en))) {
>>>          EbErrorType ret = svt_av1_enc_parse_parameter(param, en->key, en->value);
>>>          if (ret != EB_ErrorNone) {
>>>              int level = (avctx->err_recognition & AV_EF_EXPLODE) ? AV_LOG_ERROR : AV_LOG_WARNING;
>>> @@ -336,7 +336,7 @@ FF_ENABLE_DEPRECATION_WARNINGS
>>>          }
>>>      }
>>>  #else
>>> -    if ((en = av_dict_get(svt_enc->svtav1_opts, "", NULL, AV_DICT_IGNORE_SUFFIX))) {
>>> +    if (av_dict_count(svt_enc->svtav1_opts)) {
>>>          int level = (avctx->err_recognition & AV_EF_EXPLODE) ? AV_LOG_ERROR : AV_LOG_WARNING;
>>>          av_log(avctx, level, "svt-params needs libavcodec to be compiled with SVT-AV1 "
>>>                               "headers >= 0.9.1.\n");
>>> diff --git a/libavcodec/libx264.c b/libavcodec/libx264.c
>>> index 2715f277f1..29d1a7ccbc 100644
>>> --- a/libavcodec/libx264.c
>>> +++ b/libavcodec/libx264.c
>>> @@ -1385,8 +1385,8 @@ FF_ENABLE_DEPRECATION_WARNINGS
>>>          x4->params.b_repeat_headers = 1;
>>>
>>>      {
>>> -        AVDictionaryEntry *en = NULL;
>>> -        while (en = av_dict_get(x4->x264_params, "", en, AV_DICT_IGNORE_SUFFIX)) {
>>> +        const AVDictionaryEntry *en = NULL;
>>> +        while (en = av_dict_iterate(x4->x264_params, en)) {
>>
>> Missing pair of braces to make the compiler not warn about assignment in check, no?
>>
>
> I did not change the parentheses in this patch at all as doing so would
> be orthogonal to the patch. We use this style without the additional
> pair of parentheses everywhere and actually add -Wno-parentheses in
> configure (line 7429).

Indeed sorry, I had pulled parts of AVDict out of tree when I did a bunch of work
on it a while ago to easier test a lot of edge cases and debug things.

Any reason why we silence this, as just adding a pair of braces to make intent
clearer vs accidentally doing it wrong somewhere seems worth it…

Anyway, LGTM then for this patch, sorry for the noise.

>
>>>             if ((ret = x264_param_parse(&x4->params, en->key, en->value)) < 0) {
>>>                 av_log(avctx, AV_LOG_WARNING,
>>>                        "Error parsing option '%s = %s'.\n",
>>> diff --git a/libavcodec/libx265.c b/libavcodec/libx265.c
>>> index c4ceffff5d..ac1dbc4f97 100644
>>> --- a/libavcodec/libx265.c
>>> +++ b/libavcodec/libx265.c
>>> @@ -495,8 +495,8 @@ FF_ENABLE_DEPRECATION_WARNINGS
>>>      }
>>>
>>>      {
>>> -        AVDictionaryEntry *en = NULL;
>>> -        while ((en = av_dict_get(ctx->x265_opts, "", en, AV_DICT_IGNORE_SUFFIX))) {
>>> +        const AVDictionaryEntry *en = NULL;
>>> +        while ((en = av_dict_iterate(ctx->x265_opts, en))) {
>>>              int parse_ret = ctx->api->param_parse(ctx->params, en->key, en->value);
>>>
>>>              switch (parse_ret) {
>>> diff --git a/libavformat/tee.c b/libavformat/tee.c
>>> index 0c0543fa65..1a2a8ead82 100644
>>> --- a/libavformat/tee.c
>>> +++ b/libavformat/tee.c
>>> @@ -313,7 +313,7 @@ static int open_slave(AVFormatContext *avf, char *slave, TeeSlave *tee_slave)
>>>      }
>>>
>>>      entry = NULL;
>>> -    while (entry = av_dict_get(bsf_options, "", NULL, AV_DICT_IGNORE_SUFFIX)) {
>>> +    while (entry = av_dict_iterate(bsf_options, NULL)) {
>>
>> Same as above
>>
>>>          const char *spec = entry->key;
>>>          if (*spec) {
>>>              if (strspn(spec, slave_bsfs_spec_sep) != 1) {
>>> @@ -390,7 +390,7 @@ static int open_slave(AVFormatContext *avf, char *slave, TeeSlave *tee_slave)
>>>
>>>      if (options) {
>>>          entry = NULL;
>>> -        while ((entry = av_dict_get(options, "", entry, AV_DICT_IGNORE_SUFFIX)))
>>> +        while ((entry = av_dict_iterate(options, entry)))
>>>              av_log(avf2, AV_LOG_ERROR, "Unknown option '%s'\n", entry->key);
>>>          ret = AVERROR_OPTION_NOT_FOUND;
>>>          goto end;
>>> -- 
>>> 2.40.1
>>
>> Otherwise LGTM, thanks!
>>
>
> _______________________________________________
> 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".
_______________________________________________
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".