[FFmpeg-devel] [PATCH] Channel Layout Negotiation

[Stefano Sabatini]

On date Friday 2011-06-17 20:41:05 +0300, Mina Nagy Zaki encoded:
> On Friday 17 June 2011 18:45:36 Stefano Sabatini wrote:
> > On date Friday 2011-06-17 18:07:39 +0300, MNZ encoded:
> > > Another iteration over this.
> > > 
> > > This time an avfilter_make_format64_list() was added and the original was
> > > retained. Also instead of avfilter_set_common_formats() there are now separate
> > > functions for pixel_formats, sample_formats and channel_layouts. The names are a
> > > bit too long though.
> > > 
> > > --
> > > Mina
> > 
> > > From 8de1ad310758db13fea3e2f5fc093b52de178c75 Mon Sep 17 00:00:00 2001
> > > From: Mina Nagy Zaki <mnzaki at gmail.com>
> > > Date: Tue, 7 Jun 2011 21:17:23 +0300
> > > Subject: [PATCH 1/2] lavfi: Use int64_t lists in AVFilteFormats.
> > > 
> > > The list type was changed to int64_t to be able to hold
> > > channel layouts.
> > > 
> > > avfilter_make_format_list() still takes a int32_t array and converts
> > > it to int64_t. A new function, avfilter_make_format64_list, that
> > > takes int64_t arrays has been added.
> > > ---
> > >  libavfilter/avfilter.h |    5 +++--
> > >  libavfilter/formats.c  |   36 ++++++++++++++++++++++++------------
> > >  2 files changed, 27 insertions(+), 14 deletions(-)
> > > 
> > > diff --git a/libavfilter/avfilter.h b/libavfilter/avfilter.h
> > > index 7628cd5..9651d86 100644
> > > --- a/libavfilter/avfilter.h
> > > +++ b/libavfilter/avfilter.h
> > > @@ -223,7 +223,7 @@ void avfilter_unref_buffer(AVFilterBufferRef *ref);
> > >   */
> > >  typedef struct AVFilterFormats {
> > >      unsigned format_count;      ///< number of formats
> > > -    int *formats;               ///< list of media formats
> > > +    int64_t *formats;           ///< list of media formats
> > >  
> > >      unsigned refcount;          ///< number of references to this list
> > >      struct AVFilterFormats ***refs; ///< references to this list
> > > @@ -238,6 +238,7 @@ typedef struct AVFilterFormats {
> > >   * @return the format list, with no existing references
> > >   */
> > >  AVFilterFormats *avfilter_make_format_list(const int *fmts);
> > > +AVFilterFormats *avfilter_make_format64_list(const int64_t *fmts);
> > >  
> > >  /**
> > >   * Add fmt to the list of media formats contained in *avff.
> > > @@ -247,7 +248,7 @@ AVFilterFormats *avfilter_make_format_list(const int *fmts);
> > >   * @return a non negative value in case of success, or a negative
> > >   * value corresponding to an AVERROR code in case of error
> > >   */
> > > -int avfilter_add_format(AVFilterFormats **avff, int fmt);
> > > +int avfilter_add_format(AVFilterFormats **avff, int64_t fmt);
> > >  
> > >  /**
> > >   * Return a list of all formats supported by FFmpeg for the given media type.
> > > diff --git a/libavfilter/formats.c b/libavfilter/formats.c
> > > index 58593fc..40254a0 100644
> > > --- a/libavfilter/formats.c
> > > +++ b/libavfilter/formats.c
> > > @@ -72,28 +72,40 @@ AVFilterFormats *avfilter_merge_formats(AVFilterFormats *a, AVFilterFormats *b)
> > >      return ret;
> > >  }
> > >  
> > 
> > > +#define MAKE_FORMAT_LIST()                                              \
> > > +    AVFilterFormats *formats;                                           \
> > > +    int count = 0;                                                      \
> > > +do {                                                                    \
> > > +    if (fmts)                                                           \
> > > +        for (count = 0; fmts[count] != -1; count++)                     \
> > > +            ;                                                           \
> > > +    formats = av_mallocz(sizeof(AVFilterFormats));                      \
> > > +    formats->format_count = count;                                      \
> > > +    if (count)                                                          \
> > > +        formats->formats  = av_malloc(sizeof(*formats->formats)*count); \
> > > +} while (0)
> > 
> > Missing NULL check => return NULL
> 
> Added NULL checks.
> 
> > Alternatively, you could create a function
> > static AVFilterFormats *make_format_list(int count);
> > which returns NULL in case of failure.
> > 
> 
> I think the macro is more fitting since a static function leads to unnecessary
> duplication of code. Also using a macro highlights the main difference between
> the two functions: the list copying code.
> 
> I have also removed the do..while(0) wrapper  since it is useless in this case,
> as pointed out by ubitux on IRC. 
> 
> Other nits taken care of.
[...]

No more comments from me, I'll apply both patches in a few days if I
see no comments from other devs, maybe Michael?
-- 
FFmpeg = Funny Foolish Mournful Prodigious Exploitable Gorilla