[FFmpeg-devel] [PATCH][RFC] Lagarith Decoder.

[Diego Biurrun]

On Mon, Aug 10, 2009 at 11:42:19PM -0600, Nathan Caldwell wrote:
> On Sat, Aug 8, 2009 at 6:32 AM, Michael Niedermayer<michaelni at gmx.at> wrote:
> > On Thu, Aug 06, 2009 at 03:06:32PM -0600, Nathan Caldwell wrote:
> >> Here's my first attempt at a Lagarith decoder. At the moment it only
> >> handles YV12 content, I do plan on adding in the other modes (RGB24,
> >> YUY2, and RGBA). I just wanted some input on things that need changed
> >> before I get too far along.
> > [...]
> >> +
> >> +typedef struct lag_rac {
> >> +    AVCodecContext *avctx;
> >> +    unsigned low;
> >> +    unsigned range;
> >> +    unsigned scale;
> >> +    unsigned hash_shift;
> >> +
> >> +    uint8_t *bytestream_start;
> >> +    uint8_t *bytestream;
> >> +    uint8_t *bytestream_end;
> >> +
> >> +    int prob[257];
> >> +    int range_hash[256];
> >> +} lag_rac;
> >
> > the coder could be put in a seperate file and patch
> 
> Done. Just curious though, why?

1) Smaller patches are quicker to get reviewed and applied.
2) We try to support a modular build as much as possible.

> --- /dev/null
> +++ b/libavcodec/lagrange.c
> @@ -0,0 +1,55 @@
> +
> +/**
> + * @file libavcodec/lagrange.c
> + * Lagarith range decoder.

Drop the periods at the end of non-sentences.

> +void lag_rac_init(lag_rac * l, GetBitContext * gb, int length)

*l, *gb

> +    align_get_bits(gb);
> +    l->bytestream_start =
> +        l->bytestream = gb->buffer + get_bits_count(gb) / 8;
> +    l->bytestream_end = l->bytestream_start + length;
> +
> +    l->range = 0x80;
> +    l->low = *l->bytestream >> 1;
> +    l->hash_shift = FFMAX(l->scale - 8, 0);

vertical alignment, more opportunities below

> --- /dev/null
> +++ b/libavcodec/lagrange.h
> @@ -0,0 +1,93 @@
> +/**
> + * @file libavcodec/lagrange.h
> + * Lagarith range decoder.

see above

> +#endif

Add a comment, similar to all our otherr header files.

> --- /dev/null
> +++ b/libavcodec/lagarith.c
> @@ -0,0 +1,475 @@
> +/*
> + * Lagarith Decoder

decoder

> +/**
> + * @file libavcodec/lagarith.c
> + * Lagarith Lossless Decoder

ditto

> +static av_cold int lag_decode_init(AVCodecContext * avctx)

*avctx, more below

> +static void lag_memset(uint8_t * s, uint8_t c, size_t n, int step)
> +{
> +    if (n == 0)

if (!n)

more such instances below

> +static uint8_t *lag_memcpy(uint8_t * dest, const uint8_t * src, size_t n,
> +                    int step)

indentation

> +        /* Comment from reference source:
> +         * if ( b&0x80==0 ){        // order of operations is 'wrong'; it has been left this way

if (b & 0x80 == 0) {

> +         *      b=-(signed int)b;
> +         *      b&=0xFF;
> +         * } else{

some more whitespace would help here

> +            if (dst + l->zeros_rem * step > end) {
> +                count = (end - dst) / step;
> +            }

pointless {}

> --- /dev/null
> +++ b/libavcodec/lagarith.h
> @@ -0,0 +1,61 @@
> +/*
> + * Lagarith Decoder

decoder

> +/**
> + * @file libavcodec/lagarith.h
> + * Lagarith Lossless Decoder

same

Why do you call it lossless in one place, but not the other?

> +static const uint8_t run_table[]={0,2,4,6,8,10,12,14,16,18,20,22,24,26,28,30,32,
> +	34,36,38,40,42,44,46,48,50,52,54,56,58,60,62,64,66,68,70,72,74,76,78,80,82,84,
> +	86,88,90,92,94,96,98,100,102,104,106,108,110,112,114,116,118,120,122,124,126,
> +	128,130,132,134,136,138,140,142,144,146,148,150,152,154,156,158,160,162,164,
> +	166,168,170,172,174,176,178,180,182,184,186,188,190,192,194,196,198,200,202,
> +	204,206,208,210,212,214,216,218,220,222,224,226,228,230,232,234,236,238,240,
> +	242,244,246,248,250,252,254,255,253,251,249,247,245,243,241,239,237,235,233,
> +	231,229,227,225,223,221,219,217,215,213,211,209,207,205,203,201,199,197,195,
> +	193,191,189,187,185,183,181,179,177,175,173,171,169,167,165,163,161,159,157,
> +	155,153,151,149,147,145,143,141,139,137,135,133,131,129,127,125,123,121,119,
> +	117,115,113,111,109,107,105,103,101,99,97,95,93,91,89,87,85,83,81,79,77,75,73,
> +	71,69,67,65,63,61,59,57,55,53,51,49,47,45,43,41,39,37,35,33,31,29,27,25,23,21,
> +	19,17,15,13,11,9,7,5,3,1};

Spaces could help here.

> +#endif

comment

Diego