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

[Diego Biurrun]

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

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

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

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

align

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

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

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


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

pointless {}

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

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

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

This is weirdness.

Diego