[FFmpeg-soc] j2k review arithmetic coding part

[Michael Niedermayer]

Hi

heres the first part of the j2k review (ill review the rest of it ASAP
but as the arithmetic coder is sufficiently independant theres no sense
in delaying this)

aec.c:
[...]
> void ff_aec_init_contexts(AecState *aec)
> {
>     memset(aec->contexts, 0, 19*sizeof(AecContext));

sizeof(aec->contexts) would be better

[...]

aec.h:
[...]

> const static AecCxState cx_states[47] = {
>     {0x5601,  1,  1, 1},
>     {0x3401,  2,  6, 0},
>     {0x1801,  3,  9, 0},
>     {0x0AC1,  4, 12, 0},
>     {0x0521,  5, 29, 0},
>     {0x0221, 38, 33, 0},
>     {0x5601,  7,  6, 1},
>     {0x5401,  8, 14, 0},
>     {0x4801,  9, 14, 0},
>     {0x3801, 10, 14, 0},
>     {0x3001, 11, 17, 0},
>     {0x2401, 12, 18, 0},
>     {0x1C01, 13, 20, 0},
>     {0x1601, 29, 21, 0},
>     {0x5601, 15, 14, 1},
>     {0x5401, 16, 14, 0},
>     {0x5101, 17, 15, 0},
>     {0x4801, 18, 16, 0},
>     {0x3801, 19, 17, 0},
>     {0x3401, 20, 18, 0},
>     {0x3001, 21, 19, 0},
>     {0x2801, 22, 19, 0},
>     {0x2401, 23, 20, 0},
>     {0x2201, 24, 21, 0},
>     {0x1C01, 25, 22, 0},
>     {0x1801, 26, 23, 0},
>     {0x1601, 27, 24, 0},
>     {0x1401, 28, 25, 0},
>     {0x1201, 29, 26, 0},
>     {0x1101, 30, 27, 0},
>     {0x0AC1, 31, 28, 0},
>     {0x09C1, 32, 29, 0},
>     {0x08A1, 33, 30, 0},
>     {0x0521, 34, 31, 0},
>     {0x0441, 35, 32, 0},
>     {0x02A1, 36, 33, 0},
>     {0x0221, 37, 34, 0},
>     {0x0141, 38, 35, 0},
>     {0x0111, 39, 36, 0},
>     {0x0085, 40, 37, 0},
>     {0x0049, 41, 38, 0},
>     {0x0025, 42, 39, 0},
>     {0x0015, 43, 40, 0},
>     {0x0009, 44, 41, 0},
>     {0x0005, 45, 42, 0},
>     {0x0001, 45, 43, 0},
>     {0x5601, 46, 46, 0}
> };

the array should be in aec.c


> 
> typedef struct {
>     unsigned int state;
>     unsigned int mps;

mps and state can be merged so that mps is the least signifcant bit
see cabac.c/h if you dont know what i mean


[...]
> /**
>  * number of encoded bytes
>  */
> int ff_aec_length(AecState *aec);
> 
> /**
>  * flush the encoder [returns number of bytes encoded]
>  * */

* */ vs. */ is inconsistant


> int ff_aec_flush(AecState *aec);
> 

> /** decoder */
> 
> void ff_aec_initdec(AecState *aec, uint8_t *bp);

doxygen comment isnt correct for the function


[...]

aecdec.c:
[...]
> /**
>  * Arithmetic entropy decoder
>  * @file aecdec.c

the aec* names are a little generic, we already have an arithmetic coder
in lavc maybe you could give the files more specific names


[...]
>     do{
>         if (!aec->ct)
>             bytein(aec);
>         aec->a = aec->a << 1;
>         aec->c = aec->c << 1;

aec->c += aec->c;


>         aec->ct--;

i think it would be possible to put a marker bit in aec->c so that
the ct-- could be avoided and instead a check like
if(!(aec->c & 0xFF))
    bytein(aec);
could be used

this would avoid reading, writing and increasng the ct field of the struct
the same trick might also be possible on the encoder side


[...]
> int ff_aec_decode(AecState *aec, int cx)
> {
>     aec->curctx = aec->contexts + cx;

this is likely inefficient, you could just pass a pointer to the current
context as argument to ff_aec_decode and any other (likely inlined)
functions which need it


[...]

aecenc.c:
[...]
> void ff_aec_encode(AecState *aec, int cx, int d)
> {
>     int qe;
> 
>     aec->curctx = aec->contexts + cx;

as with the decoder, storing the current ctx in the struct seems inefficient


[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

I know you won't believe me, but the highest form of Human Excellence is
to question oneself and others. -- Socrates
-------------- 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-soc/attachments/20070819/259be529/attachment.pgp>