[Ffmpeg-devel] [PATCH] updated LGPL AC-3 decoder
Michael Niedermayer
michaelni
Thu Mar 8 12:10:30 CET 2007
Hi
On Wed, Mar 07, 2007 at 11:56:47PM -0500, Justin Ruggles wrote:
[...]
> > [...]
> >>+}
> >>+
> >>+/**
> >>+ * Generates a Kaiser-Bessel Derived Window.
> >>+ */
> >>+static void ac3_window_init(float *window)
> >>+{
> >>+ int i, j;
> >>+ double sum = 0.0, bessel, tmp;
> >>+ double local_window[256];
> >>+ double alpha2 = (5.0 * M_PI / 256.0) * (5.0 * M_PI / 256.0);
> >>+
> >>+ for (i = 0; i < 256; i++) {
> >>+ tmp = i * (256 - i) * alpha2;
> >>+ bessel = 1.0;
> >>+ for (j = 100; j > 0; j--) /* default to 100 iterations */
> >>+ bessel = bessel * tmp / (j * j) + 1;
> >>+ sum += bessel;
> >>+ local_window[i] = sum;
> >>+ }
> >>+
> >>+ sum++;
> >>+ for (i = 0; i < 256; i++)
> >>+ window[i] = sqrt(local_window[i] / sum);
> >>+}
> >
> >
> > hmm this looks like a little redundant with respect to ac3_window[]
> > later could be inited by this too ...
>
> This was discussed once last year. I believe one of the conclusions was
> that it was not a good thing to do because the AC-3 encoder is bit-exact
> and the code above is floating-point. If you've changed your mind
> though, I'll be glad to move it to ac3.c and use it for the encoder as well.
ok, leave the encoder window as it is for now ...
>
> > [...]
> >
> >>+/**
> >>+ * Parses the 'sync info' and 'bit stream info' from the AC-3 bitstream.
> >>+ * GetBitContext within AC3DecodeContext must point to start of the
> >>+ * synchronized AC-3 bitstream.
> >>+ */
> >>+static int parse_header(AC3DecodeContext *ctx)
> >>+{
> >>+ AC3HeaderInfo hdr;
> >>+ GetBitContext *gb = &ctx->gb;
> >>+ int err, i;
> >>+
> >>+ err = ac3_parse_header(gb->buffer, &hdr);
> >>+ if(err)
> >>+ return err;
> >>+
> >>+ /* get decoding parameters from header info */
> >>+ ctx->crc1 = hdr.crc1;
> >>+ ctx->fscod = hdr.fscod;
> >>+ ctx->bit_alloc_params.fscod = hdr.fscod;
> >>+ ctx->acmod = hdr.acmod;
> >>+ ctx->cmixlev = hdr.cmixlev;
> >>+ ctx->surmixlev = hdr.surmixlev;
> >>+ ctx->lfeon = hdr.lfeon;
> >>+ ctx->halfratecod = hdr.halfratecod;
> >>+ ctx->bit_alloc_params.halfratecod = hdr.halfratecod;
> >>+ ctx->sample_rate = hdr.sample_rate;
> >>+ ctx->bit_rate = hdr.bit_rate / 1000;
> >>+ ctx->nchans = hdr.channels;
> >>+ ctx->nfchans = ctx->nchans - ctx->lfeon;
> >>+ ctx->lfe_channel = ctx->nfchans;
> >>+ ctx->cpl_channel = ctx->lfe_channel+1;
> >>+ ctx->frame_size = hdr.frame_size;
> >
> >
> > do you see an easy way to avoid this copying between 2 structs? i mean
> > would adding AC3HeaderInfo into AC3DecodeContext be an option or would
> > that add too many ctx->foobar.blah ? if later then ive no objections
> > to the current code above, the copying might very well be the smaller
> > evil
>
> I thought about this, too. I chose the copying for the same reason you
> mention above...especially the lfeon and nchans fields are accessed
> quite a lot throughout the code.
ok
>
> > [...]
> >
> >>+/**
> >>+ * Downmixes the output.
> >>+ * This function downmixes the output when the number of input
> >>+ * channels is not equal to the number of output channels requested.
> >>+ */
> >>+static void do_downmix(AC3DecodeContext *ctx)
> >
> >
> > could you split the whole downmix out into its own patch and file and make it
> > indenpandant of AC3? so it could be used by other similar codecs?
> >
>
> I can if you'd like...but the downmixing is based strictly on AC-3
> channel order, so I don't know how useful it would be. An alternative
> might be to define a standard FFmpeg channel order and use an array of
> pointers so they could quickly be shuffled around. But that would be
> heading down the road toward a more complex/complete channel reordering
> and mixing framework as discussed in other threads.
yes, please split it out, we have to start somewhere for a generic system
split is the first step, after that it can be adapted if needed for other
codecs ...
also iam in favor of a standard ffmpeg channel ordering independant of the
codec used
>
> > [...]
> > and actually if the channels where ordered so that cpl_channel is 0
> > then code like this could be simplified to
> >
> > for(ch=ctx->cplinu; ch<=ctx->nchans; ch++) {
> > ctx->fsnroffst[ch] = get_bits(gb, 4);
> > ctx->fgaincod [ch] = get_bits(gb, 3);
> > }
> >
> > and similar code seems common ...
>
> I did think about this situation quite a bit, as the original SoC patch
> does this. But the simplicity gain in the parsing leads to more
> complexity in other areas. The coupling channel is not part of the
> final output since it is merged into the other channels, so it makes
> more sense to me to have it at the end. I will go ahead and do the
> separation of the parsing like you suggested though.
ok
>
> > [...]
> >
> >>+ /* interleave output */
> >>+ for (k = 0; k < AC3_BLOCK_SIZE; k++) {
> >>+ for (j = 0; j < avctx->channels; j++) {
> >>+ *(out_samples++) = ctx->int_output[j][k];
> >>+ }
> >
> >
> > can that extra copy be avoided? i mean directly convert the floats into
> > the final int16 array?
>
> Hmm...I never thought about that. Would the dsp conversion function
> work if you pass the same memory address for both the destination and
> the source?
probably, though it might be worth to rather change the dsp function so
it can convert+interleave at the same time ...
> If not, maybe it would still be faster to convert in-place
> using a C version...
>
> >>-static const uint16_t ac3_bitratetab[19] = {
> >>+const uint16_t ff_ac3_bitratetab[19] = {
> >> 32, 40, 48, 56, 64, 80, 96, 112, 128,
> >> 160, 192, 224, 256, 320, 384, 448, 512, 576, 640
> >> };
> >>
> >>+/* acmod to number of channels */
> >>+const uint8_t ff_ac3_channels[8] = {
> >>+ 2, 1, 2, 3, 3, 4, 4, 5
> >>+};
> >>+
> >>+/** possible frame sizes */
> >>+const uint16_t ff_ac3_frame_sizes[64][3] = {
> >>+ { 64, 69, 96 },
> >>+ { 64, 70, 96 },
> >>+ { 80, 87, 120 },
> >>+ { 80, 88, 120 },
> >>+ { 96, 104, 144 },
> >>+ { 96, 105, 144 },
> >>+ { 112, 121, 168 },
> >>+ { 112, 122, 168 },
> >>+ { 128, 139, 192 },
> >>+ { 128, 140, 192 },
> >>+ { 160, 174, 240 },
> >>+ { 160, 175, 240 },
> >>+ { 192, 208, 288 },
> >>+ { 192, 209, 288 },
> >>+ { 224, 243, 336 },
> >>+ { 224, 244, 336 },
> >>+ { 256, 278, 384 },
> >>+ { 256, 279, 384 },
> >>+ { 320, 348, 480 },
> >>+ { 320, 349, 480 },
> >>+ { 384, 417, 576 },
> >>+ { 384, 418, 576 },
> >>+ { 448, 487, 672 },
> >>+ { 448, 488, 672 },
> >>+ { 512, 557, 768 },
> >>+ { 512, 558, 768 },
> >>+ { 640, 696, 960 },
> >>+ { 640, 697, 960 },
> >>+ { 768, 835, 1152 },
> >>+ { 768, 836, 1152 },
> >>+ { 896, 975, 1344 },
> >>+ { 896, 976, 1344 },
> >>+ { 1024, 1114, 1536 },
> >>+ { 1024, 1115, 1536 },
> >>+ { 1152, 1253, 1728 },
> >>+ { 1152, 1254, 1728 },
> >>+ { 1280, 1393, 1920 },
> >>+ { 1280, 1394, 1920 },
> >>+};
> >
> >
> > moving these tables from parser.c to ac3tab.h is ok and can be applied
> > anytime as a seperate change (reduces the amount of code i have to review
> > in the next iteration of the ac3 decoder review cycle)
> >
> >
> > [...]
> >
> >>-static const uint16_t fgaintab[8]= {
> >>+const uint16_t ff_fgaintab[8]= {
> >
> >
> > all static tab -> ff_tab changes are ok, they can be applied anytime
>
> This was all kind of integrated into adding ac3.c. I guess I could go
> ahead and move the tables and change static tab to ff_tab as a
> preliminary step. I would also need to add the ff_tab definitions to
> ac3.h. Does that sound ok?
everything which decreases the patch size sounds good
[...]
--
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
No snowflake in an avalanche ever feels responsible. -- Voltaire
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20070308/33a2583c/attachment.pgp>
More information about the ffmpeg-devel
mailing list