Fu, Linjie: >> From: ffmpeg-devel <ffmpeg-devel-boun...@ffmpeg.org> On Behalf Of >> Andreas Rheinhardt >> Sent: Friday, November 22, 2019 18:02 >> To: FFmpeg development discussions and patches <ffmpeg- >> de...@ffmpeg.org> >> Subject: Re: [FFmpeg-devel] [PATCH 1/3] lavformat: Prepare to make >> avio_enum_protocols const correct >> >> Andreas Rheinhardt: >>> Andreas Rheinhardt: >>>> Using avio_enum_protocols works as follows: One initializes a pointer to >>>> void and gives avio_enum_protocols the address of said pointer as >>>> argument; the pointer will be updated to point to a member of the >>>> url_protocols array. Now the address of the pointer can be reused for >>>> another call to avio_enum_protocols. >>>> Said array consists of constant pointers (to constant URLProtocols), >>>> but the user now has a pointer to non-const to it; of course it was always >>>> intended that the user is not allowed to modify what the pointer points >>>> to and this has been enforced by hiding the real type of the underlying >>>> object. But it is better to use a const void ** as parameter to enforce >>>> this. This way avio_enum_protocols can be implemented without >> resorting >>>> to casting a const away or ignoring constness as is done currently. >>>> >>>> Given that this amounts to an ABI and API break, this can only be done >>>> at the next major version bump; as usual, the break is currently hidden >>>> behind an appropriate #if. >>>> >>>> It was also necessary to change the type of a pointer used in >>>> avio_enum_protocols. This makes the line that is not const correct move >>>> as long as the old function signature is used. With the new signature, >>>> avio_enum_protocols will be const correct. >>>> >>>> This change will eventually force changes in their callers, e.g. to >>>> show_protocols in fftools/cmdutils. (This function contains all currently >>>> existing calls to avio_enum_protocols in FFmpeg's codebase. It hasn't >>>> been touched in this commit.) >>>> >>>> Signed-off-by: Andreas Rheinhardt <andreas.rheinha...@gmail.com> >>>> --- >>>> libavformat/avio.h | 4 ++++ >>>> libavformat/protocols.c | 6 +++++- >>>> libavformat/version.h | 3 +++ >>>> 3 files changed, 12 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/libavformat/avio.h b/libavformat/avio.h >>>> index 9141642e75..e067ea8985 100644 >>>> --- a/libavformat/avio.h >>>> +++ b/libavformat/avio.h >>>> @@ -805,7 +805,11 @@ int avio_close_dyn_buf(AVIOContext *s, uint8_t >> **pbuffer); >>>> * >>>> * @return A static string containing the name of current protocol or NULL >>>> */ >>>> +#if FF_API_NONCONST_ENUM_PROTOCOLS >>>> const char *avio_enum_protocols(void **opaque, int output); >>>> +#else >>>> +const char *avio_enum_protocols(const void **opaque, int output); >>>> +#endif >>>> >>>> /** >>>> * Pause and resume playing - only meaningful if using a network >> streaming >>>> diff --git a/libavformat/protocols.c b/libavformat/protocols.c >>>> index ad95659795..c722f9a897 100644 >>>> --- a/libavformat/protocols.c >>>> +++ b/libavformat/protocols.c >>>> @@ -91,9 +91,13 @@ const AVClass >> *ff_urlcontext_child_class_next(const AVClass *prev) >>>> } >>>> >>>> >>>> +#if FF_API_NONCONST_ENUM_PROTOCOLS >>>> const char *avio_enum_protocols(void **opaque, int output) >>>> +#else >>>> +const char *avio_enum_protocols(const void **opaque, int output) >>>> +#endif >>>> { >>>> - const URLProtocol **p = *opaque; >>>> + const URLProtocol * const *p = *opaque; >>>> >>>> p = p ? p + 1 : url_protocols; >>>> *opaque = p; >>>> diff --git a/libavformat/version.h b/libavformat/version.h >>>> index 9814db8633..b0b9264382 100644 >>>> --- a/libavformat/version.h >>>> +++ b/libavformat/version.h >>>> @@ -106,6 +106,9 @@ >>>> #ifndef FF_API_AVIOFORMAT >>>> #define FF_API_AVIOFORMAT (LIBAVFORMAT_VERSION_MAJOR >> < 59) >>>> #endif >>>> +#ifndef FF_API_NONCONST_ENUM_PROTOCOLS >>>> +#define FF_API_NONCONST_ENUM_PROTOCOLS >> (LIBAVFORMAT_VERSION_MAJOR < 59) >>>> +#endif >>>> >>>> >>>> #ifndef FF_API_R_FRAME_RATE >>>> I am unsure what to do next given the feedback I received: If ABI >>> compability is no problem, then should I simply add the const and add >>> an entry to doc/APIchanges? Or is a deprecation period necessary? >>> >>> - Andreas >>> >> Ping for all three patches. >> > > Is it necessary to update the relevant code who calls avio_enum_protocols() > when this change takes effect? (either waiting for major version updates to > 59, > or applied this modification right now) > > Otherwise functions like show_protocols() may report warnings: > > Compile with FF_API_NONCONST_ENUM_PROTOCOLS forced to be 0 (gcc-7.3.0) > > Message: > fftools/cmdutils.c: In function ‘show_protocols’: > fftools/cmdutils.c:1681:40: warning: passing argument 1 of > ‘avio_enum_protocols’ from incompatible pointer type > [-Wincompatible-pointer-types] > while ((name = avio_enum_protocols(&opaque, 0))) > ^ > In file included from ./libavformat/avformat.h:321:0, > from fftools/cmdutils.c:34: > ./libavformat/avio.h:811:13: note: expected ‘const void **’ but argument is > of type ‘void **’ > const char *avio_enum_protocols(const void **opaque, int output); > ^~~~~~~~~~~~~~~~~~~ > fftools/cmdutils.c:1684:40: warning: passing argument 1 of > ‘avio_enum_protocols’ from incompatible pointer type > [-Wincompatible-pointer-types] > while ((name = avio_enum_protocols(&opaque, 1))) > ^ > In file included from ./libavformat/avformat.h:321:0, > from fftools/cmdutils.c:34: > ./libavformat/avio.h:811:13: note: expected ‘const void **’ but argument is > of type ‘void **’ > const char *avio_enum_protocols(const void **opaque, int output); >
Yes, updating the callers is necessary (as I already said in the commit message), but easy. The typical old call is (the ... usually includes a loop): void *opaque = NULL; char *name; ... name = avio_enum_protocols(&opaque, flag); ... Changing the type of opaque to const void* will make this code const-correct. (The opaque now points to one of the constant pointers in the url_protocols array, so the user having a pointer to nonconst to them is wrong.) - Andreas PS: The reason that the callers need to update their code is that there is no automatic conversion from type ** to const type **, so that an explicit cast is required. Such an automatic conversion would be dangerous as it could be used to remove const from any pointer without a cast. See http://c-faq.com/ansi/constmismatch.html PPS: There would be another way to get rid of this warning: Instead of making the void **opaque store a pointer to a pointer to a const entry of the url_protocols array one make the opaque point to a value of type uintptr_t that stores an index in the url_protocols array. This would still require a cast, but it would not cast const away. But I don't really like this approach, as forcing the user to use pointers to const void makes it clearer that the user is not to meddle with the opaque. _______________________________________________ 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".