Re: [FFmpeg-devel] [PATCH 02/35] lavu/fifo: make the contents of AVFifoBuffer private on next major bump

[Andreas Rheinhardt <andreas.rheinhardt@outlook.com>]

Anton Khirnov:
> There should be no good reason for the callers to access any of its
> contents.
> 
> Define a new type for the internal struct that currently matches
> AVFifoBuffer. This will allow adding new private fields without waiting
> for the major bump and will be useful in the following commits.
> 
> Unfortunately AVFifoBuffer fields cannot be marked as deprecated because
> it would trigger a warning wherever fifo.h is #included, due to
> inlined av_fifo_peek2().
> ---
>  doc/APIchanges         |  4 ++++
>  libavutil/fifo.c       | 23 +++++++++++++++++++----
>  libavutil/fifo.h       | 14 ++++++++++++--
>  libavutil/tests/fifo.c |  2 +-
>  libavutil/version.h    |  1 +
>  5 files changed, 37 insertions(+), 7 deletions(-)
> 
> diff --git a/doc/APIchanges b/doc/APIchanges
> index 8df0364e4c..21fa02ae9d 100644
> --- a/doc/APIchanges
> +++ b/doc/APIchanges
> @@ -14,6 +14,10 @@ libavutil:     2021-04-27
>  
>  API changes, most recent first:
>  
> +2022-01-xx - xxxxxxxxxx - lavu fifo.h
> +  Access to all AVFifoBuffer members is deprecated. The struct will
> +  become an incomplete type in a future major libavutil version.
> +
>  2022-01-04 - 78dc21b123e - lavu 57.16.100 - frame.h
>    Add AV_FRAME_DATA_DOVI_METADATA.
>  
> diff --git a/libavutil/fifo.c b/libavutil/fifo.c
> index f2f046b1f3..aaade01333 100644
> --- a/libavutil/fifo.c
> +++ b/libavutil/fifo.c
> @@ -28,9 +28,24 @@
>  
>  #define FIFO_SIZE_MAX FFMIN3((uint64_t)INT_MAX, (uint64_t)UINT32_MAX, (uint64_t)SIZE_MAX)
>  
> +#if FF_API_FIFO_PUBLIC
> +# define CTX_STRUCT_NAME FifoBuffer
> +#else
> +# define CTX_STRUCT_NAME AVFifoBuffer

This is invalid in pre-C11 (and will lead to compilation failures on old
GCC versions): Pre-C11, typedefs were subject to the ODR, yet you
already typedef AVFifoBuffer in fifo.h.

> +#endif
> +
> +typedef struct CTX_STRUCT_NAME {
> +    // These fields must match then contents of AVFifoBuffer in fifo.h

the contents

> +    // until FF_API_FIFO_PUBLIC is removed

The actual spec-compliant way for this is to add an AVFifoBuffer at the
start of this struct; it will also avoid the casts from FifoBuffer to
AVFifoBuffer. This will make the accesses to it a bit more cumbersome
and will mean more changes when FF_API_FIFO_PUBLIC is removed. (This
would not be an issue if we required support for anonymous structs
(mandatory in C11, supported by GCC and others long before that).)

> +    uint8_t *buffer;
> +    uint8_t *rptr, *wptr, *end;
> +    uint32_t rndx, wndx;
> +    /////////////////////////////////////////
> +} FifoBuffer;
> +
>  AVFifoBuffer *av_fifo_alloc_array(size_t nmemb, size_t size)
>  {
> -    AVFifoBuffer *f;
> +    FifoBuffer *f;
>      void *buffer;
>  
>      if (nmemb > FIFO_SIZE_MAX / size)
> @@ -39,15 +54,15 @@ AVFifoBuffer *av_fifo_alloc_array(size_t nmemb, size_t size)
>      buffer = av_realloc_array(NULL, nmemb, size);
>      if (!buffer)
>          return NULL;
> -    f = av_mallocz(sizeof(AVFifoBuffer));
> +    f = av_mallocz(sizeof(*f));
>      if (!f) {
>          av_free(buffer);
>          return NULL;
>      }
>      f->buffer = buffer;
>      f->end    = f->buffer + nmemb * size;
> -    av_fifo_reset(f);
> -    return f;
> +    av_fifo_reset((AVFifoBuffer*)f);
> +    return (AVFifoBuffer*)f;
>  }
>  
>  AVFifoBuffer *av_fifo_alloc(unsigned int size)
> diff --git a/libavutil/fifo.h b/libavutil/fifo.h
> index f4fd291e59..ca4e7fe060 100644
> --- a/libavutil/fifo.h
> +++ b/libavutil/fifo.h
> @@ -28,11 +28,21 @@
>  #include "avutil.h"
>  #include "attributes.h"
>  
> -typedef struct AVFifoBuffer {
> +#if FF_API_FIFO_PUBLIC
> +/**
> + * The contents of the struct are private and should not be accessed by the
> + * callers in any way.
> + */
> +#endif
> +typedef struct AVFifoBuffer
> +#if FF_API_FIFO_PUBLIC
> +{
>      uint8_t *buffer;
>      uint8_t *rptr, *wptr, *end;
>      uint32_t rndx, wndx;
> -} AVFifoBuffer;
> +}
> +#endif
> +AVFifoBuffer;
>  
>  /**
>   * Initialize an AVFifoBuffer.
> diff --git a/libavutil/tests/fifo.c b/libavutil/tests/fifo.c
> index a17d913233..e5aa88d252 100644
> --- a/libavutil/tests/fifo.c
> +++ b/libavutil/tests/fifo.c
> @@ -18,7 +18,7 @@
>  
>  #include <stdio.h>
>  #include <stdlib.h>
> -#include "libavutil/fifo.h"
> +#include "libavutil/fifo.c"
>  
>  int main(void)
>  {
> diff --git a/libavutil/version.h b/libavutil/version.h
> index 953aac9d94..7c031f547e 100644
> --- a/libavutil/version.h
> +++ b/libavutil/version.h
> @@ -110,6 +110,7 @@
>  #define FF_API_COLORSPACE_NAME          (LIBAVUTIL_VERSION_MAJOR < 58)
>  #define FF_API_AV_MALLOCZ_ARRAY         (LIBAVUTIL_VERSION_MAJOR < 58)
>  #define FF_API_FIFO_PEEK2               (LIBAVUTIL_VERSION_MAJOR < 58)
> +#define FF_API_FIFO_PUBLIC              (LIBAVUTIL_VERSION_MAJOR < 58)
>  
>  /**
>   * @}
> 

_______________________________________________
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".