[FFmpeg-devel] [PATCH 2/4] Add functions which are specific to E-AC-3 decoding.

[Michael Niedermayer]

On Sat, Jul 19, 2008 at 10:48:03PM -0400, Justin Ruggles wrote:
> Michael Niedermayer wrote:
> > On Sat, Jul 19, 2008 at 07:50:26PM -0400, justin.ruggles at gmail.com wrote:
[...]
> > [...]
> >> +static int gaq_ungroup_tab[32][3];
> > 
> > is the function to init this smaller than the table?
> > is int needed or can a smaller data type be used?
> 
> int is not needed. I can't believe I missed that one. :)
> As for the code size vs. table size, that I don't know.  

"nm -S object file" will tell you the code size

[...]

> > [...]
> >> +        // TODO: modify AC3 demuxer to allow user to select which substream to decode
> > 
> > Iam not sure how to support substreams but that is likey not the ideal solution
> > decoding all substreams and reencoding and storing them as seperates streams
> > must be possible ...
> 
> It didn't sound quite right to me either.  I can't think of any other
> good way within the current framework.  Would it be ok to just add a
> comment that we're skipping additional substreams instead of a TODO?  I
> havn't found any samples for this anyway...

ok


> 
> > 
> > [...]
> >> +    s->block_switch_syntax = get_bits1(gbc);
> >> +    if (!s->block_switch_syntax)
> >> +        memset(s->block_switch, 0, sizeof(s->block_switch));
> > 
> > this flag does not seem used anywhere except here thus does not need to
> > be in the context
> > 
> > 
> >> +
> >> +    s->dither_flag_syntax = get_bits1(gbc);
> >> +    if (!s->dither_flag_syntax) {
> > 
> > same
> > 
> > 
> >> +        s->dither_all = 1;
> >> +        for(ch = 1; ch <= s->fbw_channels; ch++)
> >> +            s->dither_flag[ch] = 1;
> >> +    }
> >> +    s->dither_flag[CPL_CH] = s->dither_flag[s->lfe_ch] = 0;
> >> +
> > 
> >> +    s->bit_allocation_syntax = get_bits1(gbc);
> >> +    if (!s->bit_allocation_syntax) {
> > 
> > and this as well
> > 
> > either this is very poorly written or VERY bad split in 2 patches.
> > its hard to review code that is incomplete or unused.
> 
> All those *_syntax variables are used in the decoder from within
> ac3dec.c.  If I combine the 2 patches, it will be pretty large, but if
> that's preferable then I can do it.

what iam saying is that how you seperate the complete in 2 patches was
the most stupid way it could be done.
First adding several unused and unrelated functions in a single patch.
Then adding the code that uses them
I strongly prefer small patches. But they must be self contained.

A patch adding a FFT is self contained even if its unused, as a FFT has
a well known definition, one knows what its input and output should be.

A fuction like ff_eac3_tables_init() is different, i surely see what it
does, but when its entirely unused the obvious question arises where is
it used, its not static so likely from 2 files but where and why from 2.
We have just 1 EAC3 decoder, that is init just from one place i assume ...

Each of these new functions should (if possible) be in a seperate patch
together with the code that uses it.

If i look at your patches the following "technical" cleanness rules fail
* Its not self contained the code is unused, unused code should be removed
  from a patch -> nothing would be left in the first. While the second is
  not compileable as it depends on the first.
* It contains unrelated code (that of each of these unused functions)

I can review an incomplete decoder that implements only some parts of the
decoding process, but its darn hard to review random broken out functions
from a decoder with no comment and no code that uses them. Unless of course
one knows the respective spec very well and fill the missing stuff in from
knowledge, but i sadly do not know *AC3 that well. I know it
approximately but i do not know all the fine details which makes reviewing
such random code snippets rather hard.

[...]
-- 
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
-------------- 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/20080720/fe8558d7/attachment.pgp>