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

[Alexander Strange]

On Aug 6, 2009, at 5:06 PM, 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.
>
> --  
> -Nathan Caldwell
> <lagarith-decoder-1.patch>

The file has mixed windows/unix line endings too.
(and hungarian notation)

A quick look:

> +        l->low |= 0xff & (l->bytestream[0] << 7 | l->bytestream[1]  
> >> 1);

AV_RB16() << 1
(cabac.h does this too...)

See if you can make lag_get_rac branchless.

> +	unsigned int series[]={1,2,3,5,8,13,21,34};

Fits in uint8_t and should be a const static variable with the others.

> +	unsigned int bit=0;
> +	unsigned int series[]={1,2,3,5,8,13,21,34};
> +	unsigned int prevbit=0;
> +	unsigned int bits=0;
> +	unsigned int i=0;
> +	unsigned int value=1;

Isn't int fine? (this applies everywhere else too)

> +static inline uint32_t clp2(uint32_t x)

This name is kind of obscure.

> +    scale_factor = temp/(double)cumul_prob;

If the reference code is really this crazy, better use fixed-point to  
emulate it.

> +    if ( (signed int)scaled_cumul_prob < 0 ){

This might not pass -Wstrict-overflow, try > INT_MAX.

> +        return 0;

Return -1 for errors.

> +static void lag_pred_line(LagarithContext *l, uint8_t *p_buf, int  
> i_width, int i_stride, int i_step, int i_mode)
> +{
> +  ...
> +    return;
> +}

Useless.

> +            if(i+count*i_step > width_scaled)
> +                count = (width_scaled - i)/i_step;

Can you get rid of the divide?

> +    uint8_t mask1 = i_esc_count < 2 ? 0x00 : 0xff;
> +    uint8_t mask2 = i_esc_count < 3 ? 0x00 : 0xff;

-(i_esc_count < 2) if it matters.

> +            length = *(const uint32_t*)(p_src+1);

AV_RL32

> +    offset_gu = *(const uint32_t*)(buf+1);
> +    offset_bv = *(const uint32_t*)(buf+5);

Same.

> +    *picture= *(AVFrame*)&l->picture;
> +    *data_size = sizeof(AVPicture);

?