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

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@gmail.com>
---
 libavformat/avio.h      | 4 ++++
 libavformat/protocols.c | 6 +++++-
 libavformat/version.h   | 3 +++
 3 files changed, 12 insertions(+), 1 deletion(-)

ons 2019-08-21 klockan 11:04 +0200 skrev 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.

I'm fairly sure this is only an API break. C ABI doesn't care about
constness. But also:

> @@ -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

This should still be perfectly compatible with all user code since
adding const is fine..

/Tomas

Tomas Härdin:
> ons 2019-08-21 klockan 11:04 +0200 skrev 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.
> 
> I'm fairly sure this is only an API break. C ABI doesn't care about
> constness. But also:
> >> @@ -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
> 
> This should still be perfectly compatible with all user code since
> adding const is fine..
> 
No. void* can be safely and automatically converted to const void*;
and the conversion from void** to void * const * is fine, too, but the
conversion from void ** to const void ** is not safe for the reason
already mentioned. I'll explain it once more. Imagine
avio_enum_protocols already used a const void ** parameter and were
const-correct and we called the function in the following way:

void *opaque = NULL;
avio_enum_protocols(&opaque, 0);

opaque now points to something const (namely the first element of the
url_protocols array, an array of const pointers (to constant
URLProtocols)), but opaque is not a pointer to something const, i.e. a
violation of the const system has happened. Therefore one needs an
explicit cast for this unsafe conversion; or the compiler complains.
It is of course easy to fix this: Simply declare opaque as const void
*. As mentioned already, the caller has no business modifying what
this pointer points to anyway.

- Andreas

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@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

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@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.

- Andreas

> 在 2019年11月22日，18:02，Andreas Rheinhardt <andreas.rheinhardt@gmail.com> 写道：
> 
> 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@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.

You can fix the typo of the title, other things LGTM.
> 
> - Andreas
> _______________________________________________
> 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".


Thanks
--------------------------------
刘歧|CDN|研发总监
吉林省高升科技有限公司北京分公司
地址：北京市海淀区西三环北路87号国际财经中心B座9层
手机：18611075131
邮箱: liuqi@gosun.com
电话：010-82602628



Thanks
Steven

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

- Linjie

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

> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of
> Andreas Rheinhardt
> Sent: Wednesday, March 18, 2020 21:29
> To: ffmpeg-devel@ffmpeg.org
> Subject: Re: [FFmpeg-devel] [PATCH 1/3] lavformat: Prepare to make
> avio_enum_protocols const correct
> 
> Fu, Linjie:
> 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.

No more concerns, this fix looks reasonable.
Thanks for the kind elaborations.

- Linjie

On 21/08/2019 10:04, Andreas Rheinhardt wrote:
> +#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

Do we actually allow breakage like this (even at major bumps)?

That is, you are not using a new function name, but breaking an existing
function signature. As far as I know, this is not somthing that we, or
semver, allow.

- Derek

Am Fr., 20. März 2020 um 19:33 Uhr schrieb Derek Buitenhuis
<derek.buitenhuis@gmail.com>:
>
> On 21/08/2019 10:04, Andreas Rheinhardt wrote:
> > +#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
>
> Do we actually allow breakage like this (even at major bumps)?
>
> That is, you are not using a new function name, but breaking an existing
> function signature. As far as I know, this is not somthing that we, or
> semver, allow.

We are only breaking it for C++ users, right?

Carl Eugen

Derek Buitenhuis:
> On 21/08/2019 10:04, Andreas Rheinhardt wrote:
>> +#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
> 
> Do we actually allow breakage like this (even at major bumps)?
> 
> That is, you are not using a new function name, but breaking an existing
> function signature. As far as I know, this is not somthing that we, or
> semver, allow.

There have been several such breakages in the past, at least in relation
to const:
Looking through some headers with git blame leads to the following
commits which add const to the type of the pointed-to type:
9b07d34e, b90d7840, 53abe324, c4ba5198, 069cf86d, 0d34dbc2, 5bb3f882,
adb0e941, 0a0f19b5, 2d3acbfe, 74aeb6b5, 7215fcf8, 77cc958f, 4a37d4b3,
cc4c2420, 0020c54c.
2d3acbfe furthermore changed the return value of a public function from
AVCodecDescriptor * to const AVCodecDescriptor *; f2c86705 and 5c439b41
did something similar.

Except for f2c86705, none of these commits bumped any version. None
added an APIchanges entry; 5c439b41 was the only one that waited until
the next major version bump with its changes.

Furthermore, several such changes are planned for libavformat. Just
search for ff_const59 in avformat.h. 3aa6208d even changed AVInputFormat
** to const AVInputFormat **. (I pondered whether I should piggyback on
this (and use ff_const59 myself), but in the end decided against it. I'd
choose differently now.)

Furthermore, I don't see where semver disallows changing the signature
of a function. After all, breaking changes are explicitly allowed at
major version bumps. But we should probably add an entry in APIchanges
for this change and all the other ff_const59 changes.

(Looking through APIchanges also leads to 2c031741 and 84c03869, but
given that the latter of these only incremented minor despite breaking
API/ABI and the former didn't increment the version at all despite
breaking API/ABI makes them bad precedents. Lest I forget: av_malloc()
and other memory-allocation functions did not start with size_t parameters.)

- Andreas

PS: Actually, there are a couple of functions that need const:
av_find_best_stream() provides a way to get a pointer to a non-const
AVCodec (it casts the const away internally); and
av_input/output_audio/video_device_next() needs either be modified to
work with const or be deprecated altogether (as the other _next-APIs
have been).

Am Fr., 20. März 2020 um 23:29 Uhr schrieb Andreas Rheinhardt
<andreas.rheinhardt@gmail.com>:

> PS: Actually, there are a couple of functions that need const:
> av_find_best_stream() provides a way to get a pointer to a non-const
> AVCodec (it casts the const away internally); and
> av_input/output_audio/video_device_next() needs either be modified to
> work with const or be deprecated altogether (as the other _next-APIs
> have been).

Will you work on constifying this functions?

Carl Eugen

Carl Eugen Hoyos:
> Am Fr., 20. März 2020 um 23:29 Uhr schrieb Andreas Rheinhardt
> <andreas.rheinhardt@gmail.com>:
> 
>> PS: Actually, there are a couple of functions that need const:
>> av_find_best_stream() provides a way to get a pointer to a non-const
>> AVCodec (it casts the const away internally); and
>> av_input/output_audio/video_device_next() needs either be modified to
>> work with const or be deprecated altogether (as the other _next-APIs
>> have been).
> 
> Will you work on constifying this functions?
> 
Feel free to do it if you like.

- Andreas

PS: AVCodecParser should probably be made const on the next bump, too;
unfortunately 7e8eba2d forgot to deprecate the AVCodecParser.next field.