diff mbox series

[FFmpeg-devel,1/2] avutil/dict: Add av_dict_iterate

Message ID 20220922020216.46589-1-epirat07@gmail.com
State New
Headers show
Series [FFmpeg-devel,1/2] avutil/dict: Add av_dict_iterate | expand

Checks

Context Check Description
andriy/configure_x86 warning Failed to run configure
yinshiyou/make_loongarch64 success Make finished
yinshiyou/make_fate_loongarch64 success Make fate finished

Commit Message

Marvin Scholz Sept. 22, 2022, 2:02 a.m. UTC
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(-)

Comments

Andreas Rheinhardt Sept. 22, 2022, 11:33 a.m. UTC | #1
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. 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).

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.

> +
>  /**
>   * 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).

>  
>  #define LIBAVUTIL_VERSION_INT   AV_VERSION_INT(LIBAVUTIL_VERSION_MAJOR, \
>                                                 LIBAVUTIL_VERSION_MINOR, \
Marvin Scholz Sept. 22, 2022, 11:47 a.m. UTC | #2
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?

> 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…

>
> 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-request@ffmpeg.org with subject "unsubscribe".
Andreas Rheinhardt Sept. 22, 2022, 11:56 a.m. UTC | #3
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, \
>>
diff mbox series

Patch

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);
+
 /**
  * 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
 
 #define LIBAVUTIL_VERSION_INT   AV_VERSION_INT(LIBAVUTIL_VERSION_MAJOR, \
                                                LIBAVUTIL_VERSION_MINOR, \