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

[Jimmy Christensen]

On 2009-09-08 10:52, Diego Biurrun wrote:
> On Tue, Sep 08, 2009 at 09:32:22AM +0200, Jimmy Christensen wrote:
>> On 2009-09-08 09:20, Jimmy Christensen wrote:
>>>
>>> Been a long time since I submitted an update on this so here goes :
>>>
>>> Re-designed a few things and made more things into functions. Don't know
>>> what you guys think of the added exr.h header, but I beleive it's the
>>> right thing to do for eg. future support for compression types.
>
> I think you can split out the header file once multiple files actually
> use it, not before...
>

Ok. Good point. Used the tiff decoder for reference since it has much of 
the same compression types.

>> --- libavcodec/exr.c	(revision 0)
>> +++ libavcodec/exr.c	(revision 0)
>> @@ -0,0 +1,458 @@
>> +static inline int get_rgb_channel(const uint8_t **buf, const uint8_t *channel_list_end, EXRContext *const s, int channel_iter, int *current_channel_offset) {
>
> Please use K&R function declarations everywhere and break overly long
> lines where easily possible.  There are a lot of places where you could
> sensibly break long lines.
>

Will do. Thanks.

>> +    if (!strncmp(*buf, "R", 2)) {
>> +        s->red_channel = channel_iter;
>> +    }
>> +    if (!strncmp(*buf, "G", 2)) {
>> +        s->green_channel = channel_iter;
>> +    }
>> +    if (!strncmp(*buf, "B", 2)) {
>> +        s->blue_channel = channel_iter;
>> +    }
>
> pointless {}
>

fixed

>> +        if (channel_iter == s->red_channel) {
>> +            s->bits_per_color_id = current_bits_per_color_id;
>> +            s->red_channel_offset = *current_channel_offset;
>
> align

fixed

>
>> +        if (channel_iter == s->green_channel) {
>> +            s->green_channel_offset = *current_channel_offset;
>> +        }
>> +        if (channel_iter == s->blue_channel) {
>> +            s->blue_channel_offset = *current_channel_offset;
>> +        }
>
> pointless {}
>

fixed

>> +
>> +                variable_buffer_data_size = get_header_variable_length(&buf, buf_end);
>> +                if (!variable_buffer_data_size) {
>> +                    return -1;
>> +                }
>> +
>> +
>> +                variable_buffer_data_size = get_header_variable_length(&buf, buf_end);
>> +                if (!variable_buffer_data_size) {
>> +                    return -1;
>> +                }
>> +
>> +                variable_buffer_data_size = get_header_variable_length(&buf, buf_end);
>> +                if (!variable_buffer_data_size) {
>> +                    return -1;
>> +                }
>> +
>> +                variable_buffer_data_size = get_header_variable_length(&buf, buf_end);
>> +                if (!variable_buffer_data_size) {
>> +                    return -1;
>> +                }
>> +
>> +                variable_buffer_data_size = get_header_variable_length(&buf, buf_end);
>> +                if (!variable_buffer_data_size) {
>> +                    return -1;
>> +                }
>
> pointless {}
>
> But more importantly, this looks like a lot of code duplication.
>

fixed. They differ in that they are checks which needs to be done on 
different locations.

>> +                switch(*buf) {
>> +                    case EXR_RAW:
>> +                        s->compr = *buf;
>> +                        break;
>> +                    case EXR_RLE:
>> +                    case EXR_ZIP1:
>
> Indent the case statements at the same level as the switch, same below.
>

Fixed.

>
>> +                for (int i = 0; i<  2; i++) {
>> +                    // Skip variable name/type
>> +                    while (buf++<  buf_end) {
>> +                        if (buf[0] == 0x0)
>> +                            break;
>> +                    }
>
> pointless {}
>

Fixed.

>> +                }
>> +                buf++;
>> +                // Skip variable length
>> +                if (buf_end - buf>= 5) {
>> +                    variable_buffer_data_size = get_header_variable_length(&buf, buf_end);
>> +                    if (!variable_buffer_data_size) {
>> +                        return -1;
>> +                    }
>
> pointless {}
>

Fixed

>> +    } else {
>> +        if (s->red_channel   == -1) {
>> +            av_log(avctx, AV_LOG_ERROR, "Missing red channel\n");
>> +        }
>> +        if (s->green_channel == -1) {
>> +            av_log(avctx, AV_LOG_ERROR, "Missing green channel\n");
>> +        }
>> +        if (s->blue_channel  == -1) {
>> +            av_log(avctx, AV_LOG_ERROR, "Missing blue channel\n");
>> +        }
>
> pointless {}
>

Fixed.

>> Property changes on: libavcodec/exr.h
>> ___________________________________________________________________
>> Added: svn:mime-type
>>     + text/plain
>
> This is weirdness.

Didn't see those, thanks. The file is removed anyway now.


Thanks for the review. Guess too much vacation makes you forget the 
basic stuff :)
-------------- next part --------------
A non-text attachment was scrubbed...
Name: OpenEXR-rev15.diff
Type: text/x-patch
Size: 19592 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20090908/66460331/attachment.bin>