Re: [FFmpeg-devel] [PATCH 03/35] lavu/fifo: introduce the notion of element size

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

Anton Khirnov:
> Many AVFifoBuffer users operate on fixed-size elements (e.g. pointers),
> but the current FIFO API deals exclusively in bytes, requiring extra
> complexity in all these callers.
> 
> Add a new AVFifoBuffer constructor creating a FIFO with an element size
> that may be larger than a byte. All operations on such a FIFO then
> operate on complete elements.
> ---
>  doc/APIchanges      |   6 ++
>  libavutil/fifo.c    | 194 ++++++++++++++++++++++++++++++--------------
>  libavutil/fifo.h    |  53 +++++++++---
>  libavutil/version.h |   2 +-
>  4 files changed, 179 insertions(+), 76 deletions(-)
> 
> diff --git a/doc/APIchanges b/doc/APIchanges
> index 21fa02ae9d..5646cf2278 100644
> --- a/doc/APIchanges
> +++ b/doc/APIchanges
> @@ -14,6 +14,12 @@ libavutil:     2021-04-27
>  
>  API changes, most recent first:
>  
> +2022-01-xx - xxxxxxxxxx - lavu 57.19.100 - fifo.h
> +  Add av_fifo_alloc2(), which allows setting a FIFO element size.
> +  Operations on FIFOs created with this function on these elements
> +  rather than bytes.
> +  Add av_fifo_elem_size().
> +
>  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.
> diff --git a/libavutil/fifo.c b/libavutil/fifo.c
> index aaade01333..fc7c93470f 100644
> --- a/libavutil/fifo.c
> +++ b/libavutil/fifo.c
> @@ -38,20 +38,28 @@ typedef struct CTX_STRUCT_NAME {
>      // These fields must match then contents of AVFifoBuffer in fifo.h
>      // until FF_API_FIFO_PUBLIC is removed
>      uint8_t *buffer;
> +#if FF_API_FIFO_PUBLIC
>      uint8_t *rptr, *wptr, *end;
>      uint32_t rndx, wndx;
> +#endif
>      /////////////////////////////////////////
> +
> +    size_t elem_size, nb_elems;
> +    size_t offset_r, offset_w;
> +    // distinguishes the ambigous situation offset_r == offset_w

ambiguous

> +    int    is_empty;
>  } FifoBuffer;
>  
> -AVFifoBuffer *av_fifo_alloc_array(size_t nmemb, size_t size)
> +AVFifoBuffer *av_fifo_alloc2(size_t nb_elems, size_t elem_size,
> +                             unsigned int flags)
>  {
>      FifoBuffer *f;
>      void *buffer;
>  
> -    if (nmemb > FIFO_SIZE_MAX / size)
> +    if (nb_elems > FIFO_SIZE_MAX / elem_size)
>          return NULL;
>  
> -    buffer = av_realloc_array(NULL, nmemb, size);
> +    buffer = av_realloc_array(NULL, nb_elems, elem_size);
>      if (!buffer)
>          return NULL;
>      f = av_mallocz(sizeof(*f));
> @@ -60,11 +68,24 @@ AVFifoBuffer *av_fifo_alloc_array(size_t nmemb, size_t size)
>          return NULL;
>      }
>      f->buffer = buffer;
> -    f->end    = f->buffer + nmemb * size;
> +#if FF_API_FIFO_PUBLIC
> +    f->end    = f->buffer + nb_elems * elem_size;
> +#endif
> +
> +    f->nb_elems  = nb_elems;
> +    f->elem_size = elem_size;
> +
>      av_fifo_reset((AVFifoBuffer*)f);
>      return (AVFifoBuffer*)f;
>  }
>  
> +AVFifoBuffer *av_fifo_alloc_array(size_t nmemb, size_t size)
> +{
> +    if (nmemb > SIZE_MAX / size)
> +        return NULL;
> +    return av_fifo_alloc2(nmemb * size, 1, 0);
> +}
> +
>  AVFifoBuffer *av_fifo_alloc(unsigned int size)
>  {
>      return av_fifo_alloc_array(size, 1);
> @@ -88,67 +109,85 @@ void av_fifo_freep(AVFifoBuffer **f)
>  
>  void av_fifo_reset(AVFifoBuffer *f)
>  {
> +    FifoBuffer *fb = (FifoBuffer*)f;
> +
> +    fb->offset_r = 0;
> +    fb->offset_w = 0;
> +    fb->is_empty = 1;
> +#if FF_API_FIFO_PUBLIC
>      f->wptr = f->rptr = f->buffer;
>      f->wndx = f->rndx = 0;
> +#endif
> +}
> +
> +size_t av_fifo_elem_size(const AVFifoBuffer *f)
> +{
> +    const FifoBuffer *fb = (const FifoBuffer*)f;
> +    return fb->elem_size;
>  }
>  
>  int av_fifo_size(const AVFifoBuffer *f)
>  {
> -    return (uint32_t)(f->wndx - f->rndx);
> +    const FifoBuffer *fb = (const FifoBuffer*)f;
> +    if (fb->offset_w <= fb->offset_r && !fb->is_empty)
> +        return fb->nb_elems - fb->offset_r + fb->offset_w;
> +    return fb->offset_w - fb->offset_r;
>  }
>  
>  int av_fifo_space(const AVFifoBuffer *f)
>  {
> -    return f->end - f->buffer - av_fifo_size(f);
> +    const FifoBuffer *fb = (const FifoBuffer*)f;
> +    return fb->nb_elems - av_fifo_size(f);
>  }
>  
>  int av_fifo_realloc2(AVFifoBuffer *f, unsigned int new_size)
>  {
> -    unsigned int old_size = f->end - f->buffer;
> +    FifoBuffer *fb = (FifoBuffer*)f;
>  
>      if (new_size > FIFO_SIZE_MAX)
>          return AVERROR(EINVAL);
>  
> -    if (old_size < new_size) {
> -        size_t offset_r = f->rptr - f->buffer;
> -        size_t offset_w = f->wptr - f->buffer;
> +    if (fb->nb_elems < new_size) {
>          uint8_t *tmp;
>  
> -        tmp = av_realloc(f->buffer, new_size);
> +        tmp = av_realloc_array(f->buffer, new_size, fb->elem_size);
>          if (!tmp)
>              return AVERROR(ENOMEM);
>  
>          // move the data from the beginning of the ring buffer
>          // to the newly allocated space
> -        // the second condition distinguishes full vs empty fifo
> -        if (offset_w <= offset_r && av_fifo_size(f)) {
> -            const size_t copy = FFMIN(new_size - old_size, offset_w);
> -            memcpy(tmp + old_size, tmp, copy);
> -            if (copy < offset_w) {
> -                memmove(tmp, tmp + copy , offset_w - copy);
> -                offset_w -= copy;
> +        if (fb->offset_w <= fb->offset_r && !fb->is_empty) {
> +            const size_t copy = FFMIN(new_size - fb->nb_elems, fb->offset_w);
> +            memcpy(tmp + fb->nb_elems * fb->elem_size, tmp, copy * fb->elem_size);
> +            if (copy < fb->offset_w) {
> +                memmove(tmp, tmp + copy * fb->elem_size,
> +                        (fb->offset_w - copy) * fb->elem_size);
> +                fb->offset_w -= copy;
>              } else
> -                offset_w = old_size + copy;
> +                fb->offset_w = fb->nb_elems + copy;
>          }
>  
>          f->buffer = tmp;
> +#if FF_API_FIFO_PUBLIC
>          f->end    = f->buffer + new_size;
> -        f->rptr   = f->buffer + offset_r;
> -        f->wptr   = f->buffer + offset_w;
> +        f->rptr   = f->buffer + fb->offset_r * fb->elem_size;
> +        f->wptr   = f->buffer + fb->offset_w * fb->elem_size;
> +#endif
> +        fb->nb_elems = new_size;
>      }
>      return 0;
>  }
>  
>  int av_fifo_grow(AVFifoBuffer *f, unsigned int size)
>  {
> -    unsigned int old_size = f->end - f->buffer;
> -    if(size + (unsigned)av_fifo_size(f) < size)
> +    FifoBuffer *fb = (FifoBuffer*)f;
> +
> +    if (size > UINT_MAX - av_fifo_size(f))
>          return AVERROR(EINVAL);
>  
>      size += av_fifo_size(f);
> -
> -    if (old_size < size)
> -        return av_fifo_realloc2(f, FFMAX(size, 2*old_size));
> +    if (fb->nb_elems < size)
> +        return av_fifo_realloc2(f, FFMAX(size, 2 * fb->nb_elems));
>      return 0;
>  }
>  
> @@ -157,62 +196,78 @@ int av_fifo_grow(AVFifoBuffer *f, unsigned int size)
>  int av_fifo_generic_write(AVFifoBuffer *f, void *src, int size,
>                            int (*func)(void *, void *, int))
>  {
> +    FifoBuffer *fb = (FifoBuffer*)f;
>      int total = size;
> +    size_t offset_w = fb->offset_w;
> +#if FF_API_FIFO_PUBLIC
>      uint32_t wndx= f->wndx;
> -    uint8_t *wptr= f->wptr;
> +#endif
>  
>      if (size > av_fifo_space(f))
>          return AVERROR(ENOSPC);
>  
>      do {
> -        int len = FFMIN(f->end - wptr, size);
> +        size_t    len = FFMIN(fb->nb_elems - offset_w, size);
> +        uint8_t *wptr = f->buffer + offset_w * fb->elem_size;
> +
>          if (func) {
> -            len = func(src, wptr, len);
> -            if (len <= 0)
> +            int ret = func(src, wptr, len);
> +            if (ret <= 0)
>                  break;
> +            len = ret;
>          } else {
> -            memcpy(wptr, src, len);
> -            src = (uint8_t *)src + len;
> +            memcpy(wptr, src, len * fb->elem_size);
> +            src = (uint8_t *)src + len * fb->elem_size;
>          }
> -        wptr += len;
> -        if (wptr >= f->end)
> -            wptr = f->buffer;
> +        offset_w += len;
> +        if (offset_w >= fb->nb_elems)
> +            offset_w = 0;
> +#if FF_API_FIFO_PUBLIC
>          wndx    += len;
> +#endif
>          size    -= len;
>      } while (size > 0);
> +#if FF_API_FIFO_PUBLIC
>      f->wndx= wndx;
> -    f->wptr= wptr;
> +    f->wptr= f->buffer + offset_w * fb->elem_size;
> +#endif
> +    fb->offset_w = offset_w;
> +
> +    if (total - size > 0)

size < total

> +        fb->is_empty = 0;
> +
>      return total - size;
>  }
>  
>  int av_fifo_generic_peek_at(AVFifoBuffer *f, void *dest, int offset, int buf_size, void (*func)(void*, void*, int))
>  {
> -    uint8_t *rptr = f->rptr;
> +    FifoBuffer *fb = (FifoBuffer*)f;
> +    size_t offset_r = fb->offset_r;
>  
>      if (offset < 0 || buf_size > av_fifo_size(f) - offset)
>          return AVERROR(EINVAL);
>  
> -    if (offset >= f->end - rptr)
> -        rptr += offset - (f->end - f->buffer);
> +    if (offset >= fb->nb_elems - offset_r)
> +        offset_r -= fb->nb_elems - offset;
>      else
> -        rptr += offset;
> +        offset_r += offset;
>  
>      while (buf_size > 0) {
> +        uint8_t *rptr = f->buffer + offset_r * fb->elem_size;
>          int len;
>  
> -        if (rptr >= f->end)
> -            rptr -= f->end - f->buffer;
> -
> -        len = FFMIN(f->end - rptr, buf_size);
> +        len = FFMIN(fb->nb_elems - offset_r, buf_size);
>          if (func)
>              func(dest, rptr, len);
>          else {
> -            memcpy(dest, rptr, len);
> -            dest = (uint8_t *)dest + len;
> +            memcpy(dest, rptr, len * fb->elem_size);
> +            dest = (uint8_t *)dest + len * fb->elem_size;
>          }
>  
>          buf_size -= len;
> -        rptr     += len;
> +        offset_r += len;
> +        if (offset_r >= fb->nb_elems)
> +            offset_r -= fb->nb_elems;
>      }
>  
>      return 0;
> @@ -221,22 +276,24 @@ int av_fifo_generic_peek_at(AVFifoBuffer *f, void *dest, int offset, int buf_siz
>  int av_fifo_generic_peek(AVFifoBuffer *f, void *dest, int buf_size,
>                           void (*func)(void *, void *, int))
>  {
> -    uint8_t *rptr = f->rptr;
> +    FifoBuffer *fb = (FifoBuffer*)f;
> +    size_t offset_r = fb->offset_r;
>  
>      if (buf_size > av_fifo_size(f))
>          return AVERROR(EINVAL);
>  
>      do {
> -        int len = FFMIN(f->end - rptr, buf_size);
> +        uint8_t *rptr = f->buffer + offset_r * fb->elem_size;
> +        int len = FFMIN(fb->nb_elems - offset_r, buf_size);
>          if (func)
>              func(dest, rptr, len);
>          else {
> -            memcpy(dest, rptr, len);
> -            dest = (uint8_t *)dest + len;
> +            memcpy(dest, rptr, len * fb->elem_size);
> +            dest = (uint8_t *)dest + len * fb->elem_size;
>          }
> -        rptr += len;
> -        if (rptr >= f->end)
> -            rptr -= f->end - f->buffer;
> +        offset_r += len;
> +        if (offset_r >= fb->nb_elems)
> +            offset_r -= fb->nb_elems;
>          buf_size -= len;
>      } while (buf_size > 0);
>  
> @@ -246,16 +303,19 @@ int av_fifo_generic_peek(AVFifoBuffer *f, void *dest, int buf_size,
>  int av_fifo_generic_read(AVFifoBuffer *f, void *dest, int buf_size,
>                           void (*func)(void *, void *, int))
>  {
> +    FifoBuffer *fb = (FifoBuffer*)f;
> +
>      if (buf_size > av_fifo_size(f))
>          return AVERROR(EINVAL);
>  
>      do {
> -        int len = FFMIN(f->end - f->rptr, buf_size);
> +        uint8_t *rptr = f->buffer + fb->offset_r * fb->elem_size;
> +        int len = FFMIN(fb->nb_elems - fb->offset_r, buf_size);
>          if (func)
> -            func(dest, f->rptr, len);
> +            func(dest, rptr, len);
>          else {
> -            memcpy(dest, f->rptr, len);
> -            dest = (uint8_t *)dest + len;
> +            memcpy(dest, rptr, len * fb->elem_size);
> +            dest = (uint8_t *)dest + len * fb->elem_size;
>          }
>          av_fifo_drain(f, len);
>          buf_size -= len;
> @@ -266,9 +326,19 @@ int av_fifo_generic_read(AVFifoBuffer *f, void *dest, int buf_size,
>  /** Discard data from the FIFO. */
>  void av_fifo_drain(AVFifoBuffer *f, int size)
>  {
> -    av_assert2(av_fifo_size(f) >= size);
> -    f->rptr += size;
> -    if (f->rptr >= f->end)
> -        f->rptr -= f->end - f->buffer;
> +    FifoBuffer *fb = (FifoBuffer*)f;
> +    const size_t cur_size = av_fifo_size(f);
> +
> +    av_assert2(cur_size >= size);
> +    if (cur_size == size)
> +        fb->is_empty = 1;
> +
> +    if (fb->offset_r >= fb->nb_elems - size)
> +        fb->offset_r -= fb->nb_elems - size;
> +    else
> +        fb->offset_r += size;
> +#if FF_API_FIFO_PUBLIC
> +    f->rptr  = f->buffer + fb->offset_r * fb->elem_size;
>      f->rndx += size;
> +#endif
>  }
> diff --git a/libavutil/fifo.h b/libavutil/fifo.h
> index ca4e7fe060..22362c0239 100644
> --- a/libavutil/fifo.h
> +++ b/libavutil/fifo.h
> @@ -48,6 +48,9 @@ AVFifoBuffer;
>   * Initialize an AVFifoBuffer.
>   * @param size of FIFO
>   * @return AVFifoBuffer or NULL in case of memory allocation failure
> + *
> + * @note the element size of the allocated FIFO will be 1, i.e. all operations
> + *       will be in bytes
>   */
>  AVFifoBuffer *av_fifo_alloc(unsigned int size);
>  
> @@ -56,9 +59,26 @@ AVFifoBuffer *av_fifo_alloc(unsigned int size);
>   * @param nmemb number of elements
>   * @param size  size of the single element
>   * @return AVFifoBuffer or NULL in case of memory allocation failure
> + *
> + * @note the element size of the allocated FIFO will be 1, i.e. all operations
> + *       will be in bytes
>   */
>  AVFifoBuffer *av_fifo_alloc_array(size_t nmemb, size_t size);
>  
> +/**
> + * Allocate and initialize an AVFifoBuffer with a given element size.
> + *
> + * @param f pointer to the newly-allocated FIFO will be written here on success
> + * @param nb_elems initial number of elements that can be stored in the FIFO
> + * @param elem_size Size in bytes of a single element. Further operations on
> + *                  the returned FIFO will implicitly use this element size.
> + * @param flags currently unused, must be 0
> + *
> + * @return newly-allocated AVFifoBuffer on success, a negative error code on failure
> + */
> +AVFifoBuffer *av_fifo_alloc2(size_t elems, size_t elem_size,
> +                             unsigned int flags);
> +
>  /**
>   * Free an AVFifoBuffer.
>   * @param f AVFifoBuffer to free
> @@ -71,6 +91,12 @@ void av_fifo_free(AVFifoBuffer *f);
>   */
>  void av_fifo_freep(AVFifoBuffer **f);
>  
> +/**
> + * @return Element size for FIFO operations. This element size is set at
> + *         FIFO allocation and remains constant during its lifetime
> + */
> +size_t av_fifo_elem_size(const AVFifoBuffer *f);
> +
>  /**
>   * Reset the AVFifoBuffer to the state right after av_fifo_alloc, in particular it is emptied.
>   * @param f AVFifoBuffer to reset
> @@ -78,16 +104,16 @@ void av_fifo_freep(AVFifoBuffer **f);
>  void av_fifo_reset(AVFifoBuffer *f);
>  
>  /**
> - * Return the amount of data in bytes in the AVFifoBuffer, that is the
> - * amount of data you can read from it.
> + * Return the amount of data in the AVFifoBuffer, that is the amount of data
> + * (in elements, as returned by av_fifo_elem_size()) you can read from it.
>   * @param f AVFifoBuffer to read from
>   * @return size
>   */
>  int av_fifo_size(const AVFifoBuffer *f);
>  
>  /**
> - * Return the amount of space in bytes in the AVFifoBuffer, that is the
> - * amount of data you can write into it.
> + * Return the amount of space in the AVFifoBuffer, that is the amount of data
> + * (in elements, as returned by av_fifo_elem_size()) you can write into it.
>   * @param f AVFifoBuffer to write into
>   * @return size
>   */
> @@ -97,8 +123,8 @@ int av_fifo_space(const AVFifoBuffer *f);
>   * Feed data at specific position from an AVFifoBuffer to a user-supplied callback.
>   * Similar as av_fifo_gereric_read but without discarding data.
>   * @param f AVFifoBuffer to read from
> - * @param offset offset from current read position
> - * @param buf_size number of bytes to read
> + * @param offset offset in elements from current read position
> + * @param buf_size number of elements to read
>   * @param func generic read function
>   * @param dest data destination
>   *
> @@ -110,7 +136,7 @@ int av_fifo_generic_peek_at(AVFifoBuffer *f, void *dest, int offset, int buf_siz
>   * Feed data from an AVFifoBuffer to a user-supplied callback.
>   * Similar as av_fifo_gereric_read but without discarding data.
>   * @param f AVFifoBuffer to read from
> - * @param buf_size number of bytes to read
> + * @param buf_size number of elements to read
>   * @param func generic read function
>   * @param dest data destination
>   *
> @@ -121,7 +147,7 @@ int av_fifo_generic_peek(AVFifoBuffer *f, void *dest, int buf_size, void (*func)
>  /**
>   * Feed data from an AVFifoBuffer to a user-supplied callback.
>   * @param f AVFifoBuffer to read from
> - * @param buf_size number of bytes to read
> + * @param buf_size number of elements to read
>   * @param func generic read function
>   * @param dest data destination
>   *
> @@ -134,13 +160,13 @@ int av_fifo_generic_read(AVFifoBuffer *f, void *dest, int buf_size, void (*func)
>   * @param f AVFifoBuffer to write to
>   * @param src data source; non-const since it may be used as a
>   * modifiable context by the function defined in func
> - * @param size number of bytes to write
> + * @param size number of elements to write
>   * @param func generic write function; the first parameter is src,
>   * the second is dest_buf, the third is dest_buf_size.
>   * func must return the number of bytes written to dest_buf, or <= 0 to
>   * indicate no more data available to write.
>   * If func is NULL, src is interpreted as a simple byte array for source data.
> - * @return the number of bytes written to the FIFO or a negative error code on failure
> + * @return the number of elements written to the FIFO or a negative error code on failure
>   */
>  int av_fifo_generic_write(AVFifoBuffer *f, void *src, int size, int (*func)(void*, void*, int));
>  
> @@ -149,7 +175,7 @@ int av_fifo_generic_write(AVFifoBuffer *f, void *src, int size, int (*func)(void
>   * In case of reallocation failure, the old FIFO is kept unchanged.
>   *
>   * @param f AVFifoBuffer to resize
> - * @param size new AVFifoBuffer size in bytes
> + * @param size new AVFifoBuffer size in elements
>   * @return <0 for failure, >=0 otherwise
>   */
>  int av_fifo_realloc2(AVFifoBuffer *f, unsigned int size);
> @@ -160,7 +186,8 @@ int av_fifo_realloc2(AVFifoBuffer *f, unsigned int size);
>   * The new fifo size may be larger than the requested size.
>   *
>   * @param f AVFifoBuffer to resize
> - * @param additional_space the amount of space in bytes to allocate in addition to av_fifo_size()
> + * @param additional_space the amount of space in elements to allocate in
> + *                         addition to av_fifo_size()
>   * @return <0 for failure, >=0 otherwise
>   */
>  int av_fifo_grow(AVFifoBuffer *f, unsigned int additional_space);
> @@ -168,7 +195,7 @@ int av_fifo_grow(AVFifoBuffer *f, unsigned int additional_space);
>  /**
>   * Read and discard the specified amount of data from an AVFifoBuffer.
>   * @param f AVFifoBuffer to read from
> - * @param size amount of data to read in bytes
> + * @param size amount of data to read in elements
>   */
>  void av_fifo_drain(AVFifoBuffer *f, int size);
>  
> diff --git a/libavutil/version.h b/libavutil/version.h
> index 7c031f547e..180ebd5223 100644
> --- a/libavutil/version.h
> +++ b/libavutil/version.h
> @@ -79,7 +79,7 @@
>   */
>  
>  #define LIBAVUTIL_VERSION_MAJOR  57
> -#define LIBAVUTIL_VERSION_MINOR  18
> +#define LIBAVUTIL_VERSION_MINOR  19
>  #define LIBAVUTIL_VERSION_MICRO 100
>  
>  #define LIBAVUTIL_VERSION_INT   AV_VERSION_INT(LIBAVUTIL_VERSION_MAJOR, \
> 

You should document that av_fifo_peek2() must not be used with a fifo
returned by av_fifo_alloc2(). (It has not been ported to use elements
instead of bytes and its documentation just claims "The used buffer size
can be checked with av_fifo_size()" which is not wrong but does not
really mention that this now returns the element count.)
_______________________________________________
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".