Marvin Scholz:
>
>
> On 22 Sep 2022, at 13:33, Andreas Rheinhardt wrote:
>
>> Marvin Scholz:
>>> This is a more explicit iteration API rather than using the "magic"
>>> av_dict_get(d, "", t, AV_DICT_IGNORE_SUFFIX) which is not really
>>> trivial to grasp what it does when casually reading through code.
>>> ---
>>> libavutil/dict.c | 19 +++++++++++++++++++
>>> libavutil/dict.h | 27 +++++++++++++++++++++++++++
>>> libavutil/version.h | 2 +-
>>> 3 files changed, 47 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/libavutil/dict.c b/libavutil/dict.c
>>> index 14ad780a79..2f690a5b8e 100644
>>> --- a/libavutil/dict.c
>>> +++ b/libavutil/dict.c
>>> @@ -20,6 +20,7 @@
>>>
>>> #include <string.h>
>>>
>>> +#include "avassert.h"
>>> #include "avstring.h"
>>> #include "dict.h"
>>> #include "dict_internal.h"
>>> @@ -38,6 +39,24 @@ int av_dict_count(const AVDictionary *m)
>>> return m ? m->count : 0;
>>> }
>>>
>>> +AVDictionaryEntry *av_dict_iterate(const AVDictionary *m,
>>> + const AVDictionaryEntry *prev)
>>> +{
>>> + int i = 0;
>>> +
>>> + if (!m)
>>> + return NULL;
>>> +
>>> + if (prev)
>>> + i = prev - m->elems + 1;
>>> +
>>> + av_assert2(i >= 0);
>>> + if (i >= m->count)
>>> + return NULL;
>>> +
>>> + return &m->elems[i];
>>> +}
>>> +
>>> AVDictionaryEntry *av_dict_get(const AVDictionary *m, const char *key,
>>> const AVDictionaryEntry *prev, int flags)
>>> {
>>> diff --git a/libavutil/dict.h b/libavutil/dict.h
>>> index 0d1afc6c64..b42b3f07fd 100644
>>> --- a/libavutil/dict.h
>>> +++ b/libavutil/dict.h
>>> @@ -32,6 +32,8 @@
>>>
>>> #include <stdint.h>
>>>
>>> +#include "attributes.h"
>>> +
>>> /**
>>> * @addtogroup lavu_dict AVDictionary
>>> * @ingroup lavu_data
>>> @@ -101,6 +103,31 @@ typedef struct AVDictionary AVDictionary;
>>> AVDictionaryEntry *av_dict_get(const AVDictionary *m, const char *key,
>>> const AVDictionaryEntry *prev, int flags);
>>>
>>> +/**
>>> + * Iterate over a dictionary
>>> + *
>>> + * Iterates through all entries in the dictionary.
>>> + *
>>> + * @warning The returned AVDictionaryEntry key/value must not be changed.
>>> + *
>>> + * @param m The dictionary to iterate over
>>> + * @param prev Pointer to the previous AVDictionaryEntry, NULL initially
>>> + *
>>> + * @retval AVDictionaryEntry* The next element in the dictionary
>>> + * @retval NULL No more elements in the dictionary
>>> + *
>>> + * Typical usage:
>>> + * @code
>>> + * AVDictionaryEntry *e = NULL;
>>> + * while (e = av_dict_iterate(m, e)) {
>>> + * // ...
>>> + * }
>>> + * @endcode
>>> + */
>>> +av_warn_unused_result
>>> +AVDictionaryEntry *av_dict_iterate(const AVDictionary *m,
>>> + const AVDictionaryEntry *prev);
>>
>> The user is not allowed to modify the returned AVDictionaryEntries, so
>> you should return a const AVDictionaryEntry here.
>
> Ok.
>
> Shouldn't _get return const as well then? If so, can that be changed or would
> it break ABI?
>
It is an API break and therefore has to wait until a major bump.
>> And there is no reason
>> to use av_warn_unused_result at all; nothing bad happens if you ignore
>> the result (except that you called av_dict_iterate unnecessarily).
>
> Yeah nothing bad happens but it is probably still wrong use of the API
> that seemed beneficial to highlight or is there any valid use-case
> to call it without using the return? I could not think of any…
>
By that philosophy, basically all our API functions would need the
av_warn_unused_result attribute. I think this attribute should stand out
to alert the user that he is doing something with grave consequences if
he ignores the result.
>>
>> Furthermore, av_dict_set's documentation contains "Adding a new entry to
>> a dictionary invalidates all existing entries previously returned with
>> av_dict_get." This needs to be updated, too.
>
> Sure, I will update the docs more, I just wanted to have some general
> feedback if this is acceptable addition before I invest more time to
> polish it.
>
>>
>>> +
>>> /**
>>> * Get number of entries in dictionary.
>>> *
>>> diff --git a/libavutil/version.h b/libavutil/version.h
>>> index 0585fa7b80..6fd07ed2a4 100644
>>> --- a/libavutil/version.h
>>> +++ b/libavutil/version.h
>>> @@ -80,7 +80,7 @@
>>>
>>> #define LIBAVUTIL_VERSION_MAJOR 57
>>> #define LIBAVUTIL_VERSION_MINOR 36
>>> -#define LIBAVUTIL_VERSION_MICRO 102
>>> +#define LIBAVUTIL_VERSION_MICRO 103
>>
>> New API additions need a minor bump (and need to reset micro).
>
> Oh indeed, sorry, will fix that in the next version.
>
>>
>>>
>>> #define LIBAVUTIL_VERSION_INT AV_VERSION_INT(LIBAVUTIL_VERSION_MAJOR, \
>>> LIBAVUTIL_VERSION_MINOR, \
>>
_______________________________________________
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".
