Re: [FFmpeg-devel] [PATCH 07/31] avutil: use av_dict_iterate

[James Almer <jamrial@gmail.com>]

On 11/26/2022 11:42 AM, Marvin Scholz wrote:
> 
> 
> On 25 Nov 2022, at 13:50, Andreas Rheinhardt wrote:
> 
>> Marvin Scholz:
>>> ---
>>>   libavutil/opt.c        | 12 ++++++------
>>>   libavutil/tests/dict.c | 10 +++++-----
>>>   2 files changed, 11 insertions(+), 11 deletions(-)
>>>
>>> diff --git a/libavutil/opt.c b/libavutil/opt.c
>>> index a3940f47fb..0a909a8b22 100644
>>> --- a/libavutil/opt.c
>>> +++ b/libavutil/opt.c
>>> @@ -1742,14 +1742,14 @@ void av_opt_free(void *obj)
>>>
>>>   int av_opt_set_dict2(void *obj, AVDictionary **options, int search_flags)
>>>   {
>>> -    AVDictionaryEntry *t = NULL;
>>> +    const AVDictionaryEntry *t = NULL;
>>>       AVDictionary    *tmp = NULL;
>>>       int ret;
>>>
>>>       if (!options)
>>>           return 0;
>>>
>>> -    while ((t = av_dict_get(*options, "", t, AV_DICT_IGNORE_SUFFIX))) {
>>> +    while ((t = av_dict_iterate(*options, t))) {
>>>           ret = av_opt_set(obj, t->key, t->value, search_flags);
>>>           if (ret == AVERROR_OPTION_NOT_FOUND)
>>>               ret = av_dict_set(&tmp, t->key, t->value, 0);
>>> @@ -2137,16 +2137,16 @@ FF_ENABLE_DEPRECATION_WARNINGS
>>>       case AV_OPT_TYPE_DICT: {
>>>           AVDictionary *dict1 = NULL;
>>>           AVDictionary *dict2 = *(AVDictionary **)dst;
>>> -        AVDictionaryEntry *en1 = NULL;
>>> -        AVDictionaryEntry *en2 = NULL;
>>> +        const AVDictionaryEntry *en1 = NULL;
>>> +        const AVDictionaryEntry *en2 = NULL;
>>>           ret = av_dict_parse_string(&dict1, o->default_val.str, "=", ":", 0);
>>>           if (ret < 0) {
>>>               av_dict_free(&dict1);
>>>               return ret;
>>>           }
>>>           do {
>>> -            en1 = av_dict_get(dict1, "", en1, AV_DICT_IGNORE_SUFFIX);
>>> -            en2 = av_dict_get(dict2, "", en2, AV_DICT_IGNORE_SUFFIX);
>>> +            en1 = av_dict_iterate(dict1, en1);
>>> +            en2 = av_dict_iterate(dict2, en2);
>>>           } while (en1 && en2 && !strcmp(en1->key, en2->key) && !strcmp(en1->value, en2->value));
>>>           av_dict_free(&dict1);
>>>           return (!en1 && !en2);
>>> diff --git a/libavutil/tests/dict.c b/libavutil/tests/dict.c
>>> index d053545f4d..8c05752ea7 100644
>>> --- a/libavutil/tests/dict.c
>>> +++ b/libavutil/tests/dict.c
>>> @@ -22,8 +22,8 @@
>>>
>>>   static void print_dict(const AVDictionary *m)
>>>   {
>>> -    AVDictionaryEntry *t = NULL;
>>> -    while ((t = av_dict_get(m, "", t, AV_DICT_IGNORE_SUFFIX)))
>>> +    const AVDictionaryEntry *t = NULL;
>>> +    while ((t = av_dict_iterate(m, t)))
>>>           printf("%s %s   ", t->key, t->value);
>>>       printf("\n");
>>>   }
>>> @@ -94,7 +94,7 @@ int main(void)
>>>       if (av_dict_get(dict, NULL, NULL, 0))
>>>           printf("av_dict_get() does not correctly handle NULL key.\n");
>>>       e = NULL;
>>> -    while ((e = av_dict_get(dict, "", e, AV_DICT_IGNORE_SUFFIX)))
>>> +    while ((e = av_dict_iterate(dict, e)))
>>>           printf("%s %s\n", e->key, e->value);
>>>       av_dict_free(&dict);
>>>
>>> @@ -106,7 +106,7 @@ int main(void)
>>>           printf("av_dict_set does not correctly handle NULL key\n");
>>>
>>>       e = NULL;
>>> -    while ((e = av_dict_get(dict, "", e, AV_DICT_IGNORE_SUFFIX)))
>>> +    while ((e = av_dict_iterate(dict, e)))
>>>           printf("'%s' '%s'\n", e->key, e->value);
>>>       av_dict_free(&dict);
>>>
>>> @@ -122,7 +122,7 @@ int main(void)
>>>       av_dict_set_int(&dict, "12", 1, 0);
>>>       av_dict_set_int(&dict, "12", 2, AV_DICT_APPEND);
>>>       e = NULL;
>>> -    while ((e = av_dict_get(dict, "", e, AV_DICT_IGNORE_SUFFIX)))
>>> +    while ((e = av_dict_iterate(dict, e)))
>>>           printf("%s %s\n", e->key, e->value);
>>>       av_dict_free(&dict);
>>>
>>
>> This stops testing the old iterating pattern; instead it should
>> explicitly test that both patterns coincide.
>>
> 
> Ok, I've removed this change for now from the pachset
> as adding tests is not really what this patchset is about.
> 
> Additionally I do not fully understand how the testing works yet
> so I don't know how I could write such a test.

This specific test just prints some hardcoded lines plus the output of 
av_dict_get() to stderr, and compares it with the reference in 
tests/ref/fate/dict.

What could be done is to either print both the output of the 
av_dict_get() and av_dict_iterate() iterations, or to compare the 
returned AVDictionaryEntry on each iteration and if they differ, print a 
line about it.
I can send a patch for that later.

> 
>> - Andreas
>>
>> _______________________________________________
>> 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".
_______________________________________________
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".