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