| Message ID | 1517600658-32681-7-git-send-email-josh@itanimul.li |
|---|---|
| State | Accepted |
| Commit | 26d879c1cea176d4f0e0a47d4b641c86495aa0e8 |
| Headers | show |
On Fri, 2 Feb 2018 19:44:18 +0000 Josh de Kock <josh@itanimul.li> wrote: > --- > fftools/cmdutils.c | 2 +- > libavcodec/avcodec.h | 6 +++++- > libavcodec/bitstream_filter.c | 4 ++-- > libavcodec/bitstream_filters.c | 29 ++++++++++++++++++----------- > 4 files changed, 26 insertions(+), 15 deletions(-) > > diff --git a/fftools/cmdutils.c b/fftools/cmdutils.c > index 9ecc51b..0b06ccc 100644 > --- a/fftools/cmdutils.c > +++ b/fftools/cmdutils.c > @@ -1586,7 +1586,7 @@ int show_bsfs(void *optctx, const char *opt, const char *arg) > void *opaque = NULL; > > printf("Bitstream filters:\n"); > - while ((bsf = av_bsf_next(&opaque))) > + while ((bsf = av_bsf_iterate(&opaque))) > printf("%s\n", bsf->name); > printf("\n"); > return 0; > diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h > index 99f5fb9..c41779a 100644 > --- a/libavcodec/avcodec.h > +++ b/libavcodec/avcodec.h > @@ -5728,7 +5728,7 @@ attribute_deprecated > void av_bitstream_filter_close(AVBitStreamFilterContext *bsf); > /** > * @deprecated the old bitstream filtering API (using AVBitStreamFilterContext) > - * is deprecated. Use av_bsf_next() from the new bitstream filtering API (using > + * is deprecated. Use av_bsf_iterate() from the new bitstream filtering API (using > * AVBSFContext). > */ > attribute_deprecated > @@ -5750,7 +5750,11 @@ const AVBitStreamFilter *av_bsf_get_by_name(const char *name); > * @return the next registered bitstream filter or NULL when the iteration is > * finished > */ > +const AVBitStreamFilter *av_bsf_iterate(void **opaque); > +#if FF_API_NEXT > +attribute_deprecated > const AVBitStreamFilter *av_bsf_next(void **opaque); > +#endif > > /** > * Allocate a context for a given bitstream filter. The caller must fill in the > diff --git a/libavcodec/bitstream_filter.c b/libavcodec/bitstream_filter.c > index ed1cf33..ca11ed3 100644 > --- a/libavcodec/bitstream_filter.c > +++ b/libavcodec/bitstream_filter.c > @@ -34,9 +34,9 @@ const AVBitStreamFilter *av_bitstream_filter_next(const AVBitStreamFilter *f) > void *opaque = NULL; > > while (filter != f) > - filter = av_bsf_next(&opaque); > + filter = av_bsf_iterate(&opaque); > > - return av_bsf_next(&opaque); > + return av_bsf_iterate(&opaque); > } > > void av_register_bitstream_filter(AVBitStreamFilter *bsf) > diff --git a/libavcodec/bitstream_filters.c b/libavcodec/bitstream_filters.c > index 7b0cb50..338ef82 100644 > --- a/libavcodec/bitstream_filters.c > +++ b/libavcodec/bitstream_filters.c > @@ -52,7 +52,7 @@ extern const AVBitStreamFilter ff_vp9_superframe_split_bsf; > > #include "libavcodec/bsf_list.c" > > -const AVBitStreamFilter *av_bsf_next(void **opaque) > +const AVBitStreamFilter *av_bsf_iterate(void **opaque) > { > uintptr_t i = (uintptr_t)*opaque; > const AVBitStreamFilter *f = bitstream_filters[i]; > @@ -63,12 +63,18 @@ const AVBitStreamFilter *av_bsf_next(void **opaque) > return f; > } > > +#if FF_API_NEXT > +const AVBitStreamFilter *av_bsf_next(void **opaque) { > + return av_bsf_iterate(opaque); > +} > +#endif > + > const AVBitStreamFilter *av_bsf_get_by_name(const char *name) > { > - int i; > + const AVBitStreamFilter *f = NULL; > + void *i = 0; > > - for (i = 0; bitstream_filters[i]; i++) { > - const AVBitStreamFilter *f = bitstream_filters[i]; > + while ((f = av_bsf_iterate(&i))) { > if (!strcmp(f->name, name)) > return f; > } > @@ -78,19 +84,20 @@ const AVBitStreamFilter *av_bsf_get_by_name(const char *name) > > const AVClass *ff_bsf_child_class_next(const AVClass *prev) > { > - int i; > + const AVBitStreamFilter *f = NULL; > + void *i = 0; > > /* find the filter that corresponds to prev */ > - for (i = 0; prev && bitstream_filters[i]; i++) { > - if (bitstream_filters[i]->priv_class == prev) { > - i++; > + while (prev && (f = av_bsf_iterate(&i))) { > + if (f->priv_class == prev) { > break; > } > } > > /* find next filter with priv options */ > - for (; bitstream_filters[i]; i++) > - if (bitstream_filters[i]->priv_class) > - return bitstream_filters[i]->priv_class; > + while ((f = av_bsf_iterate(&i))) { > + if (f->priv_class) > + return f->priv_class; > + } > return NULL; > } I like _next better than _iterate (as others have also said), but I think I can tolerate it. At least it'll be consistent across all those APIs.
On Sat, Feb 3, 2018 at 5:39 PM, wm4 <nfxjfg@googlemail.com> wrote: > On Fri, 2 Feb 2018 19:44:18 +0000 > Josh de Kock <josh@itanimul.li> wrote: > >> --- >> fftools/cmdutils.c | 2 +- >> libavcodec/avcodec.h | 6 +++++- >> libavcodec/bitstream_filter.c | 4 ++-- >> libavcodec/bitstream_filters.c | 29 ++++++++++++++++++----------- >> 4 files changed, 26 insertions(+), 15 deletions(-) >> >> diff --git a/fftools/cmdutils.c b/fftools/cmdutils.c >> index 9ecc51b..0b06ccc 100644 >> --- a/fftools/cmdutils.c >> +++ b/fftools/cmdutils.c >> @@ -1586,7 +1586,7 @@ int show_bsfs(void *optctx, const char *opt, const char *arg) >> void *opaque = NULL; >> >> printf("Bitstream filters:\n"); >> - while ((bsf = av_bsf_next(&opaque))) >> + while ((bsf = av_bsf_iterate(&opaque))) >> printf("%s\n", bsf->name); >> printf("\n"); >> return 0; >> diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h >> index 99f5fb9..c41779a 100644 >> --- a/libavcodec/avcodec.h >> +++ b/libavcodec/avcodec.h >> @@ -5728,7 +5728,7 @@ attribute_deprecated >> void av_bitstream_filter_close(AVBitStreamFilterContext *bsf); >> /** >> * @deprecated the old bitstream filtering API (using AVBitStreamFilterContext) >> - * is deprecated. Use av_bsf_next() from the new bitstream filtering API (using >> + * is deprecated. Use av_bsf_iterate() from the new bitstream filtering API (using >> * AVBSFContext). >> */ >> attribute_deprecated >> @@ -5750,7 +5750,11 @@ const AVBitStreamFilter *av_bsf_get_by_name(const char *name); >> * @return the next registered bitstream filter or NULL when the iteration is >> * finished >> */ >> +const AVBitStreamFilter *av_bsf_iterate(void **opaque); >> +#if FF_API_NEXT >> +attribute_deprecated >> const AVBitStreamFilter *av_bsf_next(void **opaque); >> +#endif >> >> /** >> * Allocate a context for a given bitstream filter. The caller must fill in the >> diff --git a/libavcodec/bitstream_filter.c b/libavcodec/bitstream_filter.c >> index ed1cf33..ca11ed3 100644 >> --- a/libavcodec/bitstream_filter.c >> +++ b/libavcodec/bitstream_filter.c >> @@ -34,9 +34,9 @@ const AVBitStreamFilter *av_bitstream_filter_next(const AVBitStreamFilter *f) >> void *opaque = NULL; >> >> while (filter != f) >> - filter = av_bsf_next(&opaque); >> + filter = av_bsf_iterate(&opaque); >> >> - return av_bsf_next(&opaque); >> + return av_bsf_iterate(&opaque); >> } >> >> void av_register_bitstream_filter(AVBitStreamFilter *bsf) >> diff --git a/libavcodec/bitstream_filters.c b/libavcodec/bitstream_filters.c >> index 7b0cb50..338ef82 100644 >> --- a/libavcodec/bitstream_filters.c >> +++ b/libavcodec/bitstream_filters.c >> @@ -52,7 +52,7 @@ extern const AVBitStreamFilter ff_vp9_superframe_split_bsf; >> >> #include "libavcodec/bsf_list.c" >> >> -const AVBitStreamFilter *av_bsf_next(void **opaque) >> +const AVBitStreamFilter *av_bsf_iterate(void **opaque) >> { >> uintptr_t i = (uintptr_t)*opaque; >> const AVBitStreamFilter *f = bitstream_filters[i]; >> @@ -63,12 +63,18 @@ const AVBitStreamFilter *av_bsf_next(void **opaque) >> return f; >> } >> >> +#if FF_API_NEXT >> +const AVBitStreamFilter *av_bsf_next(void **opaque) { >> + return av_bsf_iterate(opaque); >> +} >> +#endif >> + >> const AVBitStreamFilter *av_bsf_get_by_name(const char *name) >> { >> - int i; >> + const AVBitStreamFilter *f = NULL; >> + void *i = 0; >> >> - for (i = 0; bitstream_filters[i]; i++) { >> - const AVBitStreamFilter *f = bitstream_filters[i]; >> + while ((f = av_bsf_iterate(&i))) { >> if (!strcmp(f->name, name)) >> return f; >> } >> @@ -78,19 +84,20 @@ const AVBitStreamFilter *av_bsf_get_by_name(const char *name) >> >> const AVClass *ff_bsf_child_class_next(const AVClass *prev) >> { >> - int i; >> + const AVBitStreamFilter *f = NULL; >> + void *i = 0; >> >> /* find the filter that corresponds to prev */ >> - for (i = 0; prev && bitstream_filters[i]; i++) { >> - if (bitstream_filters[i]->priv_class == prev) { >> - i++; >> + while (prev && (f = av_bsf_iterate(&i))) { >> + if (f->priv_class == prev) { >> break; >> } >> } >> >> /* find next filter with priv options */ >> - for (; bitstream_filters[i]; i++) >> - if (bitstream_filters[i]->priv_class) >> - return bitstream_filters[i]->priv_class; >> + while ((f = av_bsf_iterate(&i))) { >> + if (f->priv_class) >> + return f->priv_class; >> + } >> return NULL; >> } > > I like _next better than _iterate (as others have also said), but I > think I can tolerate it. At least it'll be consistent across all those > APIs. What about av*iterate(int index)? As you suggested in https://ffmpeg.org/pipermail/ffmpeg-devel/2018-January/224702.html Anyway av_bsf_iterate() previously didn't exist, so we can freely choose how it will be called (with int index or void **opaque). Thank's.
Muhammad Faiz (2018-02-04):
> What about av*iterate(int index)?
I like the idea of an index better than the other options evoked,
especially the linked-list-like APIs. It is also more similar to the
APIs for enumerated types.
Another option would be to return the whole list in a mallocated array,
in a single call.
Regards,
On Sun, Feb 04, 2018 at 02:46:09PM +0100, Nicolas George wrote: > Muhammad Faiz (2018-02-04): > > What about av*iterate(int index)? This makes no sense, it's then not an API for iteration of components. > > I like the idea of an index better than the other options evoked, > especially the linked-list-like APIs. It is also more similar to the > APIs for enumerated types. If we were to add in APIs which allowed you to register external components again, this idea wouldn't work well as indexes wouldn't necessarily correspond to the component which it previously did after you register extra components. The main benefit of the opaque pointers is the flexibility of how components are stored/represented internally, and being able to change/extend it with no API changes (only additions). Sure there is the question of 'how efficient is it?', but it's not really a relevant question considering how frequently the iteration functions are called (not much relative to other parts of the library where efficiency is more crucial). > Another option would be to return the whole list in a mallocated array, > in a single call. When has exposing more internals ever ended up going well?
Josh de Kock (2018-02-04): > If we were to add in APIs which allowed you to register external components > again, this idea wouldn't work well as indexes wouldn't necessarily correspond > to the component which it previously did after you register extra components. That is not a problem if the documentation states "index change when components are registered". > > Another option would be to return the whole list in a mallocated array, > > in a single call. > When has exposing more internals ever ended up going well? This suggestion does not expose any internal at all. Are you sure you read it correctly? Regards,
On Sun, 4 Feb 2018 22:29:10 +0100 Nicolas George <george@nsup.org> wrote: > Josh de Kock (2018-02-04): > > If we were to add in APIs which allowed you to register external components > > again, this idea wouldn't work well as indexes wouldn't necessarily correspond > > to the component which it previously did after you register extra components. > > That is not a problem if the documentation states "index change when > components are registered". > > > > Another option would be to return the whole list in a mallocated array, > > > in a single call. > > When has exposing more internals ever ended up going well? > > This suggestion does not expose any internal at all. Are you sure you > read it correctly? It would require that all future implementations use an array, until we deprecate and introduce new APIs again. I suggest we stay focused, instead of getting nothing done again due to bikeshedding. Let's just go with the current naming/signature?
Josh de Kock (2018-02-04): > The main benefit of the opaque pointers is the flexibility of how components > are stored/represented internally, and being able to change/extend it with > no API changes (only additions). Sure there is the question of 'how efficient > is it?', but it's not really a relevant question considering how frequently > the iteration functions are called (not much relative to other parts of the > library where efficiency is more crucial). Sorry, I forgot to answer that part. Using an index has the same good properties. And so does returning the list at once (I ask again: did you read the suggestion properly? It is not about returning an internal array containing the list, it is about building a new array to return it to the caller; it exposes no internal at all.) Therefore, this argument cannot be use to decide between the three possibilities. Using an iterator with an opaque pointer has the drawback of forcing the iteration order: always in order, always from the beginning. It also has the drawback of requiring documentation on the validity of the opaque pointer. This is an API we intend to keep for a long time, we should try to pick the best one, the one that gathers the most consensus. Regards,
On Mon, Feb 5, 2018 at 4:29 AM, Nicolas George <george@nsup.org> wrote: > Josh de Kock (2018-02-04): >> If we were to add in APIs which allowed you to register external components >> again, this idea wouldn't work well as indexes wouldn't necessarily correspond >> to the component which it previously did after you register extra components. > > That is not a problem if the documentation states "index change when > components are registered". Index isn't changed when extra component is added. Because the extra component will reside in the last index + 1. Thank's.
diff --git a/fftools/cmdutils.c b/fftools/cmdutils.c index 9ecc51b..0b06ccc 100644 --- a/fftools/cmdutils.c +++ b/fftools/cmdutils.c @@ -1586,7 +1586,7 @@ int show_bsfs(void *optctx, const char *opt, const char *arg) void *opaque = NULL; printf("Bitstream filters:\n"); - while ((bsf = av_bsf_next(&opaque))) + while ((bsf = av_bsf_iterate(&opaque))) printf("%s\n", bsf->name); printf("\n"); return 0; diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h index 99f5fb9..c41779a 100644 --- a/libavcodec/avcodec.h +++ b/libavcodec/avcodec.h @@ -5728,7 +5728,7 @@ attribute_deprecated void av_bitstream_filter_close(AVBitStreamFilterContext *bsf); /** * @deprecated the old bitstream filtering API (using AVBitStreamFilterContext) - * is deprecated. Use av_bsf_next() from the new bitstream filtering API (using + * is deprecated. Use av_bsf_iterate() from the new bitstream filtering API (using * AVBSFContext). */ attribute_deprecated @@ -5750,7 +5750,11 @@ const AVBitStreamFilter *av_bsf_get_by_name(const char *name); * @return the next registered bitstream filter or NULL when the iteration is * finished */ +const AVBitStreamFilter *av_bsf_iterate(void **opaque); +#if FF_API_NEXT +attribute_deprecated const AVBitStreamFilter *av_bsf_next(void **opaque); +#endif /** * Allocate a context for a given bitstream filter. The caller must fill in the diff --git a/libavcodec/bitstream_filter.c b/libavcodec/bitstream_filter.c index ed1cf33..ca11ed3 100644 --- a/libavcodec/bitstream_filter.c +++ b/libavcodec/bitstream_filter.c @@ -34,9 +34,9 @@ const AVBitStreamFilter *av_bitstream_filter_next(const AVBitStreamFilter *f) void *opaque = NULL; while (filter != f) - filter = av_bsf_next(&opaque); + filter = av_bsf_iterate(&opaque); - return av_bsf_next(&opaque); + return av_bsf_iterate(&opaque); } void av_register_bitstream_filter(AVBitStreamFilter *bsf) diff --git a/libavcodec/bitstream_filters.c b/libavcodec/bitstream_filters.c index 7b0cb50..338ef82 100644 --- a/libavcodec/bitstream_filters.c +++ b/libavcodec/bitstream_filters.c @@ -52,7 +52,7 @@ extern const AVBitStreamFilter ff_vp9_superframe_split_bsf; #include "libavcodec/bsf_list.c" -const AVBitStreamFilter *av_bsf_next(void **opaque) +const AVBitStreamFilter *av_bsf_iterate(void **opaque) { uintptr_t i = (uintptr_t)*opaque; const AVBitStreamFilter *f = bitstream_filters[i]; @@ -63,12 +63,18 @@ const AVBitStreamFilter *av_bsf_next(void **opaque) return f; } +#if FF_API_NEXT +const AVBitStreamFilter *av_bsf_next(void **opaque) { + return av_bsf_iterate(opaque); +} +#endif + const AVBitStreamFilter *av_bsf_get_by_name(const char *name) { - int i; + const AVBitStreamFilter *f = NULL; + void *i = 0; - for (i = 0; bitstream_filters[i]; i++) { - const AVBitStreamFilter *f = bitstream_filters[i]; + while ((f = av_bsf_iterate(&i))) { if (!strcmp(f->name, name)) return f; } @@ -78,19 +84,20 @@ const AVBitStreamFilter *av_bsf_get_by_name(const char *name) const AVClass *ff_bsf_child_class_next(const AVClass *prev) { - int i; + const AVBitStreamFilter *f = NULL; + void *i = 0; /* find the filter that corresponds to prev */ - for (i = 0; prev && bitstream_filters[i]; i++) { - if (bitstream_filters[i]->priv_class == prev) { - i++; + while (prev && (f = av_bsf_iterate(&i))) { + if (f->priv_class == prev) { break; } } /* find next filter with priv options */ - for (; bitstream_filters[i]; i++) - if (bitstream_filters[i]->priv_class) - return bitstream_filters[i]->priv_class; + while ((f = av_bsf_iterate(&i))) { + if (f->priv_class) + return f->priv_class; + } return NULL; }