[FFmpeg-devel] [PATCH 08/35] lavu/fifo: add new FIFO read/peek functions

[Andreas Rheinhardt]

Anton Khirnov:
> As for writing, use separate functions for reading to a buffer and a
> callback. Allow the callbacks to limit the amount of data read,
> similarly to what is done for writing.
> 
> Consistently use size_t for sizes.
> ---
>  doc/APIchanges   |  3 ++-
>  libavutil/fifo.c | 68 ++++++++++++++++++++++++++++++++++++++++++++++++
>  libavutil/fifo.h | 60 ++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 130 insertions(+), 1 deletion(-)
> 
> diff --git a/doc/APIchanges b/doc/APIchanges
> index 0b179c30e5..f64759d69d 100644
> --- a/doc/APIchanges
> +++ b/doc/APIchanges
> @@ -19,7 +19,8 @@ API changes, most recent first:
>    Operations on FIFOs created with this function on these elements
>    rather than bytes.
>    Add av_fifo_elem_size(), av_fifo_can_read(), av_fifo_can_write(),
> -  av_fifo_grow2(), av_fifo_drain2(), av_fifo_write(), av_fifo_write_from_cb().
> +  av_fifo_grow2(), av_fifo_drain2(), av_fifo_write(), av_fifo_write_from_cb(),
> +  av_fifo_read(), av_fifo_read_to_cb(), av_fifo_peek(), av_fifo_peek_to_cb().
>  
>  2022-01-xx - xxxxxxxxxx - lavu fifo.h
>    Access to all AVFifoBuffer members is deprecated. The struct will
> diff --git a/libavutil/fifo.c b/libavutil/fifo.c
> index 1d94fff457..ea944bc936 100644
> --- a/libavutil/fifo.c
> +++ b/libavutil/fifo.c
> @@ -265,6 +265,74 @@ int av_fifo_write_from_cb(AVFifoBuffer *f, AVFifoCB read_cb,
>      return fifo_write_common(f, NULL, nb_elems, read_cb, opaque);
>  }
>  
> +static int fifo_peek_common(const AVFifoBuffer *f, uint8_t *buf, size_t *nb_elems,
> +                            size_t offset, AVFifoCB write_cb, void *opaque)
> +{
> +    const FifoBuffer *fb = (FifoBuffer*)f;
> +    size_t       to_read = *nb_elems;
> +    size_t      offset_r = fb->offset_r;
> +    int              ret = 0;
> +
> +    if (offset > av_fifo_can_read(f) ||
> +        to_read > av_fifo_can_read(f) - offset) {

You are calling av_fifo_can_read() multiple times.

> +        *nb_elems = 0;
> +        return AVERROR(EINVAL);
> +    }
> +
> +    if (offset_r >= fb->nb_elems - offset)
> +        offset_r -= fb->nb_elems - offset;
> +    else
> +        offset_r += offset;
> +
> +    do {

Shouldn't this be a while loop?

> +        size_t    len = FFMIN(fb->nb_elems - offset_r, to_read);
> +        uint8_t *rptr = f->buffer + offset_r * fb->elem_size;
> +
> +        if (write_cb) {
> +            ret = write_cb(opaque, rptr, &len);
> +            if (ret < 0 || len == 0)
> +                break;
> +        } else {
> +            memcpy(buf, rptr, len * fb->elem_size);
> +            buf += len * fb->elem_size;
> +        }
> +        offset_r += len;
> +        if (offset_r >= fb->nb_elems)
> +            offset_r = 0;
> +        to_read -= len;
> +    } while (to_read > 0);
> +
> +    *nb_elems -= to_read;
> +
> +    return ret;
> +}
> +
> +int av_fifo_read(AVFifoBuffer *f, void *buf, size_t nb_elems)
> +{
> +    int ret = fifo_peek_common(f, buf, &nb_elems, 0, NULL, NULL);
> +    av_fifo_drain2(f, nb_elems);

In contrast to the current av_fifo_generic_read() the callback will
still see the non-drained FIFO if the callback is called multiple times.
I don't know whether this slight behaviour change can cause problems
when updating.

> +    return ret;
> +}
> +
> +int av_fifo_read_to_cb(AVFifoBuffer *f, AVFifoCB write_cb,
> +                       void *opaque, size_t *nb_elems)
> +{
> +    int ret = fifo_peek_common(f, NULL, nb_elems, 0, write_cb, opaque);
> +    av_fifo_drain2(f, *nb_elems);
> +    return ret;
> +}
> +
> +int av_fifo_peek(AVFifoBuffer *f, void *buf, size_t nb_elems, size_t offset)
> +{
> +    return fifo_peek_common(f, buf, &nb_elems, offset, NULL, NULL);
> +}
> +
> +int av_fifo_peek_to_cb(AVFifoBuffer *f, AVFifoCB write_cb, void *opaque,
> +                       size_t *nb_elems, size_t offset)
> +{
> +    return fifo_peek_common(f, NULL, nb_elems, offset, write_cb, opaque);
> +}
> +
>  int av_fifo_realloc2(AVFifoBuffer *f, unsigned int new_size)
>  {
>      FifoBuffer *fb = (FifoBuffer*)f;
> diff --git a/libavutil/fifo.h b/libavutil/fifo.h
> index ac1245ff39..c7be5e8f7d 100644
> --- a/libavutil/fifo.h
> +++ b/libavutil/fifo.h
> @@ -189,6 +189,66 @@ int av_fifo_write(AVFifoBuffer *f, const void *buf, size_t nb_elems);
>  int av_fifo_write_from_cb(AVFifoBuffer *f, AVFifoCB read_cb,
>                            void *opaque, size_t *nb_elems);
>  
> +/**
> + * Read data from a FIFO.
> + *
> + * @param f the FIFO buffer
> + * @param buf Buffer to store the data. nb_elems * av_fifo_elem_size(f) bytes
> + *            will be written into buf.
> + * @param nb_elems number of elements to read from FIFO
> + *
> + * @return a non-negative number on success, a negative error code on failure
> + */
> +int av_fifo_read(AVFifoBuffer *f, void *buf, size_t nb_elems);
> +
> +/**
> + * Feed data from a FIFO into a user-provided callback.
> + *
> + * @param f the FIFO buffer
> + * @param write_cb Callback the data will be supplied to. May be called
> + *                 multiple times.
> + * @param opaque opaque user data to be provided to write_cb
> + * @param nb_elems Should point to the maximum number of elements that can be
> + *                 read. Will be updated to contain the total number of elements
> + *                 actually sent to the callback.
> + *
> + * @return non-negative number on success, a negative error code on failure
> + */
> +int av_fifo_read_to_cb(AVFifoBuffer *f, AVFifoCB write_cb,
> +                       void *opaque, size_t *nb_elems);
> +
> +/**
> + * Read data from a FIFO without modifying FIFO state.
> + *
> + * @param f the FIFO buffer
> + * @param buf Buffer to store the data. nb_elems * av_fifo_elem_size(f) bytes
> + *            will be written into buf.
> + * @param nb_elems number of elements to read from FIFO
> + * @param offset number of initial elements to skip; offset + nb_elems must not
> + *               be larger than av_fifo_can_read(f).
> + *
> + * @return a non-negative number on success, a negative error code on failure
> + */
> +int av_fifo_peek(AVFifoBuffer *f, void *buf, size_t nb_elems, size_t offset);
> +
> +/**
> + * Feed data from a FIFO into a user-provided callback.
> + *
> + * @param f the FIFO buffer
> + * @param write_cb Callback the data will be supplied to. May be called
> + *                 multiple times.
> + * @param opaque opaque user data to be provided to write_cb
> + * @param nb_elems Should point to the maximum number of elements that can be
> + *                 read. Will be updated to contain the total number of elements
> + *                 actually sent to the callback.
> + * @param offset number of initial elements to skip; offset + *nb_elems must not
> + *               be larger than av_fifo_can_read(f).
> + *
> + * @return a non-negative number on success, a negative error code on failure
> + */
> +int av_fifo_peek_to_cb(AVFifoBuffer *f, AVFifoCB write_cb, void *opaque,

Ok, seems like we differ on our naming: I'd call this a read_cb, because
it reads from the fifo, but it of course also writes, so it seems fine
given that you are consistent.

> +                       size_t *nb_elems, size_t offset);
> +
>  /**
>   * Discard the specified amount of data from an AVFifoBuffer.
>   * @param size number of elements to discard, MUST NOT be larger than
>