[FFmpeg-devel] [PATCH 2/3] lavu/fifo: clarify interaction of AV_FIFO_FLAG_AUTO_GROW with av_fifo_can_write()

[James Almer]

On 8/29/2022 12:00 PM, James Almer wrote:
> On 8/29/2022 11:07 AM, Anton Khirnov wrote:
>> ---
>>   libavutil/fifo.h | 8 +++++++-
>>   1 file changed, 7 insertions(+), 1 deletion(-)
>>
>> diff --git a/libavutil/fifo.h b/libavutil/fifo.h
>> index 6c6bd78842..89872d0972 100644
>> --- a/libavutil/fifo.h
>> +++ b/libavutil/fifo.h
>> @@ -97,7 +97,13 @@ void av_fifo_auto_grow_limit(AVFifo *f, size_t 
>> max_elems);
>>   size_t av_fifo_can_read(const AVFifo *f);
>>   /**
>> - * @return number of elements that can be written into the given FIFO.
>> + * @return Number of elements that can be written into the given FIFO 
>> without
>> + *         growing it.
>> + *
>> + *         In other words, this number of elements or less is 
>> guaranteed to fit
>> + *         into the FIFO. More data may be written when the
>> + *         AV_FIFO_FLAG_AUTO_GROW flag was specified at FIFO 
>> creation, but this
>> + *         may involve memory allocation, which can fail.
> 
> This patch is an API break, because before it i was told 
> av_fifo_can_write() would tell me the amount of elements i could write 
> into the FIFO, regardless of how it was created, but now it legitimates 
> the one scenario where it was not reliable. An scenario i stumbled upon 
> in my code by following the documentation, which is in at least one 
> release, the LTS one.
> 
> Instead of changing the documentation to fit the behavior, the behavior 
> should match the documentation. This means that if a call to 
> av_fifo_write() can succeed, then av_fifo_can_write() should reflect that.
> 
> That said, it would be great if making av_fifo_can_write() tell the real 
> amount of elements one can write into the FIFO was possible without 
> breaking anything, but the doxy for av_fifo_grow2() says "On success, 
> the FIFO will be large enough to hold exactly inc + av_fifo_can_read() + 
> av_fifo_can_write()", a line that was obviously aware of the fact 
> av_fifo_can_write() ignored the autogrow feature, and would no longer be 
> true if said function is fixed.
> 
> This could have been avoided if we added an av_fifo_size2() function 
> that returned nb_elems, so the line above may have been replaced by one 
> simply referring the user to it. But as is, we're breaking the API no 
> matter what we do.

Something like the following is the alternative. It's going to be a 
break in one way or another no matter what we do.

> diff --git a/libavutil/fifo.c b/libavutil/fifo.c
> index 51a5af6f39..3fc76b4247 100644
> --- a/libavutil/fifo.c
> +++ b/libavutil/fifo.c
> @@ -79,6 +79,11 @@ void av_fifo_auto_grow_limit(AVFifo *f, size_t max_elems)
>      f->auto_grow_limit = max_elems;
>  }
> 
> +size_t av_fifo_size2(const AVFifo *f)
> +{
> +    return f->nb_elems;
> +}
> +
>  size_t av_fifo_elem_size(const AVFifo *f)
>  {
>      return f->elem_size;
> @@ -93,7 +98,14 @@ size_t av_fifo_can_read(const AVFifo *f)
> 
>  size_t av_fifo_can_write(const AVFifo *f)
>  {
> -    return f->nb_elems - av_fifo_can_read(f);
> +    size_t nb_elems = f->nb_elems;
> +
> +    if (f->flags & AV_FIFO_FLAG_AUTO_GROW) {
> +        size_t autogrow = f->auto_grow_limit > nb_elems ?
> +                          f->auto_grow_limit - nb_elems : 0;
> +        nb_elems += autogrow;
> +    }
> +    return nb_elems - av_fifo_can_read(f);
>  }
> 
>  int av_fifo_grow2(AVFifo *f, size_t inc)
> diff --git a/libavutil/fifo.h b/libavutil/fifo.h
> index 4eed364afc..0f909aac55 100644
> --- a/libavutil/fifo.h
> +++ b/libavutil/fifo.h
> @@ -70,6 +70,11 @@ typedef int AVFifoCB(void *opaque, void *buf, size_t *nb_elems);
>  AVFifo *av_fifo_alloc2(size_t elems, size_t elem_size,
>                         unsigned int flags);
> 
> +/**
> + * @return Total number of elements the given FIFO can currently hold.
> + */
> +size_t av_fifo_size2(const AVFifo *f);
> +
>  /**
>   * @return Element size for FIFO operations. This element size is set at
>   *         FIFO allocation and remains constant during its lifetime
> @@ -89,20 +94,22 @@ size_t av_fifo_can_read(const AVFifo *f);
> 
>  /**
>   * @return number of elements that can be written into the given FIFO.
> + * @note   If the given FIFO was allocated with AV_FIFO_FLAG_AUTO_GROW, the
> + *         result of av_fifo_size2(f) - av_fifo_can_read(f) is the amount
> + *         of elements that can be written into it without the chance of
> + *         failure.
>   */
>  size_t av_fifo_can_write(const AVFifo *f);
> 
>  /**
>   * Enlarge an AVFifo.
>   *
> - * On success, the FIFO will be large enough to hold exactly
> - * inc + av_fifo_can_read() + av_fifo_can_write()
> - * elements. In case of failure, the old FIFO is kept unchanged.
> - *
>   * @param f AVFifo to resize
>   * @param inc number of elements to allocate for, in addition to the current
>   *            allocated size
> - * @return a non-negative number on success, a negative error code on failure
> + * @return a non-negative number on success, a negative error code on failure.
> + *         In case of failure, the old FIFO is kept unchanged.
> + * @see av_fifo_size2()
>   */
>  int av_fifo_grow2(AVFifo *f, size_t inc);
> 
> @@ -112,6 +119,9 @@ int av_fifo_grow2(AVFifo *f, size_t inc);
>   * In case nb_elems > av_fifo_can_write(f), nothing is written and an error
>   * is returned.
>   *
> + * Calling this function is guaranteed to succeed if
> + * nb_elems <= av_fifo_size2(f) - av_fifo_can_read(f).
> + *
>   * @param f the FIFO buffer
>   * @param buf Data to be written. nb_elems * av_fifo_elem_size(f) bytes will be
>   *            read from buf on success.