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

[Marvin Scholz]

On 29 Aug 2022, at 18:03, James Almer wrote:

> 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);

IIUC this is about the total space in the FIFO, so maybe
av_fifo_capacity would be a better name?

>> +
>>  /**
>>   * @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.
>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request at ffmpeg.org with subject "unsubscribe".