| Message ID | 20220922020216.46589-1-epirat07@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [FFmpeg-devel,1/2] avutil/dict: Add av_dict_iterate | expand |
| 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 |
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, \
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".
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 --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, \