On Tue, Aug 06, 2013 at 11:44:46AM +0200, Kostya Shishkov wrote:
> On Tue, Aug 06, 2013 at 11:16:28AM +0200, Diego Biurrun wrote:
> > On Tue, Aug 06, 2013 at 10:07:57AM +0200, Kostya Shishkov wrote:
> > > --- /dev/null
> > > +++ b/libavcodec/metasound.c
> > > @@ -0,0 +1,345 @@
> > > +
> > > + switch (isampf) {
> > > + case 8: some_mult = 2.0; break;
> > > + case 11: some_mult = 3.0; break;
> > > + case 16: some_mult = 3.0; break;
> > > + case 22: some_mult = ibps == 32 ? 2.0 : 4.0; break;
> > > + case 44: some_mult = 8.0; break;
> > > + default: some_mult = 4.0;
> > > + }
> >
> > Please break the lines.
>
> I like it this way because it looks like a table (except for the special case
> 22kHz 32kbps).
I don't think idiosyncratic formatting earns us anything, but do whatever
you prefer.
> > > --- /dev/null
> > > +++ b/libavcodec/metasound_data.c
> > > @@ -0,0 +1,113 @@
> > > +
> > > +static const uint16_t bark_tab_s44_128[] = {
> > > + 1, 2, 1, 2, 3, 4, 6, 10, 23, 76
> > > +};
> > > +
> > > +#include "metasound_cb0808.c"
> > > +#include "metasound_cb1616.c"
> > > +#include "metasound_cb4432.c"
> > > +#include "metasound_cb4448s.c"
> >
> > I still don't like this and would just keep the tables in one file.
>
> What about having three files? One for rate-dependent codebooks (now we have
> metasound_cb*.c, 4 of total 18 in separate files), one for sampling-rate
> dependent data (i.e. metasound_data_fcb.c metasound_data_lsp.c and
> metasound_data_shape.c) and one for small tables and mode description
> (metasound_data.c unchanged)?
Sounds better than the .c file #includes, go ahead.
> > > --- /dev/null
> > The patch is probably OK overall.
> >
> > One or more FATE tests should be added.
>
> would be nice indeed
So go ahead and add them :)
Diego
_______________________________________________
libav-devel mailing list
[email protected]
https://lists.libav.org/mailman/listinfo/libav-devel
