[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