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

Submitted by Andreas Rheinhardt on Aug. 21, 2019, 9:04 a.m.

Details

Message ID 20190821090438.10260-2-andreas.rheinhardt@gmail.com
State New
Headers show

Commit Message

Andreas Rheinhardt Aug. 21, 2019, 9:04 a.m.
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(-)

Comments

Tomas Härdin Aug. 21, 2019, 10:01 a.m.
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
Andreas Rheinhardt Aug. 21, 2019, 10:51 a.m.
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 Sept. 11, 2019, 11:41 p.m.
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

Patch hide | download patch | download mbox

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