[FFmpeg-devel] [PATCH 1/5] avformat/utils: make AVPacketList helper functions shared

[James Almer]

On 3/27/2018 5:22 PM, Michael Niedermayer wrote:
> On Mon, Mar 26, 2018 at 03:02:35PM -0300, James Almer wrote:
>> Based on a patch by Luca Barbato.
>>
>> Signed-off-by: James Almer <jamrial at gmail.com>
>> ---
>>  libavformat/internal.h | 35 ++++++++++++++++++++++++++++++++++
>>  libavformat/utils.c    | 51 +++++++++++++++++++++++++++-----------------------
>>  2 files changed, 63 insertions(+), 23 deletions(-)
>>
>> diff --git a/libavformat/internal.h b/libavformat/internal.h
>> index a020b1b417..7e1b24ebe6 100644
>> --- a/libavformat/internal.h
>> +++ b/libavformat/internal.h
>> @@ -731,6 +731,41 @@ int ff_unlock_avformat(void);
>>   */
>>  void ff_format_set_url(AVFormatContext *s, char *url);
>>  
>> +/**
>> + * Append an AVPacket to the list.
>> + *
> 
>> + * @param head List head
>> + * @param tail List tail
> 
> about the terminology
> Shouldnt this be a single List element or 2 ListEntry elements ?

What do you suggest to write instead? I don't think the current
terminology is confusing. It's two AVPacketList arguments, one pointing
to the head/first item of the list and the other to the tail/last item.

> 
> 
>> + * @param pkt  The packet being appended
>> + * @param ref  Create a new reference for the packet instead of
>> +               transferring the ownership of the existing one to the
>> + *             list.
> 
>> + * @return < 0 on failure and 0 on success
> 
> if its an AVERROR code on failure this should be documented, ATM it just
> says some unspecifified negative value

Ok.

> 
> 
>> + */
>> +int ff_packet_list_put(AVPacketList **head, AVPacketList **tail,
>> +                       AVPacket *pkt, int ref);
> 
> ref should not be a int but a enum
> ff_packet_list_put(,,,1)
> ff_packet_list_put(,,,0)
> 
> is alot more confusing to a new developer than with english words
> the code should be self explanatory not requiring looking up the
> documentation

Adding an enum like this seems extreme for an internal API, but ok.

> 
> thx
> 
> [...]
> 
> 
> 
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>