[Ffmpeg-devel] [PATCH] updated LGPL AC-3 decoder

[Justin Ruggles]

Michael Niedermayer wrote:
> Hi
> 
> On Sat, Feb 24, 2007 at 03:11:17AM -0500, Justin Ruggles wrote:
> 
>>Hello,
>>
>>Here is a much-modified patch of the AC-3 decoder.  I basically rewrote
>>about half the code.  The 5.1 channel decoding still needs speeding-up,
>>but everything is working properly now.
>>
>>This patch is of all changes against current SVN, with ac3.c being a
>>copy of ac3enc.c.  As noted before, this is meant to be applied in a
>>logical sequence of commits, not all at once.

Thanks for the review.  I know it's a large patch, and it takes time to
do careful reviews.

> [...]
>>+}
>>+
>>+/**
>>+ * 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.

> [...]
> 
>>+/**
>>+ * 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.

> [...]
> 
>>+/**
>>+ * 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.

> [...]
> 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.

> [...]
> 
>>+        /* 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?  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?

I'll need to read up more on doxygen comments before making any of those
changes.  I kinda skimmed-over all the recent list traffic on that.

Thanks!
Justin