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

[James Almer]

On 12/4/2020 5:18 PM, Andreas Rheinhardt wrote:
> 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".

Right, was thinking about it and handling it like a linked list FIFO 
where you always add complete elements at the end than as a byte array 
with read/write index pointers.

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