[Ffmpeg-devel] Re: [PATCH] Delphine Software .CIN files support

[Michael Niedermayer]

Hi

On Fri, Sep 29, 2006 at 08:51:13PM +0200, Gregory Montoir wrote:
> 
> Please ignore the previous mail, the patch it refers to is outdated.
> Attached an up-to-date one.
> 
> Regards,
> Gregory
> 

[...]
> +static int cin_decode_huffman(const unsigned char *src, int src_size, unsigned char *dst, int dst_size)
> +{
> +    unsigned char b, huff_code = 0;

why are these char and not int?


[...]
> +static void cin_decode_rle(const unsigned char *src, int src_size, unsigned char *dst, int dst_size)
> +{
> +    int len;
> +    unsigned char code;
> +
> +    while (src_size > 0 && dst_size > 0) {
> +        code = *src++; --src_size;
> +        if (code & 0x80) {
> +            len = code - 0x7F;
> +            memset(dst, *src++, FFMIN(len, dst_size));
> +            --src_size;
> +        } else {
> +            len = code + 1;
> +            memcpy(dst, src, FFMIN(len, dst_size));
> +            src += len;
> +            src_size -= len;
> +        }
> +        dst += len;
> +        dst_size -= len;
> +    }

id suggest to replace the src_size stuff by a src_end= src+src_size that
would be simpler and avoids having to maintain the src_size value
this probably also applies to a few other functions


[...]
> +    /* handle palette */
> +    if (palette_type == 0) {
> +        for (i = 0; i < palette_colors_count; ++i) {
> +            cin->palette[i] = (buf[2] << 16) | (buf[1] << 8) | buf[0];
> +            buf += 3;
> +            bitmap_frame_size -= 3;
> +        }
> +    } else {
> +        for (i = 0; i < palette_colors_count; ++i) {
> +            cin->palette[buf[0]] = (buf[3] << 16) | (buf[2] << 8) | buf[1];
> +            buf += 4;
> +            bitmap_frame_size -= 4;
> +        }
> +    }
> +    memcpy(cin->frame.data[1], cin->palette, sizeof(cin->palette));

the palette in AVFrame should be uint32_t not int entries, theres no
gurantee that int is 32bit and not maybe 64 ...


> +    cin->frame.palette_has_changed = 1;
> +
> +    /* note: the decoding routines below assumes that surface.width = surface.pitch */
> +    switch (bitmap_frame_type) {
> +    case 9:
> +        cin_decode_rle(buf, bitmap_frame_size,
> +          cin->bitmap_table[CIN_CUR_BMP], cin->bitmap_size);
> +        break;
> +    case 34:
> +        cin_decode_rle(buf, bitmap_frame_size,
> +          cin->bitmap_table[CIN_CUR_BMP], cin->bitmap_size);
> +        cin_apply_delta_data(cin->bitmap_table[CIN_PRE_BMP],
> +          cin->bitmap_table[CIN_CUR_BMP], cin->bitmap_size);
> +        break;
> +    case 35:
> +        cin_decode_huffman(buf, bitmap_frame_size,
> +          cin->bitmap_table[CIN_INT_BMP], cin->bitmap_size);
> +        cin_decode_rle(cin->bitmap_table[CIN_INT_BMP], bitmap_frame_size,
> +          cin->bitmap_table[CIN_CUR_BMP], cin->bitmap_size);
> +        break;
> +    case 36:
> +        bitmap_frame_size = cin_decode_huffman(buf, bitmap_frame_size,
> +          cin->bitmap_table[CIN_INT_BMP], cin->bitmap_size);
> +        cin_decode_rle(cin->bitmap_table[CIN_INT_BMP], bitmap_frame_size,
> +          cin->bitmap_table[CIN_CUR_BMP], cin->bitmap_size);
> +        cin_apply_delta_data(cin->bitmap_table[CIN_PRE_BMP],
> +          cin->bitmap_table[CIN_CUR_BMP], cin->bitmap_size);
> +        break;
> +    case 37:
> +        cin_decode_huffman(buf, bitmap_frame_size,
> +          cin->bitmap_table[CIN_CUR_BMP], cin->bitmap_size);
> +        break;
> +    case 38:
> +        cin_decode_lzss(buf, bitmap_frame_size,
> +          cin->bitmap_table[CIN_CUR_BMP], cin->bitmap_size);
> +        break;
> +    case 39:
> +        cin_decode_lzss(buf, bitmap_frame_size,
> +          cin->bitmap_table[CIN_CUR_BMP], cin->bitmap_size);
> +        cin_apply_delta_data(cin->bitmap_table[CIN_PRE_BMP],
> +          cin->bitmap_table[CIN_CUR_BMP], cin->bitmap_size);
> +        break;
> +    }
> +
> +    for (y = 0; y < cin->avctx->height; ++y)
> +        memcpy(cin->frame.data[0] + (cin->avctx->height - 1 - y) * cin->frame.linesize[0],
> +          cin->bitmap_table[CIN_CUR_BMP] + y * cin->avctx->width,
> +          cin->avctx->width);
> +
> +    memcpy(cin->bitmap_table[CIN_PRE_BMP],
> +      cin->bitmap_table[CIN_CUR_BMP],
> +      cin->bitmap_size);

id make CIN_PRE_BMP and CIN_CUR_BMP variables and exchange them so this
memcpy could be avoided

[...]

> +static void cinaudio_build_delta16_table(CinAudioContext *cin) {
> +    const double k = pow(32767., 1. / 128);
> +    int i, delta;
> +    double u = k;
> +
> +    cin->delta16_table[128] = 0;
> +    cin->delta16_table[127] = 0;
> +    for (delta = 0, i = 129; i < 237;) {
> +        if (delta != (int)u) {
> +            delta = (int)u;
> +            cin->delta16_table[i]       =  delta;
> +            cin->delta16_table[255 - i] = -delta;
> +            ++i;
> +        }
> +        u *= k;
> +    }
> +    for (i = 237; i < 256; ++i) {
> +        cin->delta16_table[i]       = 0;
> +        cin->delta16_table[255 - i] = 0;
> +    }
> +
> +    cin->initial_delta = 1;
> +    cin->delta = 0;

how many bytes does this function need? (nm -S delphinecinav.o)
wouldnt a hardcoded int16_t table be smaller? it would also preempt
any floating point rounding issues

if you keep the above code then the table should be a static one and
not be duplicated in every decoder context

[...]


> +    int pal_colors_count;
[...]
> +    hdr->pal_colors_count = get_le16(pb);
[...]
> +        if (hdr->pal_colors_count < 0) {

hdr->pal_colors_count cannot be < 0 as its read as unsigned 16bit into an int


[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

In the past you could go to a library and read, borrow or copy any book
Today you'd get arrested for mere telling someone where the library is