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

[Jimmy Christensen]

On 2009-09-08 16:20, Reimar D?ffinger wrote:
> 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.
>

Fixed in rev17.

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

Have added doygen documentation to all functions. And have removed 
inline from them.

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

I'm confused. It already does : 1 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".
>

Fixed in rev17.

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

Fixed in rev17.

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

Red is just the first one and I'm using that to determine the bit depth 
of the file. I will admit it's not the best solution since you in theory 
could have files which have a 16-bit red channel and 32-bit green and 
blue channels. However I don't see any reason for such things and I'm 
not so sure that any software will actually output files likes this.

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

Good idea. Will do that instead.

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

Fixed in rev17.

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

Fixed in rev17.

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

Fixed in rev17.

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

Is it?

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

Good point, fixed in rev17.

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

Must have gotten mixed up when I redid some of the code. Thanks. Fixed 
in rev17.

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

Fixed in rev17.

Thanks for the review. Will post rev17 when I've fixed the comments in 
the other posts.