[FFmpeg-devel] [PATCH 1/3] lavformat: Prepare to make avio_enum_protocols const correct

[Andreas Rheinhardt]

Fu, Linjie:
>> From: ffmpeg-devel <ffmpeg-devel-bounces at ffmpeg.org> On Behalf Of
>> Andreas Rheinhardt
>> Sent: Friday, November 22, 2019 18:02
>> To: FFmpeg development discussions and patches <ffmpeg-
>> devel at 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.rheinhardt at 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.