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

[Michael Niedermayer]

On Tue, Mar 27, 2018 at 06:17:09PM -0300, James Almer wrote:
> 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.

hmm i guess the simplest would be
@param head List head element.

It feels a bit inelegant to have to keep track of 2 elements but changing
this is probably too much work


[...]
> > 
> > 
> >> + */
> >> +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.

yes, i was thinking a bit ahead.
For a internal API it is not that important but this API is quite usefull
i think theres a good chance it might become public in the future.
But even for a internal API named constants seem better IMHO

thx

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

Those who would give up essential Liberty, to purchase a little
temporary Safety, deserve neither Liberty nor Safety -- Benjamin Franklin
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 181 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20180327/fb36db2e/attachment.sig>