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).
> > + twinvq_memset_float(out, st * gain,
> > mtab->fmode[ftype].bark_tab[idx]);
>
> long line
broken
> > + if (buf_size * 8 < avctx->bit_rate * mtab->size / avctx->sample_rate) {
> > + av_log(avctx, AV_LOG_ERROR,
> > + "Frame too small (%d bytes). Truncated file?\n", buf_size);
> > + return AVERROR(EINVAL);
>
> Shouldn't this be INVALIDDATA? AFAICT it's a bitstream error, not a
> user-supplied value.
TwinVQ expects fixed size frames, so it's user fault with providing us wrong
size frames (and that's an argument).
> > + for (i = 0; i < channels; i++) {
> > + bits->gain_bits[i] = get_bits(&gb, TWINVQ_GAIN_BITS);
> > + for (j = 0; j < sub; j++)
> > + bits->sub_gain_bits[i * sub + j] = get_bits(&gb,
> > +
> > TWINVQ_SUB_GAIN_BITS);
>
> Break after the '=' instead.
done
> > + switch ((avctx->channels << 16) + (isampf << 8) + ibps) {
> > + case (1 << 16) + ( 8 << 8) + 8:
> > + tctx->mtab = &ff_metasound_mode0808;
> > + break;
> > + case (1 << 16) + (16 << 8) + 16:
> > + tctx->mtab = &ff_metasound_mode1616;
> > + break;
> > + case (1 << 16) + (44 << 8) + 32:
> > + tctx->mtab = &ff_metasound_mode4432;
> > + break;
> > + case (2 << 16) + (44 << 8) + 48:
> > + tctx->mtab = &ff_metasound_mode4448s;
> > + break;
> > + default:
> > + av_log(avctx, AV_LOG_ERROR,
> > + "This version does not support %d kHz - %d kbit/s/ch
> > mode.\n",
> > + isampf, isampf);
> > + return -1;
>
> INVALIDDATA or PATCHWELCOME or similar?
ENOSYS
> > --- /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)?
Having it all in one file is quite inconvenient especially since I've not
added about half of the tables yet. And I don't want to expose so many tables
either.
> For twinvq there is also a huge file with tables.
And most of them are in one structure.
> > --- /dev/null
> > +++ b/libavcodec/metasound_data_lsp.c
> > @@ -0,0 +1,475 @@
> > +
> > +#include <stdint.h>
> > +
> > +static const float lsp8[] = {
> > + 0.27020001, 0.50959998, 0.6437, 0.76719999, 0.96390003, 1.0696, 1.2625,
> > 1.5789,
> > + 1.9285001, 2.2383001, 2.5129001, 2.8469999, 0.17399999, 0.36770001,
> > 0.60820001, 0.8387,
> > + 1.1084, 1.3721, 1.6362, 1.8733, 2.0639999, 2.3441999, 2.6087, 2.8548,
>
> indentation?
added
> The patch is probably OK overall.
>
> One or more FATE tests should be added.
would be nice indeed
_______________________________________________
libav-devel mailing list
[email protected]
https://lists.libav.org/mailman/listinfo/libav-devel
