[PATCH] updated LGPL AC-3 decoder
Hello, Here is what I have so far in my attempts to merge the AC-3 decoder written by Kartikey Mahendra BHATT for the Google Summer of Code. There are some things which still need work, and I have some enhancements planned for after it's included. Right now, this gives generally working results, but has some issues. The 256-point MDCT is not working correctly. I'm still trying to get that fixed. Also, the downmixing seems to not always work like it should. The code could use some clean-up as well. The patch I've attached is against current SVN. The ac3.c and ac3_decoder.c files go into libavcodec. When it's committed, ac3.c will be created by doing an svn copy of ac3enc.c and then patching it to what I've included here. I think I got the build system stuff right, but I'm not completely sure. Here is a summary of the changes to the decoder: - used the new av_random for dithering - used the common bit allocation code from ac3enc.c (via ac3.c) - replaced the exponent decoding, which was from liba52 (GPL), with an implementation I wrote for my own AC-3 parser, which is LGPL. - reused some of the macros from ac3.h and got rid of ac3_decoder.h I really want to fix the short block MDCT before merging this, but I thought I would go ahead and post what I have so it could be reviewed. Thanks, Justin
I just realized I forgot to ask something I've been meaning to bring up. Does the old, unused ac3dec.c need to be kept around for archival reasons or can it be reused/patched-over for the LGPL decoder instead of using a name like ac3_decoder.c? -Justin
On Sat, Feb 10, 2007 at 01:17:52AM -0500, Justin Ruggles wrote:
I just realized I forgot to ask something I've been meaning to bring up. Does the old, unused ac3dec.c need to be kept around for archival reasons or can it be reused/patched-over for the LGPL decoder instead of using a name like ac3_decoder.c?
For consistency reasons I would prefer to reuse the old name. Maybe we can rename the file to libac3dec.c first or something. Another thing that needs to be decided is what to do with the internal liba52 copy. I vote for removing it right away, the external one works and we will have a native decoder in the near future. Diego
Diego Biurrun wrote:
On Sat, Feb 10, 2007 at 01:17:52AM -0500, Justin Ruggles wrote:
I just realized I forgot to ask something I've been meaning to bring up. Does the old, unused ac3dec.c need to be kept around for archival reasons or can it be reused/patched-over for the LGPL decoder instead of using a name like ac3_decoder.c?
For consistency reasons I would prefer to reuse the old name. Maybe we can rename the file to libac3dec.c first or something.
Another thing that needs to be decided is what to do with the internal liba52 copy. I vote for removing it right away, the external one works and we will have a native decoder in the near future.
Diego
Agree. MvH Benjamin Larsson
On Sat, 10 Feb 2007 15:44:14 +0100 Diego Biurrun <diego at biurrun.de> wrote:
On Sat, Feb 10, 2007 at 01:17:52AM -0500, Justin Ruggles wrote:
I just realized I forgot to ask something I've been meaning to bring up. Does the old, unused ac3dec.c need to be kept around for archival reasons or can it be reused/patched-over for the LGPL decoder instead of using a name like ac3_decoder.c?
For consistency reasons I would prefer to reuse the old name.
Agree.
Maybe we can rename the file to libac3dec.c first or something.
AFAICT ac3dec.c is currently unused (there is no Makefile rule to build it). So I guess it can safely be removed. I vote for removing it right now.
Another thing that needs to be decided is what to do with the internal liba52 copy. I vote for removing it right away, the external one works and we will have a native decoder in the near future.
Agree. And IMO, we should even aim at removing external liba52 support (when native ac3 support will be fully functionnal and well tested). Aurel
Hi On Sat, Feb 10, 2007 at 04:27:51PM +0100, Aurelien Jacobs wrote:
On Sat, 10 Feb 2007 15:44:14 +0100 [...]
Another thing that needs to be decided is what to do with the internal liba52 copy. I vote for removing it right away, the external one works and we will have a native decoder in the near future.
Agree. And IMO, we should even aim at removing external liba52 support (when native ac3 support will be fully functionnal and well tested).
well tested is not needed and it will become quicker fully functional if users cant easily switch to another implementation (yes iam evil) allthough if the new code doesnt work at all (256point imdct ...) then thats of course too early also speed and complexity is important it would be great if someone could now and periodically in the future when its in svn post benchmarks, (striped) object and source (no mmx/sse) sizes of the new AC3 and liba52 [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB The worst form of inequality is to try to make unequal things equal. -- Aristotle
On Sat, Feb 10, 2007 at 04:47:02PM +0100, Michael Niedermayer wrote:
On Sat, Feb 10, 2007 at 04:27:51PM +0100, Aurelien Jacobs wrote:
On Sat, 10 Feb 2007 15:44:14 +0100 [...]
Another thing that needs to be decided is what to do with the internal liba52 copy. I vote for removing it right away, the external one works and we will have a native decoder in the near future.
Agree. And IMO, we should even aim at removing external liba52 support (when native ac3 support will be fully functionnal and well tested).
well tested is not needed and it will become quicker fully functional if users cant easily switch to another implementation (yes iam evil)
allthough if the new code doesnt work at all (256point imdct ...) then thats of course too early
I agree, but for the moment we can keep the external liba52 support. So do I have your blessing for removing the internal liba52? Diego
Hi On Sat, Feb 10, 2007 at 10:07:40PM +0100, Diego Biurrun wrote:
On Sat, Feb 10, 2007 at 04:47:02PM +0100, Michael Niedermayer wrote:
On Sat, Feb 10, 2007 at 04:27:51PM +0100, Aurelien Jacobs wrote:
On Sat, 10 Feb 2007 15:44:14 +0100 [...]
Another thing that needs to be decided is what to do with the internal liba52 copy. I vote for removing it right away, the external one works and we will have a native decoder in the near future.
Agree. And IMO, we should even aim at removing external liba52 support (when native ac3 support will be fully functionnal and well tested).
well tested is not needed and it will become quicker fully functional if users cant easily switch to another implementation (yes iam evil)
allthough if the new code doesnt work at all (256point imdct ...) then thats of course too early
I agree, but for the moment we can keep the external liba52 support.
So do I have your blessing for removing the internal liba52?
all files except thouse written by me (as the mmx/sse code in them might be usefull for the new decoder ...) [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB I count him braver who overcomes his desires than him who conquers his enemies for the hardest victory is over self. -- Aristotle
On Sun, Feb 11, 2007 at 01:13:46AM +0100, Michael Niedermayer wrote:
On Sat, Feb 10, 2007 at 10:07:40PM +0100, Diego Biurrun wrote:
So do I have your blessing for removing the internal liba52?
all files except thouse written by me (as the mmx/sse code in them might be usefull for the new decoder ...)
That would be the file resample_mmx.c, it's the only one that has your name in the copyright notice. However, this file is also present in MPlayer's copy of liba52. MPlayer's copy contains further interesting changes like AMD64 support, so I think that's a better reference. OK to remove resample_mmx.c as well then? Diego
Hi On Sun, Feb 11, 2007 at 12:15:25PM +0100, Diego Biurrun wrote:
On Sun, Feb 11, 2007 at 01:13:46AM +0100, Michael Niedermayer wrote:
On Sat, Feb 10, 2007 at 10:07:40PM +0100, Diego Biurrun wrote:
So do I have your blessing for removing the internal liba52?
all files except thouse written by me (as the mmx/sse code in them might be usefull for the new decoder ...)
That would be the file resample_mmx.c, it's the only one that has your name in the copyright notice. However, this file is also present in MPlayer's copy of liba52. MPlayer's copy contains further interesting changes like AMD64 support, so I think that's a better reference.
OK to remove resample_mmx.c as well then?
ok [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Republics decline into democracies and democracies degenerate into despotisms. -- Aristotle
On Sun, Feb 11, 2007 at 01:17:09PM +0100, Michael Niedermayer wrote:
Hi
On Sun, Feb 11, 2007 at 12:15:25PM +0100, Diego Biurrun wrote:
On Sun, Feb 11, 2007 at 01:13:46AM +0100, Michael Niedermayer wrote:
On Sat, Feb 10, 2007 at 10:07:40PM +0100, Diego Biurrun wrote:
So do I have your blessing for removing the internal liba52?
all files except thouse written by me (as the mmx/sse code in them might be usefull for the new decoder ...)
That would be the file resample_mmx.c, it's the only one that has your name in the copyright notice. However, this file is also present in MPlayer's copy of liba52. MPlayer's copy contains further interesting changes like AMD64 support, so I think that's a better reference.
OK to remove resample_mmx.c as well then?
ok
OK, I'm going to remove it soon then. IIUC this requires a major version bump. The problem is that avcodec_decode_audio is marked deprecated and will be removed by the preprocessor once the the libavcodec major version reaches 52. Can I postpone this until version 53 or do we have to address the issue right now? Diego
Diego Biurrun <diego at biurrun.de> writes:
On Sun, Feb 11, 2007 at 01:17:09PM +0100, Michael Niedermayer wrote:
Hi
On Sun, Feb 11, 2007 at 12:15:25PM +0100, Diego Biurrun wrote:
On Sun, Feb 11, 2007 at 01:13:46AM +0100, Michael Niedermayer wrote:
On Sat, Feb 10, 2007 at 10:07:40PM +0100, Diego Biurrun wrote:
So do I have your blessing for removing the internal liba52?
all files except thouse written by me (as the mmx/sse code in them might be usefull for the new decoder ...)
That would be the file resample_mmx.c, it's the only one that has your name in the copyright notice. However, this file is also present in MPlayer's copy of liba52. MPlayer's copy contains further interesting changes like AMD64 support, so I think that's a better reference.
OK to remove resample_mmx.c as well then?
ok
OK, I'm going to remove it soon then. IIUC this requires a major version bump. The problem is that avcodec_decode_audio is marked deprecated and will be removed by the preprocessor once the the libavcodec major version reaches 52.
Why would this need a version bump? There is no API or ABI change. -- M?ns Rullg?rd mans at mansr.com
On Sun, Feb 11, 2007 at 01:52:46PM +0000, M?ns Rullg?rd wrote:
Diego Biurrun <diego at biurrun.de> writes:
On Sun, Feb 11, 2007 at 01:17:09PM +0100, Michael Niedermayer wrote:
On Sun, Feb 11, 2007 at 12:15:25PM +0100, Diego Biurrun wrote:
On Sun, Feb 11, 2007 at 01:13:46AM +0100, Michael Niedermayer wrote:
On Sat, Feb 10, 2007 at 10:07:40PM +0100, Diego Biurrun wrote:
So do I have your blessing for removing the internal liba52?
all files except thouse written by me (as the mmx/sse code in them might be usefull for the new decoder ...)
That would be the file resample_mmx.c, it's the only one that has your name in the copyright notice. However, this file is also present in MPlayer's copy of liba52. MPlayer's copy contains further interesting changes like AMD64 support, so I think that's a better reference.
OK to remove resample_mmx.c as well then?
ok
OK, I'm going to remove it soon then. IIUC this requires a major version bump. The problem is that avcodec_decode_audio is marked deprecated and will be removed by the preprocessor once the the libavcodec major version reaches 52.
Why would this need a version bump? There is no API or ABI change.
The last time we talked about this Michael said a version bump would be necessary since this removes a feature from libavcodec.. Diego
Diego Biurrun <diego at biurrun.de> writes:
On Sun, Feb 11, 2007 at 01:52:46PM +0000, M?ns Rullg?rd wrote:
Diego Biurrun <diego at biurrun.de> writes:
On Sun, Feb 11, 2007 at 01:17:09PM +0100, Michael Niedermayer wrote:
On Sun, Feb 11, 2007 at 12:15:25PM +0100, Diego Biurrun wrote:
On Sun, Feb 11, 2007 at 01:13:46AM +0100, Michael Niedermayer wrote:
On Sat, Feb 10, 2007 at 10:07:40PM +0100, Diego Biurrun wrote: > > So do I have your blessing for removing the internal liba52?
all files except thouse written by me (as the mmx/sse code in them might be usefull for the new decoder ...)
That would be the file resample_mmx.c, it's the only one that has your name in the copyright notice. However, this file is also present in MPlayer's copy of liba52. MPlayer's copy contains further interesting changes like AMD64 support, so I think that's a better reference.
OK to remove resample_mmx.c as well then?
ok
OK, I'm going to remove it soon then. IIUC this requires a major version bump. The problem is that avcodec_decode_audio is marked deprecated and will be removed by the preprocessor once the the libavcodec major version reaches 52.
Why would this need a version bump? There is no API or ABI change.
The last time we talked about this Michael said a version bump would be necessary since this removes a feature from libavcodec..
As I understood it, the intention was to remove the internal liba52 and continue linking to it externally. Especially considering that it always required an explicit --enable-a52, I don't see the need for a version bump. -- M?ns Rullg?rd mans at mansr.com
On Sun, Feb 11, 2007 at 03:38:41PM +0000, M?ns Rullg?rd wrote:
Diego Biurrun <diego at biurrun.de> writes:
On Sun, Feb 11, 2007 at 01:52:46PM +0000, M?ns Rullg?rd wrote:
Diego Biurrun <diego at biurrun.de> writes:
On Sun, Feb 11, 2007 at 01:17:09PM +0100, Michael Niedermayer wrote:
On Sun, Feb 11, 2007 at 12:15:25PM +0100, Diego Biurrun wrote:
On Sun, Feb 11, 2007 at 01:13:46AM +0100, Michael Niedermayer wrote: > > On Sat, Feb 10, 2007 at 10:07:40PM +0100, Diego Biurrun wrote: > > > > So do I have your blessing for removing the internal liba52? > > all files except thouse written by me (as the mmx/sse code in them might > be usefull for the new decoder ...)
That would be the file resample_mmx.c, it's the only one that has your name in the copyright notice. However, this file is also present in MPlayer's copy of liba52. MPlayer's copy contains further interesting changes like AMD64 support, so I think that's a better reference.
OK to remove resample_mmx.c as well then?
ok
OK, I'm going to remove it soon then. IIUC this requires a major version bump. The problem is that avcodec_decode_audio is marked deprecated and will be removed by the preprocessor once the the libavcodec major version reaches 52.
Why would this need a version bump? There is no API or ABI change.
The last time we talked about this Michael said a version bump would be necessary since this removes a feature from libavcodec..
As I understood it, the intention was to remove the internal liba52 and continue linking to it externally.
Yes, at least for the moment. libavcodec/a52dec.c would remain with the LIBA52BIN #ifdefs removed.
Especially considering that it always required an explicit --enable-a52, I don't see the need for a version bump.
Well, I'll be happy to skip this step since it makes my life easier. Michael? Diego
Hi On Sun, Feb 11, 2007 at 03:19:54PM +0100, Diego Biurrun wrote:
On Sun, Feb 11, 2007 at 01:52:46PM +0000, M?ns Rullg?rd wrote:
Diego Biurrun <diego at biurrun.de> writes:
On Sun, Feb 11, 2007 at 01:17:09PM +0100, Michael Niedermayer wrote:
On Sun, Feb 11, 2007 at 12:15:25PM +0100, Diego Biurrun wrote:
On Sun, Feb 11, 2007 at 01:13:46AM +0100, Michael Niedermayer wrote:
On Sat, Feb 10, 2007 at 10:07:40PM +0100, Diego Biurrun wrote: > > So do I have your blessing for removing the internal liba52?
all files except thouse written by me (as the mmx/sse code in them might be usefull for the new decoder ...)
That would be the file resample_mmx.c, it's the only one that has your name in the copyright notice. However, this file is also present in MPlayer's copy of liba52. MPlayer's copy contains further interesting changes like AMD64 support, so I think that's a better reference.
OK to remove resample_mmx.c as well then?
ok
OK, I'm going to remove it soon then. IIUC this requires a major version bump. The problem is that avcodec_decode_audio is marked deprecated and will be removed by the preprocessor once the the libavcodec major version reaches 52.
Why would this need a version bump? There is no API or ABI change.
The last time we talked about this Michael said a version bump would be necessary since this removes a feature from libavcodec..
me? i dont remember ... as far as my memory works, i was never in favor of messing with the major or minor versions when codecs where added or removed as there is no ABI/API change, it was IIRC roberto who wanted the minor version to be bumped when codecs where added ... also as mans said no codec is removed its just internal liba52 external is still there which from the user apps POV shouldnt be different ... [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Why not whip the teacher when the pupil misbehaves? -- Diogenes of Sinope
On Sun, Feb 11, 2007 at 05:13:24PM +0100, Michael Niedermayer wrote:
On Sun, Feb 11, 2007 at 03:19:54PM +0100, Diego Biurrun wrote:
On Sun, Feb 11, 2007 at 01:52:46PM +0000, M?ns Rullg?rd wrote:
Diego Biurrun <diego at biurrun.de> writes:
On Sun, Feb 11, 2007 at 01:17:09PM +0100, Michael Niedermayer wrote:
On Sun, Feb 11, 2007 at 12:15:25PM +0100, Diego Biurrun wrote:
On Sun, Feb 11, 2007 at 01:13:46AM +0100, Michael Niedermayer wrote: > > On Sat, Feb 10, 2007 at 10:07:40PM +0100, Diego Biurrun wrote: > > > > So do I have your blessing for removing the internal liba52? > > all files except thouse written by me (as the mmx/sse code in them might > be usefull for the new decoder ...)
That would be the file resample_mmx.c, it's the only one that has your name in the copyright notice. However, this file is also present in MPlayer's copy of liba52. MPlayer's copy contains further interesting changes like AMD64 support, so I think that's a better reference.
OK to remove resample_mmx.c as well then?
ok
OK, I'm going to remove it soon then. IIUC this requires a major version bump. The problem is that avcodec_decode_audio is marked deprecated and will be removed by the preprocessor once the the libavcodec major version reaches 52.
Why would this need a version bump? There is no API or ABI change.
The last time we talked about this Michael said a version bump would be necessary since this removes a feature from libavcodec..
me? i dont remember ...
I might well remember wrong..
as far as my memory works, i was never in favor of messing with the major or minor versions when codecs where added or removed as there is no ABI/API change, it was IIRC roberto who wanted the minor version to be bumped when codecs where added ...
also as mans said no codec is removed its just internal liba52 external is still there which from the user apps POV shouldnt be different ...
OK, fine with me, I'll commit in a moment without version bump. Diego
On Sun, Feb 11, 2007 at 05:16:29PM +0100, Diego Biurrun wrote:
On Sun, Feb 11, 2007 at 05:13:24PM +0100, Michael Niedermayer wrote:
also as mans said no codec is removed its just internal liba52 external is still there which from the user apps POV shouldnt be different ...
OK, fine with me, I'll commit in a moment without version bump.
Done. Diego
On Sat, Feb 10, 2007 at 01:02:20AM -0500, Justin Ruggles wrote:
The patch I've attached is against current SVN. The ac3.c and ac3_decoder.c files go into libavcodec. When it's committed, ac3.c will be created by doing an svn copy of ac3enc.c and then patching it to what I've included here. I think I got the build system stuff right, but I'm not completely sure.
Build system stuff is mostly OK, I think this patch will take multiple iterations anyway, so it's not a pressing issue. I'll try to come up with a clean solution for handling alternative decoders through configure. Please fix the license header and remove all trailing whitespace from ac3_decoder.c. Diego
Hi On Sat, Feb 10, 2007 at 01:02:20AM -0500, Justin Ruggles wrote:
Hello,
Here is what I have so far in my attempts to merge the AC-3 decoder written by Kartikey Mahendra BHATT for the Google Summer of Code. There are some things which still need work, and I have some enhancements planned for after it's included. Right now, this gives generally working results, but has some issues.
The 256-point MDCT is not working correctly. I'm still trying to get that fixed. Also, the downmixing seems to not always work like it should. The code could use some clean-up as well.
The patch I've attached is against current SVN. The ac3.c and ac3_decoder.c files go into libavcodec. When it's committed, ac3.c will be created by doing an svn copy of ac3enc.c and then patching it to what I've included here. I think I got the build system stuff right, but I'm not completely sure.
you should have diffed ac3.c against ac3enc.c then, also keep in mind that code duplication (i dont know if theres any after the copy and patch) is unacceptable
Here is a summary of the changes to the decoder: - used the new av_random for dithering - used the common bit allocation code from ac3enc.c (via ac3.c) - replaced the exponent decoding, which was from liba52 (GPL), with an implementation I wrote for my own AC-3 parser, which is LGPL. - reused some of the macros from ac3.h and got rid of ac3_decoder.h
I really want to fix the short block MDCT before merging this, but I thought I would go ahead and post what I have so it could be reviewed.
[...]
#include "avcodec.h" #include "ac3.h" #include "ac3tab.h"
static inline int calc_lowcomp1(int a, int b0, int b1) { if ((b0 + 256) == b1) { a = 384 ; } else if (b0 > b1) { a = a - 64; if (a < 0) a=0; } return a; }
calc_lowcomp1(int a, int b0, int b1, int C){ if ((b0 + 256) == b1) { a= C; } else if (b0 > b1) { a= FFMAX(a - 64, 0); } return a;
static inline int calc_lowcomp(int a, int b0, int b1, int bin) { if (bin < 7) { if ((b0 + 256) == b1) { a = 384 ; } else if (b0 > b1) { a = a - 64; if (a < 0) a=0; } } else if (bin < 20) { if ((b0 + 256) == b1) { a = 320 ; } else if (b0 > b1) { a= a - 64; if (a < 0) a=0; } } else { a = a - 128; if (a < 0) a=0; } return a; }
if (bin < 7) { return calc_lowcomp1(a, b0, b1, 384); } else if (bin < 20) { return calc_lowcomp1(a, b0, b1, 320); } else { return FFMAX(a - 128, 0); }
/* AC3 bit allocation. The algorithm is the one described in the AC3 spec. */ void ac3_parametric_bit_allocation(AC3BitAllocParameters *s, uint8_t *bap,
not doxygen compatible comment
int8_t *exp, int start, int end, int snroffset, int fgain, int is_lfe, int deltbae,int deltnseg, uint8_t *deltoffst, uint8_t *deltlen, uint8_t *deltba) { int bin,i,j,k,end1,v,v1,bndstrt,bndend,lowcomp,begin; int fastleak,slowleak,address,tmp; int16_t psd[256]; /* scaled exponents */ int16_t bndpsd[50]; /* interpolated exponents */ int16_t excite[50]; /* excitation */ int16_t mask[50]; /* masking value */
/* exponent mapping to PSD */ for(bin=start;bin<end;bin++) { psd[bin]=(3072 - (exp[bin] << 7)); }
/* PSD integration */ j=start; k=masktab[start]; do { v=psd[j]; j++; end1=bndtab[k+1]; if (end1 > end) end1=end;
end1= FFMIN(end1, end);
for(i=j;i<end1;i++) { int c,adr; /* logadd */ v1=psd[j]; c=v-v1; if (c >= 0) { adr=c >> 1; if (adr > 255) adr=255; v=v + latab[adr]; } else { adr=(-c) >> 1; if (adr > 255) adr=255; v=v1 + latab[adr]; }
adr= FFMIN(FFABS(v-v1)>>1, 255) v= FFMAX(v,v1) + latab[adr];
j++; } bndpsd[k]=v; k++; } while (end > bndtab[k]);
/* excitation function */ bndstrt = masktab[start]; bndend = masktab[end-1] + 1;
if (bndstrt == 0) { lowcomp = 0; lowcomp = calc_lowcomp1(lowcomp, bndpsd[0], bndpsd[1]) ; excite[0] = bndpsd[0] - fgain - lowcomp ; lowcomp = calc_lowcomp1(lowcomp, bndpsd[1], bndpsd[2]) ; excite[1] = bndpsd[1] - fgain - lowcomp ; begin = 7 ; for (bin = 2; bin < 7; bin++) { if (!(is_lfe && bin == 6)) lowcomp = calc_lowcomp1(lowcomp, bndpsd[bin], bndpsd[bin+1]) ; fastleak = bndpsd[bin] - fgain ; slowleak = bndpsd[bin] - s->sgain ; excite[bin] = fastleak - lowcomp ; if (!(is_lfe && bin == 6)) { if (bndpsd[bin] <= bndpsd[bin+1]) { begin = bin + 1 ; break ; } } }
end1=bndend; if (end1 > 22) end1=22;
for (bin = begin; bin < end1; bin++) { if (!(is_lfe && bin == 6)) lowcomp = calc_lowcomp(lowcomp, bndpsd[bin], bndpsd[bin+1], bin) ;
fastleak -= s->fdecay ; v = bndpsd[bin] - fgain; if (fastleak < v) fastleak = v;
FFMAX
slowleak -= s->sdecay ; v = bndpsd[bin] - s->sgain; if (slowleak < v) slowleak = v;
FFMAX
v=fastleak - lowcomp; if (slowleak > v) v=slowleak;
v= FFMAX(fastleak - lowcomp, slowleak) [...]
for (k = i; k < end1; k++) { address = (psd[i] - v) >> 5 ; if (address < 0) address=0; else if (address > 63) address=63;
clip() [...] also improvements to existing files (ac3enc.c) must be commited seperately as they are seperate from the actual ac3 decoder [...]
#define ALT_BITSTREAM_READER
why?
#include "avcodec.h" #include "ac3.h" #include "bitstream.h" #include "dsputil.h" #include "random.h"
static const int nfchans_tbl[8] = { 2, 1, 2, 3, 3, 4, 4, 5 };
uint8_t [...]
/* Adjustments in dB gain */ #define LEVEL_MINUS_3DB 0.7071067811865476
add "// sqrt(2)/2"
#define LEVEL_MINUS_4POINT5DB 0.5946035575013605 #define LEVEL_MINUS_6DB 0.5000000000000000 #define LEVEL_PLUS_3DB 1.4142135623730951
add "// sqrt(2)" [...]
#define AC3_INPUT_DUALMONO 0x00 #define AC3_INPUT_MONO 0x01 #define AC3_INPUT_STEREO 0x02 #define AC3_INPUT_3F 0x03 #define AC3_INPUT_2F_1R 0x04 #define AC3_INPUT_3F_1R 0x05 #define AC3_INPUT_2F_2R 0x06 #define AC3_INPUT_3F_2R 0x07
this could be changed to a enum
typedef struct { uint16_t crc1; uint8_t fscod;
uint8_t acmod;
trailing whitespace
uint8_t cmixlev; uint8_t surmixlev; uint8_t dsurmod;
uint8_t blksw; uint8_t dithflag; uint8_t cplinu; uint8_t chincpl; uint8_t phsflginu; uint8_t cplbegf; uint8_t cplendf; uint8_t cplcoe; uint32_t cplbndstrc; uint8_t rematstr; uint8_t rematflg; uint8_t cplexpstr; uint8_t lfeexpstr; uint8_t chexpstr[FBW_CHANNELS]; uint8_t halfratecod; uint8_t sdcycod; uint8_t fdcycod; uint8_t sgaincod; uint8_t dbpbcod; uint8_t floorcod; uint8_t csnroffst; uint8_t cplfsnroffst; uint8_t cplfgaincod; uint8_t fsnroffst[FBW_CHANNELS]; uint8_t fgaincod[FBW_CHANNELS]; uint8_t lfefsnroffst; uint8_t lfefgaincod; uint8_t cplfleak; uint8_t cplsleak; uint8_t cpldeltbae; uint8_t deltbae[FBW_CHANNELS]; uint8_t cpldeltnseg; uint8_t cpldeltoffst[DBA_SEG_MAX]; uint8_t cpldeltlen[DBA_SEG_MAX]; uint8_t cpldeltba[DBA_SEG_MAX]; uint8_t deltnseg[FBW_CHANNELS]; uint8_t deltoffst[FBW_CHANNELS][DBA_SEG_MAX]; uint8_t deltlen[FBW_CHANNELS][DBA_SEG_MAX]; uint8_t deltba[FBW_CHANNELS][DBA_SEG_MAX];
/* Derived Attributes. */ int sampling_rate; int bit_rate; int frame_size;
int nfchans; // number of channels int lfeon; // lfe channel in use
float dynrng; // dynamic range gain float dynrng2; // dynamic range gain for 1+1 mode float chcoeffs[AC3_MAX_CHANNELS]; // normalized channel coefficients float cplco[FBW_CHANNELS][18]; // coupling coordinates int ncplbnd; // number of coupling bands int ncplsubnd; // number of coupling sub bands int cplstrtmant; // coupling start mantissa int cplendmant; // coupling end mantissa int endmant[FBW_CHANNELS]; // channel end mantissas
are all these wierd names from the ac3 spec? if no then id strongly vote for more readable names cplstrtmant -> coupling_start_mantisse for example if they are in the ac3 spec then iam undecided if these names should be keept
uint8_t dcplexps[AC3_MAX_COEFS]; // decoded coupling exponents uint8_t dexps[FBW_CHANNELS][AC3_MAX_COEFS]; // decoded fbw channel exponents uint8_t dlfeexps[AC3_MAX_COEFS]; // decoded lfe channel exponents uint8_t cplbap[AC3_MAX_COEFS]; // coupling bit allocation pointers uint8_t bap[FBW_CHANNELS][AC3_MAX_COEFS]; // fbw channel bit allocation pointers uint8_t lfebap[AC3_MAX_COEFS]; // lfe channel bit allocation pointers
the comments are all not doxygen compatible (///<) [...]
static int16_t dither_int16(AVRandomState *state) { uint32_t y = av_random(state); return (int16_t)((y << 16) >> 16); }
both high and low 16bit should be used due to speed reasons ... [...]
/* * Generate quantizer tables. */ static void generate_quantizers_table(int16_t quantizers[], int level, int length) { int i;
for (i = 0; i < length; i++) quantizers[i] = ((2 * i - level + 1) << 15) / level; }
static void generate_quantizers_table_1(int16_t quantizers[], int level, int length1, int length2, int size) { int i, j; int16_t v;
for (i = 0; i < length1; i++) { v = ((2 * i - level + 1) << 15) / level; for (j = 0; j < length2; j++) quantizers[i * length2 + j] = v; }
for (i = length1 * length2; i < size; i++) quantizers[i] = 0; }
static void generate_quantizers_table_2(int16_t quantizers[], int level, int length1, int length2, int size) { int i, j; int16_t v;
for (i = 0; i < length1; i++) { v = ((2 * (i % level) - level + 1) << 15) / level; for (j = 0; j < length2; j++) quantizers[i * length2 + j] = v; }
for (i = length1 * length2; i < size; i++) quantizers[i] = 0;
}
static void generate_quantizers_table_3(int16_t quantizers[], int level, int length1, int length2, int size) { int i, j;
for (i = 0; i < length1; i++) for (j = 0; j < length2; j++) quantizers[i * length2 + j] = ((2 * (j % level) - level + 1) << 15) / level;
for (i = length1 * length2; i < size; i++) quantizers[i] = 0; }
replace the quadruplicated shit above by generate_quantizers_table(int16_t quantizers[], int level, int length1, int length2, int size, int im, int jm){ int i, j; for (i = 0; i < length1; i++) for (j = 0; j < length2; j++) quantizers[i * length2 + j] = ((2 * (j % jm + i % im) - level + 1) << 15) / level; } also note the = 0 is useless as static&global arrays are initalized to 0 [...]
/* Parse the 'sync_info' from the ac3 bitstream. * This function extracts the sync_info from ac3 bitstream. * GetBitContext within AC3DecodeContext must point to * start of the synchronized ac3 bitstream. * * @param ctx AC3DecodeContext * @return Returns framesize, returns 0 if fscod, frmsizecod or bsid is not valid
almost all code in lav* returns negative values on failure id suggest that ac3 does too doxygen comments start with /** or various other forms but not "/* "
static int ac3_parse_sync_info(AC3DecodeContext *ctx) { GetBitContext *gb = &ctx->gb; int frmsizecod, bsid;
skip_bits(gb, 16); //skip the sync_word, sync_info->sync_word = get_bits(gb, 16); ctx->crc1 = get_bits(gb, 16); ctx->fscod = get_bits(gb, 2); if (ctx->fscod == 0x03) return 0; frmsizecod = get_bits(gb, 6); if (frmsizecod >= 38) return 0;
/* we include it here in order to determine validity of ac3 frame */ bsid = get_bits(gb, 5); if (bsid > 10) return 0; skip_bits(gb, 3); //skip the bsmod, bsi->bsmod = get_bits(gb, 3);
ctx->halfratecod = FFMAX(0, bsid - 8); ctx->sampling_rate = ac3_freqs[ctx->fscod] >> ctx->halfratecod; ctx->bit_rate = ac3_bitratetab[frmsizecod >> 1];
ctx->frame_size = 0; switch (ctx->fscod) { case 0x00: ctx->frame_size = 4 * ctx->bit_rate; break; case 0x01: ctx->frame_size = 2 * (320 * ctx->bit_rate / 147 + (frmsizecod & 1)); break; case 0x02: ctx->frame_size = 6 * ctx->bit_rate; break; }
ctx->bit_rate >>= ctx->halfratecod;
return ctx->frame_size; }
this looks a little bit similar to what is in the avparser, if so it is code duplication which is not ok (put the common code in a common function) [...]
/* Decodes the grouped exponents. * This function decodes the coded exponents according to exponent strategy * and stores them in the decoded exponents buffer. * * @param gb GetBitContext which points to start of coded exponents * @param expstr Exponent coding strategy * @param ngrps Number of grouped exponetns * @param absexp Absolute exponent * @param dexps Decoded exponents are stored in dexps */ static void decode_exponents(GetBitContext *gb, int expstr, int ngrps, uint8_t absexp, uint8_t *dexps) { int i, j, grp, grpsize; int dexp[256]; int aexp[256]; int expacc, prevexp;
// unpack groups grpsize = expstr; if(expstr == EXP_D45) grpsize = 4; for(grp=0; grp<ngrps; grp++) { expacc = get_bits(gb, 7); dexp[grp*3] = expacc / 25; expacc -= 25 * dexp[grp*3]; dexp[(grp*3)+1] = expacc / 5; expacc -= 5 * dexp[(grp*3)+1]; dexp[(grp*3)+2] = expacc;
what about using a table which store the /25 (%25)/5 %5 values? should be faster ...
}
// convert to absolute exps prevexp = absexp; for(i=0; i<ngrps*3; i++) { aexp[i] = prevexp + (dexp[i]-2); aexp[i] = FFMIN(FFMAX(aexp[i], 0), 24);
clip()
prevexp = aexp[i];
also id prevexp += dexp[i]-2; prevexp= clip(prevexp, ...) aexp[i]= prevexp; somehow that looks cleaner to me ... [...]
#define TRANSFORM_COEFF(tc, m, e, f) (tc) = (m) * (f)[(e)]
i hate macros which hide trivial arithmetic operations, please replace all TRANSFORM_COEFF by what they acctually do, and feel free to add a comment to all like // transforms coeff
/* Get the transform coefficients for coupling channel and uncouple channels. * The coupling transform coefficients starts at the the cplstrtmant, which is * equal to endmant[ch] for fbw channels. Hence we can uncouple channels before * getting transform coefficients for the channel. */ static int get_transform_coeffs_cpling(AC3DecodeContext *ctx, mant_groups *m) { GetBitContext *gb = &ctx->gb; int ch, start, end, cplbndstrc, bnd, gcode, tbap; float cplcos[5], cplcoeff; uint8_t *exps = ctx->dcplexps; uint8_t *bap = ctx->cplbap;
cplbndstrc = ctx->cplbndstrc; start = ctx->cplstrtmant; bnd = 0;
while (start < ctx->cplendmant) { end = start + 12; while (cplbndstrc & 1) { end += 12; cplbndstrc >>= 1; } cplbndstrc >>= 1; for (ch = 0; ch < ctx->nfchans; ch++) cplcos[ch] = ctx->chcoeffs[ch] * ctx->cplco[ch][bnd]; bnd++;
while (start < end) { tbap = bap[start]; switch(tbap) { case 0: for (ch = 0; ch < ctx->nfchans; ch++) if (((ctx->chincpl) >> ch) & 1) { if ((ctx->dithflag >> ch) & 1) { TRANSFORM_COEFF(cplcoeff, dither_int16(&ctx->dith_state), exps[start], scale_factors); ctx->transform_coeffs[ch + 1][start] = cplcoeff * cplcos[ch] * LEVEL_MINUS_3DB; } else ctx->transform_coeffs[ch + 1][start] = 0; } start++; continue; case 1: if (m->l3ptr > 2) { gcode = get_bits(gb, 5); m->l3_quantizers[0] = l3_quantizers_1[gcode]; m->l3_quantizers[1] = l3_quantizers_2[gcode]; m->l3_quantizers[2] = l3_quantizers_3[gcode]; m->l3ptr = 0; } TRANSFORM_COEFF(cplcoeff, m->l3_quantizers[m->l3ptr++], exps[start], scale_factors); break;
case 2: if (m->l5ptr > 2) { gcode = get_bits(gb, 7); m->l5_quantizers[0] = l5_quantizers_1[gcode]; m->l5_quantizers[1] = l5_quantizers_2[gcode]; m->l5_quantizers[2] = l5_quantizers_3[gcode]; m->l5ptr = 0; } TRANSFORM_COEFF(cplcoeff, m->l5_quantizers[m->l5ptr++], exps[start], scale_factors); break;
case 3: TRANSFORM_COEFF(cplcoeff, l7_quantizers[get_bits(gb, 3)], exps[start], scale_factors); break;
case 4: if (m->l11ptr > 1) { gcode = get_bits(gb, 7); m->l11_quantizers[0] = l11_quantizers_1[gcode]; m->l11_quantizers[1] = l11_quantizers_2[gcode]; m->l11ptr = 0; } TRANSFORM_COEFF(cplcoeff, m->l11_quantizers[m->l11ptr++], exps[start], scale_factors); break;
case 5: TRANSFORM_COEFF(cplcoeff, l15_quantizers[get_bits(gb, 4)], exps[start], scale_factors); break;
default: TRANSFORM_COEFF(cplcoeff, get_sbits(gb, qntztab[tbap]) << (16 - qntztab[tbap]), exps[start], scale_factors); }
all the TRANSFORM_COEFF are equal excpet the second argument so this could be simplified like ... case 3: coeff= l7_quantizers[get_bits(gb, 3)]; break; case 4: if (m->l11ptr > 1) { gcode = get_bits(gb, 7); m->l11_quantizers[0] = l11_quantizers_1[gcode]; m->l11_quantizers[1] = l11_quantizers_2[gcode]; m->l11ptr = 0; } coeff= m->l11_quantizers[m->l11ptr++]; break; case 5: coeff= l15_quantizers[get_bits(gb, 4)]; break; } TRANSFORM_COEFF(cplcoeff, coeff, exps[start], scale_factors);
for (ch = 0; ch < ctx->nfchans; ch++) if ((ctx->chincpl >> ch) & 1)
maybe chincpl and dithflag should be changed to normal arrays? or maybe not? dunno [...]
if (ch_index != -1) { /* fbw channels */ dithflag = (ctx->dithflag >> ch_index) & 1; exps = ctx->dexps[ch_index]; bap = ctx->bap[ch_index]; coeffs = ctx->transform_coeffs[ch_index + 1]; end = ctx->endmant[ch_index]; } else if (ch_index == -1) {
that else if is silly it must be true if the first is false
dithflag = 0; exps = ctx->dlfeexps; bap = ctx->lfebap; coeffs = ctx->transform_coeffs[0]; end = 7; }
for (i = 0; i < end; i++) { tbap = bap[i]; switch (tbap) { case 0: if (!dithflag) { coeffs[i] = 0; continue; } else { TRANSFORM_COEFF(coeffs[i], dither_int16(&ctx->dith_state), exps[i], factors); coeffs[i] *= LEVEL_MINUS_3DB; continue; }
case 1: if (m->l3ptr > 2) { gcode = get_bits(gb, 5); m->l3_quantizers[0] = l3_quantizers_1[gcode]; m->l3_quantizers[1] = l3_quantizers_2[gcode]; m->l3_quantizers[2] = l3_quantizers_3[gcode]; m->l3ptr = 0; } TRANSFORM_COEFF(coeffs[i], m->l3_quantizers[m->l3ptr++], exps[i], factors); continue;
case 2: if (m->l5ptr > 2) { gcode = get_bits(gb, 7); m->l5_quantizers[0] = l5_quantizers_1[gcode]; m->l5_quantizers[1] = l5_quantizers_2[gcode]; m->l5_quantizers[2] = l5_quantizers_3[gcode]; m->l5ptr = 0; } TRANSFORM_COEFF(coeffs[i], m->l5_quantizers[m->l5ptr++], exps[i], factors); continue;
case 3: TRANSFORM_COEFF(coeffs[i], l7_quantizers[get_bits(gb, 3)], exps[i], factors); continue;
case 4: if (m->l11ptr > 1) { gcode = get_bits(gb, 7); m->l11_quantizers[0] = l11_quantizers_1[gcode]; m->l11_quantizers[1] = l11_quantizers_2[gcode]; m->l11ptr = 0; } TRANSFORM_COEFF(coeffs[i], m->l11_quantizers[m->l11ptr++], exps[i], factors); continue;
case 5: TRANSFORM_COEFF(coeffs[i], l15_quantizers[get_bits(gb, 4)], exps[i], factors); continue;
default: TRANSFORM_COEFF(coeffs[i], get_sbits(gb, qntztab[tbap]) << (16 - qntztab[tbap]), exps[i], factors); continue; } }
return 0; }
duplicate code (see get_transform_coeffs_cpling) [...]
if (get_transform_coeffs_ch(ctx, -1, &m)) return -1;
indention looks slightly wrong [...]
case AC3_INPUT_3F: switch (to) { case AC3_OUTPUT_MONO: nf = LEVEL_MINUS_3DB / (1.0 + clev); ctx->chcoeffs[0] *= (nf * LEVEL_MINUS_3DB); ctx->chcoeffs[2] *= (nf * LEVEL_MINUS_3DB); ctx->chcoeffs[1] *= ((nf * clev * LEVEL_MINUS_3DB) / 2.0);
nf = 0.5 / (1.0 + clev); ctx->chcoeffs[0] *= nf; ctx->chcoeffs[2] *= nf; ctx->chcoeffs[1] *= nf * clev * 0.5; [...]
case AC3_INPUT_2F_1R: switch (to) { case AC3_OUTPUT_MONO: nf = 2.0 * LEVEL_MINUS_3DB / (2.0 + slev); ctx->chcoeffs[0] *= (nf * LEVEL_MINUS_3DB); ctx->chcoeffs[1] *= (nf * LEVEL_MINUS_3DB); ctx->chcoeffs[2] *= (nf * slev * LEVEL_MINUS_3DB);
nf = 1.0 / (2.0 + slev); ctx->chcoeffs[0] *= nf; ctx->chcoeffs[1] *= nf; ctx->chcoeffs[2] *= nf * slev; (LEVEL_MINUS_3DB * LEVEL_MINUS_3DB = 0.5) [...]
/*********** BEGIN DOWNMIX FUNCTIONS ***********/
id put these in their own file
static inline void mix_dualmono_to_mono(AC3DecodeContext *ctx) { int i; float (*output)[AC3_BLOCK_SIZE] = ctx->output;
all these functions seem to just use ctx->output so that should be passed as argument instead of AC3DecodeContext [...]
static inline void upmix_mono_to_stereo(AC3DecodeContext *ctx) { int i; float (*output)[AC3_BLOCK_SIZE] = ctx->output;
for (i = 0; i < 256; i++) output[2][i] = output[1][i];
memcpy()
}
static inline void mix_stereo_to_mono(AC3DecodeContext *ctx) { int i; float (*output)[AC3_BLOCK_SIZE] = ctx->output;
for (i = 0; i < 256; i++) output[1][i] += output[2][i]; memset(output[2], 0, sizeof(output[2])); }
duplicate of mix_dualmono_to_mono() [...]
static inline void mix_2f_1r_to_mono(AC3DecodeContext *ctx) { int i; float (*output)[AC3_BLOCK_SIZE] = ctx->output;
for (i = 0; i < 256; i++) output[1][i] += (output[2][i] + output[3][i]); memset(output[2], 0, sizeof(output[2])); memset(output[3], 0, sizeof(output[3]));
duplicate of mix_3f_to_mono
}
static inline void mix_2f_1r_to_stereo(AC3DecodeContext *ctx) { int i; float (*output)[AC3_BLOCK_SIZE] = ctx->output;
for (i = 0; i < 256; i++) { output[1][i] += output[2][i]; output[2][i] += output[3][i]; } memset(output[3], 0, sizeof(output[3]));
duplicate of mix_3f_to_stereo [...]
static inline void mix_2f_2r_to_mono(AC3DecodeContext *ctx) { int i; float (*output)[AC3_BLOCK_SIZE] = ctx->output;
for (i = 0; i < 256; i++) output[1][i] = (output[2][i] + output[3][i] + output[4][i]); memset(output[2], 0, sizeof(output[2])); memset(output[3], 0, sizeof(output[3])); memset(output[4], 0, sizeof(output[4]));
duplictae of mix_3f_1r_to_mono [...]
for (k = 0; k < N / 8; k++) { o_ptr[2 * k] = -ptr1[k].im * w[2 * k] + d_ptr[2 * k] + 384.0; o_ptr[2 * k + 1] = ptr1[N / 8 - k - 1].re * w[2 * k + 1] + 384.0; o_ptr[N / 4 + 2 * k] = -ptr1[k].re * w[N / 4 + 2 * k] + d_ptr[N / 4 + 2 * k] + 384.0; o_ptr[N / 4 + 2 * k + 1] = ptr1[N / 8 - k - 1].im * w[N / 4 + 2 * k + 1] + d_ptr[N / 4 + 2 * k + 1] + 384.0; d_ptr[2 * k] = ptr2[k].re * w[k / 2 - 2 * k - 1]; d_ptr[2 * k + 1] = -ptr2[N / 8 - k - 1].im * w[N / 2 - 2 * k - 2]; d_ptr[N / 4 + 2 * k] = ptr2[k].im * w[N / 4 - 2 * k - 1]; d_ptr[N / 4 + 2 * k + 1] = -ptr2[N / 8 - k - 1].re * w[N / 4 - 2 * k - 2]; }
what a mess ... [...]
if (get_bits1(gb)) { /* dynamic range */ dynrng = get_sbits(gb, 8); ctx->dynrng = ((((dynrng & 0x1f) | 0x20) << 13) * scale_factors[3 - (dynrng >> 5)]); }
if (acmod == 0x00 && get_bits1(gb)) { /* dynamic range 1+1 mode */ dynrng = get_sbits(gb, 8); ctx->dynrng2 = ((((dynrng & 0x1f) | 0x20) << 13) * scale_factors[3 - (dynrng >> 5)]); }
dynrng/dynrng2 -> dynrng[] and for(i=0; i<= !acmod; i++){ if(get_bits1(gb)){ a= get_sbits(gb, 3); b= get_bits(gb, 5); ctx->dynrng[i]= ((b | 0x20) << 13) * scale_factors[3 - a]; } } [...]
if (!(ctx->cplinu) || ctx->cplbegf > 2) for (rbnd = 0; rbnd < 4; rbnd++) ctx->rematflg |= get_bits1(gb) << rbnd; if (ctx->cplbegf > 0 && ctx->cplbegf <= 2 && ctx->cplinu) for (rbnd = 0; rbnd < 3; rbnd++) ctx->rematflg |= get_bits1(gb) << rbnd; if (ctx->cplbegf == 0 && ctx->cplinu) for (rbnd = 0; rbnd < 2; rbnd++) ctx->rematflg |= get_bits1(gb) << rbnd;
rbnd_len= 4; if(ctx->cplinu) rbnd_len-= (ctx->cplbegf==0) + (ctx->cplbegf<=2); for (rbnd = 0; rbnd < rbnd_len; rbnd++) ctx->rematflg |= get_bits1(gb) << rbnd; [...]
av_log(NULL, AV_LOG_ERROR, "coupling delta bit allocation strategy reserved\n");
av_log(NULL -> av_log(avctx yes all av_log not just this one [...]
if (ctx->cplinu) if (ctx->cpldeltbae == DBA_NEW) { /*coupling delta offset, len and bit allocation */ ctx->cpldeltnseg = get_bits(gb, 3); for (seg = 0; seg <= ctx->cpldeltnseg; seg++) { ctx->cpldeltoffst[seg] = get_bits(gb, 5); ctx->cpldeltlen[seg] = get_bits(gb, 4); ctx->cpldeltba[seg] = get_bits(gb, 3); } }
for (i = 0; i < nfchans; i++) if (ctx->deltbae[i] == DBA_NEW) {/*channel delta offset, len and bit allocation */ ctx->deltnseg[i] = get_bits(gb, 3); for (seg = 0; seg <= ctx->deltnseg[i]; seg++) { ctx->deltoffst[i][seg] = get_bits(gb, 5); ctx->deltlen[i][seg] = get_bits(gb, 4); ctx->deltba[i][seg] = get_bits(gb, 3); } }
hmm the 2 loops above could be merged if cpldelt* where in the same arrays as delt*[] [...]
static inline int16_t convert(int32_t i) { if (i > 0x43c07fff) return 32767; else if (i <= 0x43bf8000) return -32768; else return (i - 0x43c00000); }
duplicate see a52dec/ac3dec.c
//static int frame_count = 0;
hmm, forgotten debug code?
/* Decode ac3 frame. * * @param avctx Pointer to AVCodecContext * @param data Pointer to pcm smaples * @param data_size Set to number of pcm samples produced by decoding * @param buf Data to be decoded * @param buf_size Size of the buffer */ static int ac3_decode_frame(AVCodecContext * avctx, void *data, int *data_size, uint8_t *buf, int buf_size) { AC3DecodeContext *ctx = (AC3DecodeContext *)avctx->priv_data; int frame_start; int16_t *out_samples = (int16_t *)data; int i, j, k, start; int32_t *int_ptr[6];
for (i = 0; i < 6; i++) int_ptr[i] = (int32_t *)(&ctx->output[i]);
//av_log(NULL, AV_LOG_INFO, "decoding frame %d buf_size = %d\n", frame_count++, buf_size);
//Synchronize the frame. frame_start = ac3_synchronize(buf, buf_size);
as the ac3 bitstream, should have been sent through a parser there should be no need to search for any synccode, either its there or theres an error [...]
Index: libavcodec/ac3tab.h =================================================================== --- libavcodec/ac3tab.h (revision 7906) +++ libavcodec/ac3tab.h (working copy) @@ -25,10 +25,10 @@ */
/* possible frequencies */ -static const uint16_t ac3_freqs[3] = { 48000, 44100, 32000 }; +const uint16_t ac3_freqs[3] = { 48000, 44100, 32000 };
/* possible bitrates */ -static const uint16_t ac3_bitratetab[19] = { +const uint16_t ac3_bitratetab[19] = { 32, 40, 48, 56, 64, 80, 96, 112, 128, 160, 192, 224, 256, 320, 384, 448, 512, 576, 640 }; @@ -36,7 +36,7 @@ /* AC3 MDCT window */
/* MDCT window */ -static const int16_t ac3_window[256] = { +const int16_t ac3_window[256] = { 4, 7, 12, 16, 21, 28, 34, 42, 51, 61, 72, 84, 97, 111, 127, 145, 164, 184, 207, 231, 257, 285, 315, 347, @@ -165,41 +165,34 @@ 15, 15, 15, 15, };
global tables need a ff_ prefix to avoid name clashed [...]
+/* delta bit allocation strategy */ +#define DBA_NEW 0x01 +#define DBA_NONE 0x02 +#define DBA_RESERVED 0x03 +#define DBA_REUSE 0x00
could be changed to an enum [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Many that live deserve death. And some that die deserve life. Can you give it to them? Then do not be too eager to deal out death in judgement. For even the very wise cannot see all ends. -- Gandalf
Michael Niedermayer wrote:
Hi
On Sat, Feb 10, 2007 at 01:02:20AM -0500, Justin Ruggles wrote:
Hello,
Here is what I have so far in my attempts to merge the AC-3 decoder written by Kartikey Mahendra BHATT for the Google Summer of Code. There are some things which still need work, and I have some enhancements planned for after it's included. Right now, this gives generally working results, but has some issues.
The 256-point MDCT is not working correctly. I'm still trying to get that fixed. Also, the downmixing seems to not always work like it should. The code could use some clean-up as well.
The patch I've attached is against current SVN. The ac3.c and ac3_decoder.c files go into libavcodec. When it's committed, ac3.c will be created by doing an svn copy of ac3enc.c and then patching it to what I've included here. I think I got the build system stuff right, but I'm not completely sure.
you should have diffed ac3.c against ac3enc.c then, also keep in mind that code duplication (i dont know if theres any after the copy and patch) is unacceptable
Thanks for the review. I was trying to change as little code in the decoder as possible since I'm not the original author, but I'll go ahead and delve into it more to clean it up and add your suggestions. I'll also look closer for any code duplication I may have missed. As far as your suggested changes to the bit allocation, I have many changes ready for that as well, but thought it would be simpler to just leave it as-is first with svn copy and improve it after. That said, I'll go ahead and post my suggested changes all at once, knowing it's understood that it will be multiple commits. Thanks, Justin
Hi On Sat, Feb 10, 2007 at 10:57:39AM -0500, Justin Ruggles wrote:
Michael Niedermayer wrote:
Hi
On Sat, Feb 10, 2007 at 01:02:20AM -0500, Justin Ruggles wrote:
Hello,
Here is what I have so far in my attempts to merge the AC-3 decoder written by Kartikey Mahendra BHATT for the Google Summer of Code. There are some things which still need work, and I have some enhancements planned for after it's included. Right now, this gives generally working results, but has some issues.
The 256-point MDCT is not working correctly. I'm still trying to get that fixed. Also, the downmixing seems to not always work like it should. The code could use some clean-up as well.
The patch I've attached is against current SVN. The ac3.c and ac3_decoder.c files go into libavcodec. When it's committed, ac3.c will be created by doing an svn copy of ac3enc.c and then patching it to what I've included here. I think I got the build system stuff right, but I'm not completely sure.
you should have diffed ac3.c against ac3enc.c then, also keep in mind that code duplication (i dont know if theres any after the copy and patch) is unacceptable
Thanks for the review. I was trying to change as little code in the decoder as possible since I'm not the original author, but I'll go ahead and delve into it more to clean it up and add your suggestions.
I'll also look closer for any code duplication I may have missed. As far as your suggested changes to the bit allocation, I have many changes ready for that as well, but thought it would be simpler to just leave it as-is first with svn copy and improve it after. That said, I'll go ahead and post my suggested changes all at once, knowing it's understood that it will be multiple commits.
iam not opposed to having the decoder commited to svn and then cleaned up afterwards if you prefer ... (as long as nothing breaks and as long as the code does get cleaned up instead of just rotting after commit ...) [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB I count him braver who overcomes his desires than him who conquers his enemies for the hardest victory is over self. -- Aristotle
On Sat, Feb 10, 2007 at 05:09:36PM +0100, Michael Niedermayer wrote:
On Sat, Feb 10, 2007 at 10:57:39AM -0500, Justin Ruggles wrote:
That said, I'll go ahead and post my suggested changes all at once, knowing it's understood that it will be multiple commits.
iam not opposed to having the decoder commited to svn and then cleaned up afterwards if you prefer ... (as long as nothing breaks and as long as the code does get cleaned up instead of just rotting after commit ...)
This sounds like a plan to me, too, but the current ac3dec.c needs to be out of the way first (and libavcodec/liba52/ probably should as well). So OK to remove the file to make the way clear for Justin? Diego
Hi On Sun, Feb 11, 2007 at 12:25:22PM +0100, Diego Biurrun wrote:
On Sat, Feb 10, 2007 at 05:09:36PM +0100, Michael Niedermayer wrote:
On Sat, Feb 10, 2007 at 10:57:39AM -0500, Justin Ruggles wrote:
That said, I'll go ahead and post my suggested changes all at once, knowing it's understood that it will be multiple commits.
iam not opposed to having the decoder commited to svn and then cleaned up afterwards if you prefer ... (as long as nothing breaks and as long as the code does get cleaned up instead of just rotting after commit ...)
This sounds like a plan to me, too, but the current ac3dec.c needs to be out of the way first (and libavcodec/liba52/ probably should as well). So OK to remove the file to make the way clear for Justin?
if ac3dec.c isnt referenced by anything then remove it [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Freedom in capitalist society always remains about the same as it was in ancient Greek republics: Freedom for slave owners. -- Vladimir Lenin
On Sun, Feb 11, 2007 at 01:21:20PM +0100, Michael Niedermayer wrote:
On Sun, Feb 11, 2007 at 12:25:22PM +0100, Diego Biurrun wrote:
On Sat, Feb 10, 2007 at 05:09:36PM +0100, Michael Niedermayer wrote:
On Sat, Feb 10, 2007 at 10:57:39AM -0500, Justin Ruggles wrote:
That said, I'll go ahead and post my suggested changes all at once, knowing it's understood that it will be multiple commits.
iam not opposed to having the decoder commited to svn and then cleaned up afterwards if you prefer ... (as long as nothing breaks and as long as the code does get cleaned up instead of just rotting after commit ...)
This sounds like a plan to me, too, but the current ac3dec.c needs to be out of the way first (and libavcodec/liba52/ probably should as well). So OK to remove the file to make the way clear for Justin?
if ac3dec.c isnt referenced by anything then remove it
Done. Diego
On Sat, 10 Feb 2007 16:30:41 +0100 Michael Niedermayer <michaelni at gmx.at> wrote:
On Sat, Feb 10, 2007 at 01:02:20AM -0500, Justin Ruggles wrote: [...]
#include "avcodec.h" #include "ac3.h" #include "ac3tab.h"
static inline int calc_lowcomp1(int a, int b0, int b1) { if ((b0 + 256) == b1) { a = 384 ; } else if (b0 > b1) { a = a - 64; if (a < 0) a=0; } return a; }
calc_lowcomp1(int a, int b0, int b1, int C){ if ((b0 + 256) == b1) { a= C; } else if (b0 > b1) { a= FFMAX(a - 64, 0); } return a;
I had already spoted lots of potential FFMIN/FFMAX usage before seeing Michael's reply. So here are a few more that I spoted:
[...] end1=bndend; if (end1 > 22) end1=22;
end1 = FFMIN(bndend, 22);
for (bin = begin; bin < end1; bin++) { if (!(is_lfe && bin == 6)) lowcomp = calc_lowcomp(lowcomp, bndpsd[bin], bndpsd[bin+1], bin) ;
fastleak -= s->fdecay ; v = bndpsd[bin] - fgain; if (fastleak < v) fastleak = v;
FFMAX
fastleak = FFMAX(fastleak - s->fdecay, bndpsd[bin] - fgain);
slowleak -= s->sdecay ; v = bndpsd[bin] - s->sgain; if (slowleak < v) slowleak = v;
FFMAX
slowleak = FFMAX(slowleak - s->sdecay, bndpsd[bin] - s->sgain);
v=fastleak - lowcomp; if (slowleak > v) v=slowleak;
v= FFMAX(fastleak - lowcomp, slowleak)
[...]
/* compute masking curve */
for (bin = bndstrt; bin < bndend; bin++) { v1 = excite[bin]; tmp = s->dbknee - bndpsd[bin]; if (tmp > 0) { v1 += tmp >> 2; } v=hth[bin >> s->halfratecod][s->fscod]; if (v1 > v) v=v1; mask[bin] = v;
mask[bin] = FFMAX(hth[bin >> s->halfratecod][s->fscod], v1);
[...] v=mask[j]; v -= snroffset ; v -= s->floor ; if (v < 0) v = 0; v &= 0x1fe0 ; v += s->floor ;
v = (FFMAX(mask[j] - snroffset - s->floor, 0) & 0x1fe0) + s->floor;
end1=bndtab[j] + bndsz[j]; if (end1 > end) end1=end;
end1 = FFMIN(bndtab[j] + bndsz[j], end); Aurel
Hi On Sat, Feb 10, 2007 at 05:29:13PM +0100, Aurelien Jacobs wrote:
On Sat, 10 Feb 2007 16:30:41 +0100 Michael Niedermayer <michaelni at gmx.at> wrote:
On Sat, Feb 10, 2007 at 01:02:20AM -0500, Justin Ruggles wrote: [...]
#include "avcodec.h" #include "ac3.h" #include "ac3tab.h"
static inline int calc_lowcomp1(int a, int b0, int b1) { if ((b0 + 256) == b1) { a = 384 ; } else if (b0 > b1) { a = a - 64; if (a < 0) a=0; } return a; }
calc_lowcomp1(int a, int b0, int b1, int C){ if ((b0 + 256) == b1) { a= C; } else if (b0 > b1) { a= FFMAX(a - 64, 0); } return a;
I had already spoted lots of potential FFMIN/FFMAX usage before seeing Michael's reply. So here are a few more that I spoted:
[...]
v=hth[bin >> s->halfratecod][s->fscod]; if (v1 > v) v=v1; mask[bin] = v;
mask[bin] = FFMAX(hth[bin >> s->halfratecod][s->fscod], v1);
[...] v=mask[j]; v -= snroffset ; v -= s->floor ; if (v < 0) v = 0; v &= 0x1fe0 ; v += s->floor ;
v = (FFMAX(mask[j] - snroffset - s->floor, 0) & 0x1fe0) + s->floor;
i didnt mention these 2 because they look obfuscated, they put too much on a single line IMHO, but then again if justin prefers them ive no objections at all ... [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Freedom in capitalist society always remains about the same as it was in ancient Greek republics: Freedom for slave owners. -- Vladimir Lenin
Michael Niedermayer wrote:
[...]
uint8_t cmixlev; uint8_t surmixlev; uint8_t dsurmod;
uint8_t blksw; uint8_t dithflag; uint8_t cplinu; uint8_t chincpl; uint8_t phsflginu; uint8_t cplbegf; uint8_t cplendf; uint8_t cplcoe; uint32_t cplbndstrc; uint8_t rematstr; uint8_t rematflg; uint8_t cplexpstr; uint8_t lfeexpstr; uint8_t chexpstr[FBW_CHANNELS]; uint8_t halfratecod; uint8_t sdcycod; uint8_t fdcycod; uint8_t sgaincod; uint8_t dbpbcod; uint8_t floorcod; uint8_t csnroffst; uint8_t cplfsnroffst; uint8_t cplfgaincod; uint8_t fsnroffst[FBW_CHANNELS]; uint8_t fgaincod[FBW_CHANNELS]; uint8_t lfefsnroffst; uint8_t lfefgaincod; uint8_t cplfleak; uint8_t cplsleak; uint8_t cpldeltbae; uint8_t deltbae[FBW_CHANNELS]; uint8_t cpldeltnseg; uint8_t cpldeltoffst[DBA_SEG_MAX]; uint8_t cpldeltlen[DBA_SEG_MAX]; uint8_t cpldeltba[DBA_SEG_MAX]; uint8_t deltnseg[FBW_CHANNELS]; uint8_t deltoffst[FBW_CHANNELS][DBA_SEG_MAX]; uint8_t deltlen[FBW_CHANNELS][DBA_SEG_MAX]; uint8_t deltba[FBW_CHANNELS][DBA_SEG_MAX];
/* Derived Attributes. */ int sampling_rate; int bit_rate; int frame_size;
int nfchans; // number of channels int lfeon; // lfe channel in use
float dynrng; // dynamic range gain float dynrng2; // dynamic range gain for 1+1 mode float chcoeffs[AC3_MAX_CHANNELS]; // normalized channel coefficients float cplco[FBW_CHANNELS][18]; // coupling coordinates int ncplbnd; // number of coupling bands int ncplsubnd; // number of coupling sub bands int cplstrtmant; // coupling start mantissa int cplendmant; // coupling end mantissa int endmant[FBW_CHANNELS]; // channel end mantissas
are all these wierd names from the ac3 spec? if no then id strongly vote for more readable names cplstrtmant -> coupling_start_mantisse for example
if they are in the ac3 spec then iam undecided if these names should be keept
Yes, they are from the AC-3 spec. I actually like them. It makes it easier for me to understand since I am very familiar with the spec. Also, the encoder uses many of these same names. I would not be too opposed to expanding some of the abbreviations for easier reading though. -Justin
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. -Justin
On Sat, Feb 24, 2007 at 03:11:17AM -0500, Justin Ruggles wrote:
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.
:)
--- libavcodec/ac3dec.c (revision 0) +++ libavcodec/ac3dec.c (revision 0) @@ -0,0 +1,1245 @@ +/* + * Copyright (c) 2006 Kartikey Mahendra BHATT (bhattkm at gmail dot com). + * Copyright (c) 2007 Justin Ruggles.
My anal self wouldn't put a period at the end of these lines ..
--- libavcodec/Makefile (revision 8113) +++ libavcodec/Makefile (working copy) @@ -248,7 +248,11 @@ OBJS-$(CONFIG_ADPCM_YAMAHA_ENCODER) += adpcm.o
# external codec libraries +ifeq (($CONFIG_LIBA52),yes) OBJS-$(CONFIG_LIBA52) += a52dec.o +else +OBJS-$(CONFIG_AC3_DECODER) += ac3dec.o ac3.o +endif
Just add CONFIG_AC3_DECODER along with the others. We don't do these checks for libvorbis and similar. Diego
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.
[...]
+ + /* audio info */ + int sample_rate; ///< sample rate in Hz + int bit_rate; ///< bit rate in kbps + int frame_size; ///< frame size in bytes + int nfchans; ///< number of full-bandwidth channels + int nchans; ///< number of total channels, including LFE + int lfeon; ///< indicates if LFE channel is present + int lfe_channel; ///< index of LFE channel + int cpl_channel; ///< index of coupling channel + int blkoutput; ///< output configuration for block + int out_channels; ///< number of output channels + float downmix_scale_factors[AC3_MAX_CHANNELS]; ///< downmix gain for each channel
the "audio info" comment is not doxygen compaible, it should rather be /** audio info */ //@{ int balh int foo int bar //@} IIRC the syntax correctly [...]
+static int16_t dither_int16(AVRandomState *state) +{ + uint32_t y = av_random(state); + return (int16_t)(y ^ (y >> 16));
a simple av_random(state) & 0xFFFF; instead of dither_int16(state) will do too, its not optimal but the code above is even slower
+} + +/** + * 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 ... [...]
+ /* generate dynamic range table */ + for(i=0; i<256; i++) { + int v = i >> 5; + if(v > 3) v -= 7; + else v++; + dynrng_tbl[i] = powf(2.0f, v); + v = (i & 0x1F) | 0x20; + dynrng_tbl[i] *= v / 64.0f;
maybe the following gives the same value ... int v= ((int8_t)i >> 5) + 1 - 6; dynrng_tbl[i] = powf(2.0f, v); dynrng_tbl[i] *= (i & 0x1F) | 0x20; [...]
+/** + * 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 [...]
+/** + * Generates transform coefficients for each coupled channel in the coupling + * range using the coupling coefficients and coupling coordinates. + */ +static void uncouple_channels(AC3DecodeContext *ctx) +{ + int i, ch, end, bnd, subbnd; + + subbnd = 0; + for(i=ctx->cplstrtmant,bnd=0; i<ctx->cplendmant; bnd++) { + /* get last bin in coupling band using coupling band structure */ + end = i + 12; + while (ctx->cplbndstrc[subbnd]) { + end += 12; + subbnd++; + } + subbnd++; + + /* calculate transform coeffs */ + while(i < end) { + for(ch=0; ch<ctx->nfchans; ch++) { + if(ctx->chincpl[ch]) { + ctx->transform_coeffs[ch][i] = ctx->cpl_coeffs[i] * + ctx->cplco[ch][bnd]; + } + } + i++; + } + } +}
subbnd = 0; for(i=ctx->cplstrtmant,bnd=0; i<ctx->cplendmant; bnd++) { do{ for(j=0; j<12; j++){ for(ch=0; ch<ctx->nfchans; ch++) { if(ctx->chincpl[ch]) { ctx->transform_coeffs[ch][i] = ctx->cpl_coeffs[i] * ctx->cplco[ch][bnd]; } } i++; } }while(ctx->cplbndstrc[subbnd++]); }
+ +/* ungrouped mantissas */ +typedef struct {
comment not doxygen compatible [...]
+ do { + ctx->transform_coeffs[ch][end] = 0; + } while(++end < 256);
memset() [...]
+/** + * 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? [...]
+ /* phase flags */ + if (acmod == AC3_CHANNEL_MODE_STEREO && ctx->phsflginu && + (ctx->cplcoe & 1 || ctx->cplcoe & 2)) {
ctx->cplcoe & 3 [...]
+ /* coupling and channel exponent strategies */ + got_cplch = !ctx->cplinu; + for(ch=0; ch<nfchans; ch++) { + if(!got_cplch) { + ch = ctx->cpl_channel; + got_cplch = 1; + } + ctx->expstr[ch] = get_bits(gb, 2); + if(ctx->expstr[ch] != EXP_REUSE) { + ctx->bit_alloc_stages[ch] = 3; + } + if(ch == ctx->cpl_channel) + ch = -1; + } + /* lfe exponent strategy */ + if(ctx->lfeon) { + ctx->expstr[ctx->lfe_channel] = get_bits1(gb); + if(ctx->expstr[ctx->lfe_channel] != EXP_REUSE) { + ctx->bit_alloc_stages[ctx->lfe_channel] = 3; + } + }
if(ctx->cplinu){ ctx->expstr[ctx->cpl_channel] = get_bits(gb, 2); } for(ch=0; ch<nfchans; ch++) { ctx->expstr[ch] = get_bits(gb, 2); } if(ctx->lfeon) { ctx->expstr[ctx->lfe_channel] = get_bits1(gb); } for(ch=0; ch<=AC3_MAX_CHANNELS; ch++){ if(ctx->expstr[ch] != EXP_REUSE) ctx->bit_alloc_stages[ch] = 3; } [...]
+ /* fbw & lfe exponents */ + for(ch=0; ch<ctx->nchans; ch++) { + if (ctx->expstr[ch] != EXP_REUSE) { + int ngrps = 2; + if(ch != ctx->lfe_channel) {
isnt ctx->lfe_channel == ctx->nchans ? if so ch will never be == ctx->lfe_channel here i think [...]
+ /* fine snr offsets and fast gain codes */ + got_cplch = !ctx->cplinu; + for(ch=0; ch<ctx->nchans; ch++) { + if(!got_cplch) { + ch = ctx->cpl_channel; + got_cplch = 1; + } + ctx->fsnroffst[ch] = get_bits(gb, 4); + ctx->fgaincod[ch] = get_bits(gb, 3); + if(ch == ctx->cpl_channel) + ch = -1; + }
if(ctx->cplinu){ ctx->fsnroffst[ctx->cpl_channel] = get_bits(gb, 4); ctx->fgaincod [ctx->cpl_channel] = get_bits(gb, 3); } for(ch=0; ch<ctx->nchans; ch++) { ctx->fsnroffst[ch] = get_bits(gb, 4); ctx->fgaincod [ch] = get_bits(gb, 3); } 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 ... [...]
+ /* 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? [...]
-/* possible frequencies */ +/** possible frequencies */ -static const uint16_t ac3_freqs[3] = { 48000, 44100, 32000 }; +const uint16_t ff_ac3_freqs[3] = { 48000, 44100, 32000 };
-/* possible bitrates */ +/** possible bitrates */
great, doxygenization is welcome but not as part of a already >100kb patch which is about adding a ac3 decoder not about cleaning up comments so feel free to apply such seperate changes anytime :)
-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
0x080, 0x100, 0x180, 0x200, 0x280, 0x300, 0x380, 0x400, };
-static const uint8_t bndsz[50]={ +static const uint8_t bndsz[50]= {
cosmetic [...]
+static inline void mix_mono_to_stereo(float output[AC3_MAX_CHANNELS][AC3_BLOCK_SIZE]) +{ + int i; + for(i=0; i<AC3_BLOCK_SIZE; i++) { + output[1][i] = output[0][i]; + }
memcpy() [...]
/* exponent mapping to PSD */ - for(bin=start;bin<end;bin++) { - psd[bin]=(3072 - (exp[bin] << 7)); + for(i=start; i<end; i++) { + psd[i] = 3072 - (exp[i] << 7); }
while i like the new code above better it is a cosmetic change and must be commited seperately if you want to use i instead of bin (and yes you can commit this anytime) [...]
+void ac3_bit_allocation_calc_mask(AC3BitAllocParameters *s, int16_t *mask, + int16_t *bndpsd, int start, int end, + int fgain, int is_lfe, + int deltbae, int deltnseg, + uint8_t *deltoffst, uint8_t *deltlen, + uint8_t *deltba)
static or ff_ prefix, the same applies to all new functions which are non static and dont have a av_ or ff_ prefix [...]
- lowcomp = calc_lowcomp1(lowcomp, bndpsd[0], bndpsd[1]) ; - excite[0] = bndpsd[0] - fgain - lowcomp ; - lowcomp = calc_lowcomp1(lowcomp, bndpsd[1], bndpsd[2]) ; - excite[1] = bndpsd[1] - fgain - lowcomp ; - begin = 7 ; + lowcomp = calc_lowcomp1(lowcomp, bndpsd[0], bndpsd[1], 384); + excite[0] = bndpsd[0] - fgain - lowcomp; + lowcomp = calc_lowcomp1(lowcomp, bndpsd[1], bndpsd[2], 384); + excite[1] = bndpsd[1] - fgain - lowcomp; + begin = 7;
while iam happy with X ; -> X; it is a cosmetic change and should be done seperately, that is feel free to commit such cleanup anytime just not as part of a ac3 decoder patch or commit [...]
@@ -803,755 +226,58 @@ for(i=0;i<50;i++) { bndtab[i] = l; v = bndsz[i]; - for(j=0;j<v;j++) masktab[k++]=i; + for(j=0; j<v; j++) masktab[k++]=i;
cosmetic [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB There will always be a question for which you do not know the correct awnser.
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
On Wed, Mar 07, 2007 at 11:56:47PM -0500, Justin Ruggles wrote:
Michael Niedermayer wrote:
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.
I'm sure further changes like this are easy to do once the code is in mainline. Thus I encourage you to split this off of the AC-3 patch and commit it right away. Diego
Hi Justin Ruggles wrote:
Michael Niedermayer wrote:
[...]
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.
If it's not too hard/boring please do, it could be used right now for other codecs (aac, pcm, vorbis) even if channel order is wrong. Im thinking about ffplay which actually have no downsampling feature, and you cannot play more than 2 channels, except dts/ac3) -- Baptiste COUDURIER GnuPG Key Id: 0x5C1ABAAA SMARTJOG S.A. http://www.smartjog.com Key fingerprint 8D77134D20CC9220201FC5DB0AC9325C5C1ABAAA Phone: +33 1 49966312
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
Justin Ruggles wrote:
Here is what I have so far in my attempts to merge the AC-3 decoder written by Kartikey Mahendra BHATT for the Google Summer of Code. There are some things which still need work, and I have some enhancements planned for after it's included. Right now, this gives generally working results, but has some issues.
The 256-point MDCT is not working correctly. I'm still trying to get that fixed. Also, the downmixing seems to not always work like it should. The code could use some clean-up as well.
It's been over a week now, so I just wanted to let everyone know I am still working on this. I've moved all common code between the encoder, decoder, and parser to ac3.c. The code has been cleaned up and commented. Downmixing works great and is in a separate file, ac3mix.h. Speed is good for stereo content, same speed as liba52 on my system. I still have 3 issues I need to sort out though. 1) still don't have 256-point IMDCT working. I've made it add silence instead of loud pops though. 2) 6-channel output doesn't seem to be working properly. 3) 5.1-channel decoding speed is about 40% slower than liba52 on my system. Once I get these fixed, I'll post a new patch. -Justin
Justin Ruggles wrote:
1) still don't have 256-point IMDCT working. I've made it add silence instead of loud pops though.
I just want to share my excitement. I finally got the 256-point transform working. Now I just have to simplify it. :)
Justin Ruggles wrote:
Justin Ruggles wrote:
1) still don't have 256-point IMDCT working. I've made it add silence instead of loud pops though.
I just want to share my excitement. I finally got the 256-point transform working. Now I just have to simplify it. :)
Great =) lu - wondering if this time he could manage to vectorize it before x86 people ^^ -- Luca Barbato Gentoo/linux Gentoo/PPC http://dev.gentoo.org/~lu_zero
On Sun, Feb 18, 2007 at 11:50:50PM -0500, Justin Ruggles wrote:
Justin Ruggles wrote:
1) still don't have 256-point IMDCT working. I've made it add silence instead of loud pops though.
I just want to share my excitement. I finally got the 256-point transform working. Now I just have to simplify it. :)
\o/ BTW, I think I did all the preparatory work for the native AC3 decoder, i.e. removing internal liba52, renaming the liba52 decoder etc. Is something still missing or is everything ready for the new decoder to get dropped in? Diego
Diego Biurrun wrote:
On Sun, Feb 18, 2007 at 11:50:50PM -0500, Justin Ruggles wrote:
Justin Ruggles wrote:
1) still don't have 256-point IMDCT working. I've made it add silence instead of loud pops though.
I just want to share my excitement. I finally got the 256-point transform working. Now I just have to simplify it. :)
\o/
BTW, I think I did all the preparatory work for the native AC3 decoder, i.e. removing internal liba52, renaming the liba52 decoder etc. Is something still missing or is everything ready for the new decoder to get dropped in?
Everything looks fine as far as preparing for the new decoder. You did mention about coming up with a solution for choosing between different decoders. Is my previous way of doing it ok for now? That is, by appending different object files in the libavcodec Makefile depending on whether or not liba52 is enabled. I also have a question about applying the changes once a final patch set is approved. I want to make sure I have the right idea/order of things. Here is my best guess so far as to how to do it. 1) svn copy ac3enc.c to ac3.c, patched to remove all but bit allocation and ac3_common_init. 2) remove common code from ac3enc.c and make necessary changes so that it uses ac3.c. 3) split up and clean up the common bit allocation (probably 2 or more commits in this step) 4) add header parsing to ac3.c and modify the ac3 parser to use it. 5) add the decoder Thanks, Justin
Hi On Mon, Feb 19, 2007 at 08:34:29PM -0500, Justin Ruggles wrote: [...]
I also have a question about applying the changes once a final patch set is approved. I want to make sure I have the right idea/order of things. Here is my best guess so far as to how to do it.
1) svn copy ac3enc.c to ac3.c, patched to remove all but bit allocation and ac3_common_init.
2) remove common code from ac3enc.c and make necessary changes so that it uses ac3.c.
3) split up and clean up the common bit allocation (probably 2 or more commits in this step)
4) add header parsing to ac3.c and modify the ac3 parser to use it.
5) add the decoder
this looks ok [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB The worst form of inequality is to try to make unequal things equal. -- Aristotle
participants (8)
-
aurel＠gnuage.org -
banan＠student.ltu.se -
baptiste.coudurier＠smartjog.com -
diego＠biurrun.de -
justinruggles＠bellsouth.net -
lu_zero＠gentoo.org -
mans＠mansr.com -
michaelni＠gmx.at