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

Jimmy Christensen jimmy
Wed Jul 1 23:00:14 CEST 2009


On 2009-07-01 17:57, Reimar D?ffinger wrote:
> 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.

Originally taken from the tga decoder.

>
>> +    char variable_buffer_name[64], variable_buffer_type[64], channel_name[31];
>
> char can be signed or unsigned.

Should have used uint8_t instead, thanks.

>
>> +    static int bits_per_color_table[3] = {8, 16, 32};
>
> const. Also it looks like it is just 8<<  i
>

It was just to be descriptive and according to the documentation of the 
format. If eg. the format is updated with 3 = 12-bit it would be rather 
simple to implement it.

>> +    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).
>

Will change that.

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

With this, the maximum of the image height is only 4k which may not be 
enough.

>> +    magic_number = AV_RL32(buf);
>
> bytestream_get_le32 might be nicer (even though you'll still have the
> addition to skip 4 more bytes...

Will use that instead.

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

Will do that. It was just to have the code a bit more ordered. The 
header is IMHO a weird combination of strings and bytes. I guess to make 
it more flexible.

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

Will use that instead.

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

Will look into it.

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

Yeah, you're right, will change it.

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

See my previous comment about bits_per_color_id.

>> +            av_log(avctx, AV_LOG_ERROR, "Unkown color format : %d\n", bits_per_color_id);
>
> The word is "Unknown" (an 'n' is missing).

Whoops, will fix.

>
>> +    // 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?

It's stated in the documentation for the fileformat :

The line offset table allows random access to scan line blocks. The 
table is a sequence of scan line offsets, with one offset per scan line 
block. A scan line offset, of type unsigned long, indicates the 
distance, in bytes, between the start of the file and the start of the 
scan line block. In the table, scan line offsets are ordered according 
to increasing scan line y coordinates.

I would suspect that the idea perhaps is that on 64-bit systems it can 
use the full range, but not on 32-bit systems.

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

Thanks. Will look at that.

>
>> +    // 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.

Thanks, didn't know about that.

>
>> +        // 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.

Will fix.

>
>> +        // 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.
>

Yes, but need to have a function that works with 32-bit floats first. 
This is the reference method.

>> +                }
>> +                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.
>

Not happy with this part either :)

-- 
Best Regards
Jimmy Christensen
Developer
Ghost A/S



More information about the ffmpeg-devel mailing list