[FFmpeg-devel] [PATCH] AAC decoder

[Michael Niedermayer]

On Fri, Jun 13, 2008 at 01:47:29PM +0100, Robert Swain wrote:
> 2008/6/12 Michael Niedermayer <michaelni at gmx.at>:
> > On Tue, Jun 03, 2008 at 01:26:55AM +0100, Robert Swain wrote:
> >> 2008/6/2 Robert Swain <robert.swain at gmail.com>:
> >> > You'll be happy to know that I'm working on flattening the channel element
> >> > structs (sce_struct, cpe_struct, cc_struct) into one (che_struct). When I
> >> > submit the patch, I expect it will take an iteration or two to be clean
> >> > enough to commit but it has simplified a lot of the code so far. :) There
> >> > will also be some considerations about whether to dynamically allocate some
> >> > things if they're too large to be allocated with every channel element.
> >>
> >> See attached. It's a rather large patch and I tried to make it clean
> >> (not affecting indentation, etc. as I went along) but maybe you will
> >> want it to be split into a number of commits somehow. I don't know
> >> whether it will be particularly beneficial but the simplifications
> >> resulting from the changes to the structs could be committed
> >> separately. It's been tested with stereo and 5.1 files and seems to be
> >> working fine. Comments welcome.
> >
> > Id like it split in a number of patches first.
> > * changing the 4 different structs to 1 array of the same struct
> > * code simplifications
> > * indention fixup to otherwise unchanged lines
> >
> > being a possibility
> 
> See attached.
> 
> Note that I ordered the elements of the che_struct such that the
> cpe_struct code didn't need to be changed until removal of cpe_struct
> was desirable. I hope these are OK. :)
> 
> Thanks,
> Rob

> From 82f9fa27ec080bbe42279558269fc90d646bdc9a Mon Sep 17 00:00:00 2001
> From: Robert Swain <rob at opendot.cl>
> Date: Fri, 13 Jun 2008 11:38:34 +0100
> Subject: [PATCH] Add code necessary to switch to a generic channel element struct
[...]
> +typedef struct {
> +    // CPE specific
> +    int common_window;     ///< Set if channels share a common 'ics_struct' in bitstream
> +    ms_struct ms;
> +    // Shared
> +    sce_struct ch[2];
> +    // CCE specific
> +    coupling_struct coup;
> +} che_struct;

Ive thought that maybe some of these could be placed at the end and only
allocated when needed, either way that would be for a different and
future patch. (it could also be several structs which share the first few
fields)

otherswise ok


> From 025a1d319ad465448edf365a586a77c8186414b9 Mon Sep 17 00:00:00 2001
> From: Robert Swain <rob at opendot.cl>
> Date: Fri, 13 Jun 2008 12:29:11 +0100
> Subject: [PATCH] - Switch to using che_struct in program_config_struct and AACContext
>  - Alter code accordingly such that it is still as functional as before

ok


[...]
> From 5412f7b493fb38471f05b4723256a7d3d9855e2c Mon Sep 17 00:00:00 2001
> From: Robert Swain <rob at opendot.cl>
> Date: Fri, 13 Jun 2008 13:01:11 +0100
> Subject: [PATCH] Switch mixdown code to use che_struct

ok


[...]
> From cb1466531de8a533f528b7cc1a734b1236a048ee Mon Sep 17 00:00:00 2001
> From: Robert Swain <rob at opendot.cl>
> Date: Fri, 13 Jun 2008 13:04:42 +0100
> Subject: [PATCH] Make remaining code that uses cpe_struct use che_struct instead

ok


[...]


> From 219106c85ff29d54a127020ae28f39326c10ca6b Mon Sep 17 00:00:00 2001
> From: Robert Swain <rob at opendot.cl>
> Date: Fri, 13 Jun 2008 13:06:08 +0100
> Subject: [PATCH] Remove unused code

ok

[...]


> From 99acb20d34f87ad4bc3d1e346f469ccb8ec43452 Mon Sep 17 00:00:00 2001
> From: Robert Swain <rob at opendot.cl>
> Date: Fri, 13 Jun 2008 13:25:47 +0100
> Subject: [PATCH] Make code simplifications enabled by previous commits

ok

[...]


> From b1d801dee3dec324c5927892ab0df1399d9c57e5 Mon Sep 17 00:00:00 2001
> From: Robert Swain <rob at opendot.cl>
> Date: Fri, 13 Jun 2008 13:35:43 +0100
> Subject: [PATCH] Cosmetics
> 
> ---
>  aac.c |    8 ++++----
>  1 files changed, 4 insertions(+), 4 deletions(-)
> 

> diff --git a/aac.c b/aac.c
> index 8a86885..b473058 100644
> --- a/aac.c
> +++ b/aac.c
> @@ -491,9 +491,9 @@ static int output_configure(AACContext *ac, program_config_struct *newpcs) {
>          channels += !!pcs->che_type[ID_SCE][i] + !!pcs->che_type[ID_CPE][i] * 2 + !!pcs->che_type[ID_LFE][i];
>  
>          for(j = 0; j < 4; j++) {
> -            if(pcs->che_type[j][i] && !ac->che[j][i]) {
> +            if(pcs->che_type[j][i] && !ac->che[j][i])
>                  ac->che[j][i] = av_mallocz(sizeof(che_struct));
> -        } else
> +            else

i prefer

if(){
}else

because it results in cleaner looking diffs if something is added while we
dont loose a line to a '}'


[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Everything should be made as simple as possible, but not simpler.
-- Albert Einstein
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20080619/18e8ba84/attachment.pgp>