[FFmpeg-devel] [PATCH] OpenEXR decoder rev-3

[Reimar Döffinger]

On Wed, Jul 01, 2009 at 04:06:58PM +0200, Jimmy Christensen wrote:
> +    EXRContext *const s = avctx->priv_data;
> +    AVFrame *const p = &s->picture;

I don't see why const would be particularly appropriate.

> +    char variable_buffer_name[64], variable_buffer_type[64], channel_name[31];

char can be signed or unsigned.

> +    static int bits_per_color_table[3] = {8, 16, 32};

const. Also it looks like it is just 8 << i

> +    uint8_t red_channel   = -1;
> +    uint8_t green_channel = -1;
> +    uint8_t blue_channel  = -1;

~0 looks nicer for unsigned values and also would work on
non-twos-complement systems (though FFmpeg won't work on them anyway).

> +    unsigned long line_offsets[4096];

This are either 32 or 64 bit, depending on the system, are you sure that
is what you wanted? Also 32 kB on the stack really is not reasonable,
such "monsters" usually belong e.g. into the context.

> +    magic_number = AV_RL32(buf);

bytestream_get_le32 might be nicer (even though you'll still have the
addition to skip 4 more bytes...

> +        strcpy(variable_buffer_name, buf);
> +        buf += strlen(variable_buffer_name)+1;
> +        strcpy(variable_buffer_type, buf);
> +        buf += strlen(variable_buffer_type)+1;

One possible buffer overflow after the other.
Apart from that, I don't see the point why you make those copies,
variable_buffer_type is not used at all, and variable_buffer_name can
be compared in-place.

> +        variable_buffer_data_size = AV_RL32(buf);
> +        buf += 4;

bytestream_get_...

> +            const uint8_t *ptr_tmp = buf;
> +            for (channel_iter = 0; (buf + variable_buffer_data_size) > (ptr_tmp + 1); channel_iter++) {

buf + variable_buffer_data_size could overflow. Though that is not
actually an issue here, the issue is that you compare nothing against
the actual remaining size of what buf points to.

> +                strcpy(channel_name, ptr_tmp);
> +                ptr_tmp += strlen(channel_name) + 1;
> +                if (!strcmp(channel_name, "R")) {
> +                    red_channel = channel_iter;
> +                    bits_per_color_id = AV_RL32(ptr_tmp);
> +                }
> +                if (!strcmp(channel_name, "G"))
> +                    green_channel = channel_iter;
> +                if (!strcmp(channel_name, "B"))
> +                    blue_channel = channel_iter;

I can't see the point on the copy here - except another buffer overflow.

> +    switch (bits_per_color_table[bits_per_color_id]) {

If you made the switch simply one bits_per_color_id it at least wouldn't
crash when bits_per_color_id is too big.

> +            av_log(avctx, AV_LOG_ERROR, "Unkown color format : %d\n", bits_per_color_id);

The word is "Unknown" (an 'n' is missing).

> +    // Get the line offsets which are stored as unsigned long

Really? I wonder how they managed that. They are stored in a format that
magically changes from 32 to 64 bits and back when you copy it from a 32
bit to a 64 bit Linux system?

> +    for (i = ymin; i <= ymax; i++) {
> +        line_offsets[i] = AV_RL64(buf);
> +        buf += 8;

bytestream_get_...
Obviously you'd have to check that those line_offsets actually are
valid.

> +    // Zero out the start if ymin is not 0
> +    for (y = 0; y < ymin; y++) {
> +        uint16_t* ptr_x = (uint16_t*)ptr;
> +        for (x = 0; x < avctx->width; x++) {
> +            *ptr_x++ = 0;
> +            *ptr_x++ = 0;
> +            *ptr_x++ = 0;
> +        }

That's what memset is there for.

> +        // Zero out the start if xmin is not 0
> +        for (x = 0; x < xmin; x++) {
> +            *ptr_x++ = 0;
> +            *ptr_x++ = 0;
> +            *ptr_x++ = 0;
> +        }

same here.

> +        // Process the actual pixels
> +        switch (bits_per_color_table[bits_per_color_id]) {
> +            // 32bit float
> +            case 32:
> +                for (x = 0; x < xdelta; x++) {
> +                    buf = avpkt->data + line_offsets[y] + 8 + (xdelta)*4*red_channel + (x)*4;
> +                    *ptr_x++ = (int)(av_int2flt(AV_RL32(buf)) * 65535);
> +                    buf = avpkt->data + line_offsets[y] + 8 + (xdelta)*4*green_channel + (x)*4;
> +                    *ptr_x++ = (int)(av_int2flt(AV_RL32(buf)) * 65535);
> +                    buf = avpkt->data + line_offsets[y] + 8 + (xdelta)*4*blue_channel + (x)*4;
> +                    *ptr_x++ = (int)(av_int2flt(AV_RL32(buf)) * 65535);

You'll probably want to use the same code as for half-float here, except
that denormals are definitely 0 here.

> +                }
> +                break;
> +            // 16bit half float
> +            case 16:
> +                for (x = 0; x < xdelta; x++) {
> +                    buf = avpkt->data + line_offsets[y] + 8 + (xdelta)*2*red_channel + (x)*2;
> +                    *ptr_x++ = exr_halflt2uint(AV_RL16(buf));
> +                    buf = avpkt->data + line_offsets[y] + 8 + (xdelta)*2*green_channel + (x)*2;
> +                    *ptr_x++ = exr_halflt2uint(AV_RL16(buf));
> +                    buf = avpkt->data + line_offsets[y] + 8 + (xdelta)*2*blue_channel + (x)*2;
> +                    *ptr_x++ = exr_halflt2uint(AV_RL16(buf));


You certainly don't need to recalculate buf each time from scratch.