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

[Reimar Döffinger]

On Tue, Sep 08, 2009 at 03:30:26PM +0200, Jimmy Christensen wrote:
> +static const unsigned int bytes_per_color_table[3] = {
> +        1, 2, 4
> +};

Currently that is of course the same as 1 << n.
Anyway the main point is
a) this is used only in one function so better declare it there
b) should use uint8_t so it does not take up so much space.

> +static inline int check_header_variable(const uint8_t *buf,
> +                                        const uint8_t *buf_end,
> +                                        const char *value_name,
> +                                        const char *value_type,
> +                                        unsigned int minimum_length)

I think all functions lack doxygen documentation.
Also since speed is not very relevant for header parsing most of these
probably should not be inline

> +{
> +    if (buf_end - buf >= minimum_length && !strncmp(buf, value_name, strlen(value_name))) {
> +        buf += strlen(value_name)+1;
> +        if (!strncmp(buf, value_type, strlen(value_type)))
> +            return 1;
> +        av_log(NULL, AV_LOG_ERROR, "Unknown data type for header variable %s\n", value_name);
> +    }
> +    return 0;

Please don't do this, for return values either use
1 good, 0 error
or
0 good, < 0 error

> +    unsigned int variable_buffer_data_size = bytestream_get_le32(buf);
> +    if (buf_end - *buf <= variable_buffer_data_size) {
> +        av_log(NULL, AV_LOG_ERROR, "Incomplete header\n");
> +        return 0;
> +    } else {
> +        return variable_buffer_data_size;
> +    }

You return in the if path, there is no need for "else".

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

strcmp, no need for strncmp.

> +    if (channel_iter == s->red_channel ||
> +        channel_iter == s->green_channel ||
> +        channel_iter == s->blue_channel) {
> +        *buf += 2;
> +        current_bits_per_color_id = bytestream_get_le32(buf);
> +        if (current_bits_per_color_id > 2)
> +            return -1;
> +        if (channel_iter == s->red_channel) {
> +            s->bits_per_color_id  = current_bits_per_color_id;
> +            s->red_channel_offset = *current_channel_offset;
> +        }
> +        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;

This seems very convoluted, why is red special? And are you misusing
those "channel_iter == s->red_channel" tests to check which one you've
just assigned?
This might be nicer with
{red,green,blue}_channel_offset instead being and array channel_offsets
and maybe enum {RED_CHANNEL, GREEN_CHANNEL, BLUE_CHANNEL} (or just 0 - 2
manually).

> +    if (*buf < channel_list_end) {
> +        while (*buf[0] != 0x0)
> +            *buf += 1;
> +        *buf += 1;

e.g.
while (bytestream_get(&buf)) /* skip */;
More importantly though this still misses a check so it will not overrun
beyond the end of the buffer.

> +        current_bits_per_color_id = bytestream_get_le32(buf);
> +        if (current_bits_per_color_id > 2)
> +            return -1;
> +        *current_channel_offset += bytes_per_color_table[current_bits_per_color_id];
> +        *buf += 12;
> +    }

This duplicates code from the RGB case.

> +    if (buf_end - buf < 10) {
> +        av_log(avctx, AV_LOG_ERROR, "Too short header to parse\n");
> +        return -1;
> +    } else {

Return in if -> do not add else to avoid unnecessary indentation and to
avoid cluttering the "default" code path due to error handling.

> +                channel_list_end += variable_buffer_data_size;
> +                while (channel_list_end >= buf + 19) {

Thanks to padding it is probably not wrong, but it is inconsistent when
channel_list_end - buf >= 19 is used elsewhere.

> +                    if (get_rgb_channel(&buf,
> +                                        channel_list_end,
> +                                        s,
> +                                        channel_iter,
> +                                        &current_channel_offset) == -1) {

> +            // Process the dataWindow variable
> +            if (check_header_variable(buf, buf_end, "dataWindow", "box2i", 31) == 1) {

Which so specific (i.e. == -1 and == 1?) If someone extended those
functions to return other value, would it really make sense?
Or in other words, it should be
if (get_rgb_channel() < 0)
and
if (check_header_variable())

> +                    while (buf++ < buf_end)
> +                        if (buf[0] == 0x0)
> +                            break;

That skips a string just like another piece of code but checks for the
buffer end.
Maybe this should just be a function, then you can't forgot some of the checks
in half of the places.

> +    if (buf < buf_end) {
> +        // Move pointer out of header
> +        buf++;
> +    } else {
> +        av_log(avctx, AV_LOG_ERROR, "Incomplete file\n");
> +        return -1;
> +    }

Huh? I thought you had the "fixed" once already:
if (buf >= buf_end) {
    av_log();
    return -1;
}
buf++;

> +    if (s->red_channel != -1 &&
> +        s->blue_channel != -1 &&
> +        s->green_channel != -1) {
> +        if (s->picture.data[0])
> +            avctx->release_buffer(avctx, &s->picture);
> +        if (avcodec_check_dimensions(avctx, w, h))
> +            return -1;
> +        if (w != avctx->width || h != avctx->height) {
> +            // Verify the xmin, xmax, ymin, ymax and xdelta before setting the actual image size
> +            if (xmin > w || xmin == ~0 ||
> +                xmax > w || xmax == ~0 ||
> +                ymin > h || ymin == ~0 ||
> +                ymax > h || ymax == ~0 ||
> +                xdelta == ~0) {
> +                av_log(avctx, AV_LOG_ERROR, "Wrong sizing or missing size information\n");
> +                return -1;
> +            }
> +            avcodec_set_dimensions(avctx, w, h);
> +        }
> +
> +        if (avctx->get_buffer(avctx, p) < 0) {
> +            av_log(avctx, AV_LOG_ERROR, "get_buffer() failed\n");
> +            return -1;
> +        }
> +    } 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");
> +        return -1;
> +    }

As always, where possible do not indent the non-error code-path.