[FFmpeg-devel] [PATCH 1/2][RFC] avcodec/packet: move AVPacketList definition and function helpers over from libavformat

[James Almer]

On 9/1/2020 2:00 PM, Andreas Rheinhardt wrote:
> James Almer:
>> diff --git a/libavcodec/packet_internal.h b/libavcodec/packet_internal.h
>> index cdb9a27f2f..387b63169a 100644
>> --- a/libavcodec/packet_internal.h
>> +++ b/libavcodec/packet_internal.h
>> @@ -23,6 +23,51 @@
>>  
>>  #include "packet.h"
>>  
>> +
>> +/** Create a new reference for the packet instead of
>> + * transferring the ownership of the existing one to the
>> + * list.
>> + */
>> +#define FF_PACKETLIST_FLAG_REF_PACKET (1 << 0)
>> +
>> +/**
>> + * Append an AVPacket to the list.
>> + *
>> + * @param head  List head element
>> + * @param tail  List tail element
>> + * @param pkt   The packet being appended. The data described in it will
>> + *              be made reference counted if it isn't already.
>> + * @param flags Any combination of FF_PACKETLIST_FLAG_* flags
>> + * @return 0 on success, negative AVERROR value on failure. On failure,
>> +           the list is unchanged
>> + */
>> +int avpriv_packet_list_put(AVPacketList **head, AVPacketList **tail,
>> +                           AVPacket *pkt, int flags);
>> +
>> +/**
>> + * Remove the oldest AVPacket in the list and return it.
>> + *
>> + * @note The pkt will be overwritten completely on success. The caller
>> + *       owns the packet and must unref it by itself.
>> + *
>> + * @param head List head element
>> + * @param tail List tail element
>> + * @param pkt  Pointer to an AVPacket struct
>> + * @return 0 on success. AVERROR(EAGAIN) if nothing is returned. Another
>> + *         negative AVERROR code on failure. On failure, the list is
>> + *         unchanged
> 
> I really dislike that you are removing the guarantee that one can always
> remove a packet from a list if the list is not empty. This means that
> one would now have to add unnecessary checks to several existing callers
> of this function.
> 
> Furthermore, your wording is weird: If nothing is returned, the return
> value is AVERROR(EAGAIN). This implies that if some other AVERROR code
> were to be returned, something is returned, i.e. the packet will be changed.

I can remove the "another negative AVERROR" part. Just added it for the
sake of being extensible, but since it's private it doesn't matter.

And sure, can also change the EAGAIN part so it's clear what it means.