| Message ID | d12f6c95bc514b0d8451541393cc5afc@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel,2.1/3] libavformat/protocols.c: avio_enum_protocols(): Split declaration and initialization | expand |
| Context | Check | Description |
|---|---|---|
| andriy/configure | warning | Failed to apply patch |
Michael Witten: > In the repo, there is only one function that enumerates protocols: > > fftools/cmdutils.c: show_protocols() > > This commit simply has that function make calls directly to the > desired functions, namely: > > * avio_enum_protocols_for_input() > * avio_enum_protocols_for_output() > --- > fftools/cmdutils.c | 4 ++-- > libavformat/avio.h | 22 ++++++++++++++++++++++ > libavformat/protocols.c | 2 -- > 3 files changed, 24 insertions(+), 4 deletions(-) > > diff --git a/fftools/cmdutils.c b/fftools/cmdutils.c > index 64237a4796..87f9be64c3 100644 > --- a/fftools/cmdutils.c > +++ b/fftools/cmdutils.c > @@ -1681,10 +1681,10 @@ int show_protocols(void *optctx, const char *opt, const char *arg) > > printf("Supported file protocols:\n" > "Input:\n"); > - while ((name = avio_enum_protocols(&opaque, 0))) > + while ((name = avio_enum_protocols_for_input(&opaque))) > printf(" %s\n", name); > printf("Output:\n"); > - while ((name = avio_enum_protocols(&opaque, 1))) > + while ((name = avio_enum_protocols_for_output(&opaque))) You did not reset opaque before the second call; instead you are relying on undocumented behaviour, namely that opaque is reset to NULL when no further protocol exists. (Instead the implementation could also make opaque point to the sentinel of the array of protocols.) In fact, the current code already relied on this, but with two functions it is worse, because in the second loop the opaque for the second function does not come from a call of avio_enum_protocols_for_output() at all. > printf(" %s\n", name); > return 0; > } > diff --git a/libavformat/avio.h b/libavformat/avio.h > index 3b92cf742a..455b872260 100644 > --- a/libavformat/avio.h > +++ b/libavformat/avio.h > @@ -788,6 +788,28 @@ int avio_close_dyn_buf(AVIOContext *s, uint8_t **pbuffer); > */ > const char *avio_enum_protocols(void **const opaque, const int output); > > +/** > + * Iterate through names of available output protocols. > + * > + * @param opaque A private pointer representing current protocol. > + * It must be a pointer to NULL on first iteration and will > + * be updated by successive calls to avio_enum_protocols_for_output. > + * > + * @return A static string containing the name of current protocol or NULL > + */ > +const char *avio_enum_protocols_for_output(void **const opaque); > + > +/** > + * Iterate through names of available input protocols. > + * > + * @param opaque A private pointer representing current protocol. > + * It must be a pointer to NULL on first iteration and will > + * be updated by successive calls to avio_enum_protocols_for_input. > + * > + * @return A static string containing the name of current protocol or NULL > + */ > +const char *avio_enum_protocols_for_input(void **const opaque); I don't think it is worth adding two more public symbols for this. > + > /** > * Get AVClass by names of available protocols. > * > diff --git a/libavformat/protocols.c b/libavformat/protocols.c > index 4cb8ae0b63..ca04ed2eb5 100644 > --- a/libavformat/protocols.c > +++ b/libavformat/protocols.c > @@ -102,13 +102,11 @@ const AVClass *ff_urlcontext_child_class_iterate(void **iter) > *opaque = NULL; \ > return NULL; > > -static inline > const char *avio_enum_protocols_for_output(void **const opaque) > { > AVIO_ENUM_PROTOCOLS(url_write); > } > > -static inline > const char *avio_enum_protocols_for_input(void **const opaque) > { > AVIO_ENUM_PROTOCOLS(url_read); >
| Michael Witten:
|
| > In the repo, there is only one function that enumerates protocols:
| >
| > fftools/cmdutils.c: show_protocols()
| >
| > This commit simply has that function make calls directly to the
| > desired functions, namely:
| >
| > * avio_enum_protocols_for_input()
| > * avio_enum_protocols_for_output()
| >
| > [...]
| > - while ((name = avio_enum_protocols(&opaque, 0)))
| > + while ((name = avio_enum_protocols_for_input(&opaque)))
| > printf(" %s\n", name);
| > printf("Output:\n");
| > - while ((name = avio_enum_protocols(&opaque, 1)))
| > + while ((name = avio_enum_protocols_for_output(&opaque)))
|
| Andreas Rheinhardt:
|
| > You did not reset opaque before the second call; instead you are relying
| > on undocumented behaviour, namely that opaque is reset to NULL when no
| > further protocol exists. (Instead the implementation could also make
| > opaque point to the sentinel of the array of protocols.) In fact, the
| > current code already relied on this, but with two functions it is worse,
| > because in the second loop the opaque for the second function does not
| > come from a call of avio_enum_protocols_for_output() at all.
| >
| > [...]
| >
| > I don't think it is worth adding two more public symbols for this.
* Indeed, I did not attempt to alter 'show_protocols()', other than to
replace one API call with another, more direct API call.
* Perhaps it should simply be documented that 'opaque' is set to NULL.
* I would not be opposed to dropping this patch; it is just a potentiality.
Sincerely,
Michael WItten
diff --git a/fftools/cmdutils.c b/fftools/cmdutils.c index 64237a4796..87f9be64c3 100644 --- a/fftools/cmdutils.c +++ b/fftools/cmdutils.c @@ -1681,10 +1681,10 @@ int show_protocols(void *optctx, const char *opt, const char *arg) printf("Supported file protocols:\n" "Input:\n"); - while ((name = avio_enum_protocols(&opaque, 0))) + while ((name = avio_enum_protocols_for_input(&opaque))) printf(" %s\n", name); printf("Output:\n"); - while ((name = avio_enum_protocols(&opaque, 1))) + while ((name = avio_enum_protocols_for_output(&opaque))) printf(" %s\n", name); return 0; } diff --git a/libavformat/avio.h b/libavformat/avio.h index 3b92cf742a..455b872260 100644 --- a/libavformat/avio.h +++ b/libavformat/avio.h @@ -788,6 +788,28 @@ int avio_close_dyn_buf(AVIOContext *s, uint8_t **pbuffer); */ const char *avio_enum_protocols(void **const opaque, const int output); +/** + * Iterate through names of available output protocols. + * + * @param opaque A private pointer representing current protocol. + * It must be a pointer to NULL on first iteration and will + * be updated by successive calls to avio_enum_protocols_for_output. + * + * @return A static string containing the name of current protocol or NULL + */ +const char *avio_enum_protocols_for_output(void **const opaque); + +/** + * Iterate through names of available input protocols. + * + * @param opaque A private pointer representing current protocol. + * It must be a pointer to NULL on first iteration and will + * be updated by successive calls to avio_enum_protocols_for_input. + * + * @return A static string containing the name of current protocol or NULL + */ +const char *avio_enum_protocols_for_input(void **const opaque); + /** * Get AVClass by names of available protocols. * diff --git a/libavformat/protocols.c b/libavformat/protocols.c index 4cb8ae0b63..ca04ed2eb5 100644 --- a/libavformat/protocols.c +++ b/libavformat/protocols.c @@ -102,13 +102,11 @@ const AVClass *ff_urlcontext_child_class_iterate(void **iter) *opaque = NULL; \ return NULL; -static inline const char *avio_enum_protocols_for_output(void **const opaque) { AVIO_ENUM_PROTOCOLS(url_write); } -static inline const char *avio_enum_protocols_for_input(void **const opaque) { AVIO_ENUM_PROTOCOLS(url_read);