[Ffmpeg-devel] [PATCH] Targa encoder

[Bobby Bingham]

Michael Niedermayer wrote:

>> The third adds RLE 
>> encoding support.
> 
> [...]
>> +
>> +/**
>> + * RLE compress the image, with maximum size of out_size
>> + * @param outbuf Output buffer
>> + * @param out_size Maximum output size
>> + * @param pic Image to compress
>> + * @param bpp Bytes per pixel
>> + * @param w Image width
>> + * @param h Image height
>> + * @return Size of output in bytes, or -1 if larger than out_size
>> + */
>> +static int targa_encode_rle(uint8_t *outbuf, int out_size, AVFrame *pic,
>> +                            int bpp, int w, int h)
>> +{
>> +    int count, x, y;
>> +    uint8_t *ptr, *line, *out;
>> +
>> +    out = outbuf;
>> +    line = pic->data[0];
>> +
>> +    for(y = 0; y < h; y ++) {
>> +        ptr = line;
>> +
>> +        for(x = 0; x < w; x += count) {
>> +            /* see if we can encode the next set of pixels with RLE */
>> +            if((count = count_pixels(ptr, w-x, bpp, 1)) > 1) {
>> +                *out++ = 0x80 | (count - 1);
>> +                memcpy(out, ptr, bpp);
>> +                out += bpp;
>> +
>> +                if(out > outbuf + out_size) return -1;
> 
> writing first and checking later is not a good idea unless you know te buffer
> is large enough ...

The buffer is guaranteed to be large enough.  The value of out_size 
passed to this function is the size that the image would take were it 
uncompressed.  At the beginning of targa_encode_frame(), the size of the 
buffer is checked to ensure it's at least large enough for that size, 
plus the 26 byte footer which goes after the image data (appended at the 
end of targa_encode_frame()).  So, this write of bpp (which is <= 3) 
bytes won't overflow.

But in any case, it can't hurt, and is probably clearer to move the 
check before the memcpy.

> 
> 
>> +            } else {
>> +                /* fall back on uncompressed */
>> +                count = count_pixels(ptr, w-x, bpp, 0);
>> +                *out++ = count - 1;
>> +
>> +                if(out + bpp*count > outbuf + out_size) return -1; 
>> +                memcpy(out, ptr, bpp * count);
>> +                out += bpp * count;
>> +            }
>> +            ptr += count * bpp;
>> +        }
> 
> this loop is not optimal
> for example
> 0 1 1 0
> would be encoded as 0 0 81 1 0 0 but should be encoded as 3 0 1 1 0
> 

Hrmm..  You're right.  I'll take a look at this.
> 
> [...]

-- 
Bobby Bingham
??????????????????????