[FFmpeg-devel] [PATCH] avcodec/decode: port last_pkt_props to AVFifoBuffer

[Andreas Rheinhardt]

James Almer:
> On 12/4/2020 4:08 PM, Andreas Rheinhardt wrote:
>> James Almer:
>>> On 12/4/2020 2:08 PM, Andreas Rheinhardt wrote:
>>>> James Almer:
>>>>> Signed-off-by: James Almer <jamrial at gmail.com>
>>>>> ---
>>>>> The idea of making avpriv_packet_list_* public was not liked, and
>>>>> it was
>>>>> suggested to just use AVFifoBuffer instead.
>>>>>
>>>>> After this, the avpriv_packet_list_* functions can be made local to
>>>>> libavformat again.
>>>>>
>>>>>    libavcodec/decode.c   | 41
>>>>> +++++++++++++++++++++++++++++------------
>>>>>    libavcodec/internal.h |  4 ++--
>>>>>    libavcodec/utils.c    | 11 ++++++-----
>>>>>    3 files changed, 37 insertions(+), 19 deletions(-)
>>>>>
>>>>> diff --git a/libavcodec/decode.c b/libavcodec/decode.c
>>>>> index 5a1849f944..0550637029 100644
>>>>> --- a/libavcodec/decode.c
>>>>> +++ b/libavcodec/decode.c
>>>>> @@ -145,22 +145,40 @@ fail2:
>>>>>      #define IS_EMPTY(pkt) (!(pkt)->data)
>>>>>    +static int copy_packet_props(void *_src, void *_dst, int size)
>>>>> +{
>>>>> +    AVPacket *src = _src, *dst = _dst;
>>>>> +    int ret;
>>>>> +
>>>>> +    av_init_packet(dst);
>>>>> +    ret = av_packet_copy_props(dst, src);
>>>>> +    if (ret < 0)
>>>>> +        return ret;> +
>>>>> +    dst->size = src->size; // HACK: Needed for
>>>>> ff_decode_frame_props().
>>>>> +    dst->data = (void*)1;  // HACK: Needed for IS_EMPTY().
>>>>> +
>>>>> +    return sizeof(*dst);
>>>>> +}
>>>>
>>>> This is not how a write function for a fifo should look like: A fifo
>>>> may
>>>> need to store the beginning of a packet at the end of the fifo and the
>>>> end of a packet at the beginning of a fifo; you can check for this by
>>>> checking whether size is < sizeof(AVPacket).
>>>
>>> I'm already ensuring there's always sizeof(AVPacket) bytes left before
>>> calling av_fifo_generic_write().
>>>
>>
>> And? This just means one can write sizeof(AVPacket) bytes to the fifo,
>> not that there are sizeof(AVPacket) contiguous bytes available at the
>> current write pointer. The free space might be partially at the end and
>> partially at the beginning of the fifo buffer.
> 
> It wraps around? This is not obvious from reading the doxy.
> 
> In any case, by always incrementing the FIFO by sizeof(AVPacket) bytes
> it shouldn't be an issue since it will always be a multiple of it. But
> as i said, i'll just do it all outside of av_fifo_generic_write() anyway
> since i can't propagate errors.
> 

James, this is a fifo: It uses a circular buffer. Of course it wraps
around. And this is documented: "a very simple circular buffer FIFO
implementation".

And as I said:

>>
>>>> (The current implementation seems to actually only allocate
>>>> multiples of
>>>> a certain unit if all av_fifo_grow()/av_fifo_realloc2()/av_fifo_alloc()
>>>> calls use only multiples of this unit, but it doesn't seem to be
>>>> documented. Should probably be done as this simplifies using fifo
>>>> arrays.)
>

So, yes, it really seems to work nicely when using it to store arrays;
yet this is undocumented.

- Andreas