[FFmpeg-devel] [libav-devel] [ffmpeg-devel] [PATCH 1/2] lavfi: make AVFilteFormats use int64_t lists to support channel layouts.

Mina Nagy Zaki mnzaki at gmail.com
Sun Jun 12 20:01:48 CEST 2011


On Sunday 12 June 2011 21:30:34 Michael Niedermayer wrote:
> On Sun, Jun 12, 2011 at 08:00:25PM +0200, Stefano Sabatini wrote:
> > On date Sunday 2011-06-12 14:42:17 +0300, Mina Nagy Zaki encoded:
> > > On Thursday 09 June 2011 17:36:16 Stefano Sabatini wrote:
> > > > On date Thursday 2011-06-09 13:25:25 +0300, Mina Nagy Zaki encoded:
> > > > > The list type was changed to int64_t to be able to hold
> > > > > channel layouts.
> > > > > 
> > > > > Usage of avfilter_make_format_list for
> > > > > PixelFromats/[AV]SampleFormats had to be changed to use int64_t[]
> > > > > instead of enums, as they are 32bit.
> > > > 
> > > > I discussed this with Mina and this looked like the best solution for
> > > > avoiding separate int/int64_t functions. If you have reasons to think
> > > > there are better solutions, please comment.
> > > 
> > > [...]
> > > 
> > > Ok, I just realized there's a *third* option. Keep all the 32 bit ints
> > > and drop support for any channel layouts that have more than 32
> > > channels. I was told that it was originally int64 to support formats
> > > that can do more than 32 channels. Right now though, the only value
> > > defined that's outside the int32 range is the value for
> > > AV_CH_LAYOUT_NATIVE which is unapplicable to channel layout
> > > negotiation anyway.
> > > 
> > > This way, no breaking ABI/API, no losing type-safety. It's just that
> > > filters will not support more than 32 channels. FFmpeg itself doesn't
> > > support it, and int64 is only there for *possibly* supporting it. So I
> > > don't see what's wrong. If we ever support it in FFmpeg, we can make
> > > the int64 change in lavfi then.
> > 
> > So resuming:
> > 
> > 1) make AVFilterFormats contain int64_t, and implement two separate:
> > 
> > AVFilterFormats *avfilter_make_format_list  (const int     *fmts)
> > AVFilterFormats *avfilter_make_format64_list(const int64_t *fmts)
> > 
> > Type-safe, but requires some code duplication (source code duplication
> > would be avoided by using a macro
> > DEFINE_AVFILTER_MAKE_FORMAT_LIST(type)).
> > 
> > 2) always use avfilter_make_format_list(const int64_t *fmts)
> > 
> > in this case we don't need two distinct avfilter_make_format*_list
> > functions, but we lose type-safety/debuggability and we need to update
> > current filters enum PixelFormat pix_fmts lists.
> > 
> > 3) always use avfilter_make_format_list(const int *fmts)
> > 
> > but we can't support non-int and AV_CH_LAYOUT_NATIVE channel layouts
> > if we want to add support to them.
> > 
> > ...
> 
> 4)
> static AVFilterFormats *make_format_list  (const void *fmts, int
> format_elem_size) AVFilterFormats *avfilter_make_pixfmt_list  (const enum
> PixelFormat *fmts){ return make_format_list(fmts, sizeof(...
> }
> ...

That is the stated solution number 1, but with different function names. It is 
also almost identical to what I first implemented, before trying the posted 
patch.

The thing with solution 1 is just the minor annoyance of function names, as 
can be seen from this (typical) piece of code:
AVFilterFormats *chlayouts = avfilter_make_chlayout_list(list);
avfilter_formats_add(chlayouts, AV_CH_LAYOUT_WHATHAVEYOU);
avfilter_formats_ref(chlayouts);

But it's just a petty thing. We could actually compromise between 1 & 3 and 
just use avfilter_make_formats_list() [the 32bit version] for everything and 
only use the 64bit version for any filters that will actually expect specific 
layouts with 32+ channels. Most filters will (probably) be just doing 
avfilter_all_chlayouts() anyway.

I will post a patch that implements solution 1, as that seems to be the agreed 
upon one. Soonish.

-- 
Mina


More information about the ffmpeg-devel mailing list