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

[Gregory Montoir]

Hi,

Here's an updated patch, most of your comments taken into account.


Michael Niedermayer wrote:
> Hi
> 
> [...]
>> +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

Yes, this simplifies the code. I did the change in other decoding
functions as well.


> [...]
>> +    /* 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 ...

Yes, indeed.
FWIW, there's the same problem with VqaContext (in vqavideo.c).


>> + [...]
>> +
>> +    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

Indeed, nice idea. Yet, I haven't introduced new variables, I just
swap pointers values directly.


> [...]
> 
>> +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

Yes, a static table for this is definitely smaller than the amount of
code needed to generate it (0xC80 -> 0x200). And accessing it will be
faster that way too.


> [...]


Regards,
Gregory
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: ffmpeg-delphinecin-20060930.diff
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20061001/30880ac6/attachment.txt>