[FFmpeg-devel] [PATCH 1/3] lavu/fifo: add the header to its own doxy group

[FFmpeg-devel] [PATCH 1/3] lavu/fifo: add the header to its own doxy group

[Anton Khirnov]

Also, drop mentions of it being a circular buffer, as this is an
internal implementation detail that should be invisible to the caller.
---
 libavutil/fifo.h | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/libavutil/fifo.h b/libavutil/fifo.h
index 4eed364afc..6c6bd78842 100644
--- a/libavutil/fifo.h
+++ b/libavutil/fifo.h
@@ -18,7 +18,8 @@
 
 /**
  * @file
- * a very simple circular buffer FIFO implementation
+ * @ingroup lavu_fifo
+ * A generic FIFO API
  */
 
 #ifndef AVUTIL_FIFO_H
@@ -30,6 +31,14 @@
 #include "attributes.h"
 #include "version.h"
 
+/**
+ * @defgroup lavu_fifo AVFifo
+ * @ingroup lavu_data
+ *
+ * @{
+ * A generic FIFO API
+ */
+
 typedef struct AVFifo AVFifo;
 
 /**
@@ -423,4 +432,8 @@ static inline uint8_t *av_fifo_peek2(const AVFifoBuffer *f, int offs)
 #endif
 #endif
 
+/**
+ * @}
+ */
+
 #endif /* AVUTIL_FIFO_H */
-- 
2.35.1

_______________________________________________
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".

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

[Anton Khirnov]

---
 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.
  */
 size_t av_fifo_can_write(const AVFifo *f);
 
-- 
2.35.1

_______________________________________________
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".

Re: [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 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.

>    */
>   size_t av_fifo_can_write(const AVFifo *f);
>   
_______________________________________________
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".

Re: [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.

_______________________________________________
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".

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

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

[Anton Khirnov]

Quoting James Almer (2022-08-29 17:00:54)
> 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.

I disagree that this is a break.

The issue in my view is that 'can be written' is ambiguous here, so we
are interpreting it differently. Your interpretation is apparently
'maximum number of elements for which a write can possibly succeeed',
whereas my intended interpretation was 'maximum number of elements for
which a write is always guaranteed to succeed'.
One of these interpretations is correct, because it matches the actual
behaviour. So the right solution IMO is to clarify the documentation so
it is no longer ambiguous, but I do not consider this an API break.

More generally:
- a FIFO conceptually has a well-defined size at any given moment
- that size is can_read() + can_write()
- you can change the size by growing the FIFO
- AV_FIFO_FLAG_AUTO_GROW does not fundamentally change the above
  concepts, it merely calls av_fifo_grow2() when a write would
  otherwise fail

Now we can introduce a function for 'maximum number that can possibly
succeed' if you think it's useful - something like av_fifo_max_write().

-- 
Anton Khirnov
_______________________________________________
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".

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

[James Almer]

On 8/30/2022 3:35 AM, Anton Khirnov wrote:
> Quoting James Almer (2022-08-29 17:00:54)
>> 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.
> 
> I disagree that this is a break.
> 
> The issue in my view is that 'can be written' is ambiguous here, so we
> are interpreting it differently. Your interpretation is apparently
> 'maximum number of elements for which a write can possibly succeeed',
> whereas my intended interpretation was 'maximum number of elements for
> which a write is always guaranteed to succeed'.

IMO it's not really ambiguous. If you don't state that's the intention, 
which you're doing in this patch, then "can be written" has one literal 
meaning.

> One of these interpretations is correct, because it matches the actual
> behaviour. So the right solution IMO is to clarify the documentation so
> it is no longer ambiguous, but I do not consider this an API break.

av_fifo_write() says "In case nb_elems > av_fifo_can_write(f), nothing 
is written and an error is returned.", which is definitely not 
ambiguous, and you're changing it in patch 3/3 to include the case where 
having enabled autogrow could result in the function succeeding when 
nb_elems > av_fifo_can_write(f).
The behavior of the function remains intact, but a library user reading 
the documentation in ffmpeg 5.1 and the documentation in what will be 
5.2 after this patch could rightly assume the function was changed and 
will behave differently between versions (Which is not the case, but to 
find out you'll have to read the implementation, or the git history, or 
test code with both versions). So this is technically an API break.

> 
> More generally:
> - a FIFO conceptually has a well-defined size at any given moment
> - that size is can_read() + can_write()

But this could (should?) have been av_fifo_size2(). That way can_write() 
could effectively become a generic "can write", instead of begin stuck 
as "can write without the chance of failure".

> - you can change the size by growing the FIFO
> - AV_FIFO_FLAG_AUTO_GROW does not fundamentally change the above
>    concepts, it merely calls av_fifo_grow2() when a write would
>    otherwise fail
> 
> Now we can introduce a function for 'maximum number that can possibly
> succeed' if you think it's useful - something like av_fifo_max_write().

Maybe, but only if we find a usecase for it.

Hendrik had an opinion about what av_fifo_can_write() should report, but 
it was on IRC. But otherwise, knowing that anything we do will be 
breaking, if the current behavior was your intention all along as the 
author of the API, then i guess this set can go in unless someone else 
chimes.
_______________________________________________
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".

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

[Anton Khirnov]

Quoting James Almer (2022-08-30 14:56:45)
> > 
> > I disagree that this is a break.
> > 
> > The issue in my view is that 'can be written' is ambiguous here, so we
> > are interpreting it differently. Your interpretation is apparently
> > 'maximum number of elements for which a write can possibly succeeed',
> > whereas my intended interpretation was 'maximum number of elements for
> > which a write is always guaranteed to succeed'.
> 
> IMO it's not really ambiguous. If you don't state that's the intention, 
> which you're doing in this patch, then "can be written" has one literal 
> meaning.

I would disagree here. Consider an autogrowing fifo in an out-of-memory
situation. What "can be written" into it?

> > One of these interpretations is correct, because it matches the actual
> > behaviour. So the right solution IMO is to clarify the documentation so
> > it is no longer ambiguous, but I do not consider this an API break.
> 
> av_fifo_write() says "In case nb_elems > av_fifo_can_write(f), nothing 
> is written and an error is returned.", which is definitely not 
> ambiguous, and you're changing it in patch 3/3 to include the case where 
> having enabled autogrow could result in the function succeeding when 
> nb_elems > av_fifo_can_write(f).

That is quite clearly a bug in the documentation IMO. That line was not
present in the original patches I sent, but added at some time later in
the development (don't remember whether by myself or Andreas); then
whichever of us added it forgot to update it in the patch adding
AV_FIFO_FLAG_AUTO_GROW.

> The behavior of the function remains intact, but a library user reading 
> the documentation in ffmpeg 5.1 and the documentation in what will be 
> 5.2 after this patch could rightly assume the function was changed and 
> will behave differently between versions (Which is not the case, but to 
> find out you'll have to read the implementation, or the git history, or 
> test code with both versions). So this is technically an API break.

Technically yes, but the unfortunate fact of the matter is that our
API documentation simply is not, and never was, sufficiently complete
and precise to be the sole source of truth. Plenty of things are
missing, obsolete, inconsistent, and sometimes just wrong. I wish it
were otherwise, and I believe the situation is slowly improving, but we
just don't have the resources to make our docs anywhere close to
perfect any time soon. So unfortunately people have to test their code,
and testing in this case would immediately reveal how it actually works.

As a consequence we have to be pragmatic when choosing whether to change
code to match the docs or vice versa.

> 
> > 
> > More generally:
> > - a FIFO conceptually has a well-defined size at any given moment
> > - that size is can_read() + can_write()
> 
> But this could (should?) have been av_fifo_size2(). That way can_write() 
> could effectively become a generic "can write", instead of begin stuck 
> as "can write without the chance of failure".

Maybe, but it's a bit late for that. Actually I remember considering an
av_fifo_size2(), but then decided against it, probably because it could
confuse people into thinking it's like av_fifo_size(), which it most
definitely is not.

-- 
Anton Khirnov
_______________________________________________
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".

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

[Anton Khirnov]

---
 libavutil/fifo.h | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/libavutil/fifo.h b/libavutil/fifo.h
index 89872d0972..70f9376d97 100644
--- a/libavutil/fifo.h
+++ b/libavutil/fifo.h
@@ -124,9 +124,12 @@ int av_fifo_grow2(AVFifo *f, size_t inc);
 /**
  * Write data into a FIFO.
  *
- * In case nb_elems > av_fifo_can_write(f), nothing is written and an error
+ * In case nb_elems > av_fifo_can_write(f) and the AV_FIFO_FLAG_AUTO_GROW flag
+ * was not specified at FIFO creation, nothing is written and an error
  * is returned.
  *
+ * Calling function is guaranteed to succeed if nb_elems <= av_fifo_can_write(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.
-- 
2.35.1

_______________________________________________
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".

Re: [FFmpeg-devel] [PATCH 1/3] lavu/fifo: add the header to its own doxy group

[Michael Niedermayer]

[-- Attachment #1.1: Type: text/plain, Size: 502 bytes --]

On Mon, Aug 29, 2022 at 04:07:15PM +0200, Anton Khirnov wrote:
> Also, drop mentions of it being a circular buffer, as this is an
> internal implementation detail that should be invisible to the caller.
> ---
>  libavutil/fifo.h | 15 ++++++++++++++-
>  1 file changed, 14 insertions(+), 1 deletion(-)

agree

thx

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Nations do behave wisely once they have exhausted all other alternatives. 
-- Abba Eban

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

[-- Attachment #2: Type: text/plain, Size: 251 bytes --]

_______________________________________________
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".