[libav-devel] [PATCH 2/2] Add OpenMG audio muxer

Diego Biurrun diego at biurrun.de
Wed Nov 2 10:53:14 CET 2011


On Sat, Oct 22, 2011 at 03:27:04PM +0200, Michael Karcher wrote:
> Am Samstag, den 22.10.2011, 14:19 +0200 schrieb Diego Biurrun:
> 
> First: Thanks for your review!

You are most welcome.

> > > --- a/libavformat/oma.c
> > > +++ b/libavformat/oma.c
> > > @@ -65,6 +68,10 @@ static const AVCodecTag codec_oma_tags[] = {
> > >  
> > > +static const uint16_t srate_tab[6] = {320,441,480,882,960,0};
> > > +
> > > +#if CONFIG_OMA_DEMUXER
> > 
> > The table is only used in the muxer, so it should be below the
> > appropriate #ifdef.
> 
> You seem to have missed something. It is used in the demuxer (in
> oma_read_header, directly after the "case OMA_CODECID_ATRAC3:" line, as
> well in the loop you quoted below in the muxer. I wonder whether the
> optimization of using 100Hz units to save space by storing as 16 bit
> values really makes sense, though, as it makes the code bigger. Scaling
> by 2 should be enough to fit all the rates into 16 bits, and "*2" should
> be smaller and faster than "*100".

OK, forget about this then.

> > I wonder if you could not (easily) put the muxer in a separate file and
> > skip the ifdeffery.
> This is indeed possible, except for the sample rate table mentioned
> above, which is shared between encoder and decoder. I expect making
> three files out of it (one shared, containing only the sample rate
> table, one for the muxer, one for the demuxer) is overkill, the other
> two options are duplicating the table or using the ifdefs as it is now.
> I don't think there is an option to have the compiler merge the tables
> if they are identically defined in two source files (which gcc does for
> strings)

We have three files in many similar cases, but it's not necessary here.
Please update your patch so that we can get it integrated.

Diego


More information about the libav-devel mailing list