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

Reimar Döffinger Reimar.Doeffinger
Wed Jul 29 13:54:12 CEST 2009


On Wed, Jul 29, 2009 at 07:50:47AM +0200, Jimmy Christensen wrote:
> On 2009-07-29 00:46, Michael Niedermayer wrote:
>> On Tue, Jul 21, 2009 at 08:45:10PM +0200, Jimmy Christensen wrote:
>>> New patch. Re-arranged some small things. Now supports variable channel
>>> size for all other than the R, G and B channels which are expected to be of
>>> the same size.
>>>
>>> Also added a few more checks for possible buffer overruns.
>> [...
>>> +static inline uint16_t exr_flt2uint(uint32_t v)
>>> +{
>>> +    int exp = v>>  23;
>>
>>> +    if (v&  0x80000000)
>>> +        return 0;
>>> +    if (exp<= 127+7-24) // we would shift out all bits anyway
>>> +        return 0;
>>
>> if v where signed then the first if would be unneded
>>
>
> Not sure it would yield the expected result. If it's negative it needs  
> to output 0 and not a positive number.

If v is signed, exp will be < for negative numbers. So yes it would work.

>>> +                        channel_iter++;
>>> +                        if (buf - ptr_tmp>= 19&&  !strncmp(ptr_tmp, "R", 2)) {
>>> +                            ptr_tmp += 2;
>>> +                            red_channel = channel_iter;
>>> +                            current_bits_per_color_id = bytestream_get_le32(&ptr_tmp);
>>> +                            if (current_bits_per_color_id>  2) {
>>> +                                av_log(avctx, AV_LOG_ERROR, "Unkown color format : %d\n", bits_per_color_id);
>>> +                                return -1;
>>> +                            }
>>> +                            red_channel_offset = current_channel_offset;
>>> +                            current_channel_offset += bytes_per_color_table[current_bits_per_color_id];
>>> +                            bits_per_color_id = current_bits_per_color_id;
>>> +                            ptr_tmp += 12;
>>> +                            continue;
>>> +                        }
>>> +                        if (buf - ptr_tmp>= 19&&  !strncmp(ptr_tmp, "G", 2)) {
>>> +                            ptr_tmp += 2;
>>> +                            green_channel = channel_iter;
>>> +                            current_bits_per_color_id = bytestream_get_le32(&ptr_tmp);
>>> +                            if (current_bits_per_color_id>  2) {
>>> +                                av_log(avctx, AV_LOG_ERROR, "Unkown color format : %d\n", bits_per_color_id);
>>> +                                return -1;
>>> +                            }
>>> +                            green_channel_offset = current_channel_offset;
>>> +                            current_channel_offset += bytes_per_color_table[current_bits_per_color_id];
>>> +                            ptr_tmp += 12;
>>> +                            continue;
>>> +                        }
>>
>> code duplication
>>
>
> Could probably make it less duplicate code, but these steps needs to be  
> done only for R, G and B and needs to assign specific variables 
> accordingly.

That's what functions are there for. I think in quite a few places readability
could be improved quite a bit by creating a well named function, but I think
you so far ignored all my suggestions in that regard.

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

Pointless else, that was basically the whole point why I suggested changing
the order of this.
If you are more used to languages like Java etc., think of this as a poor-man's
exception.

> +                if (buf_end - buf <= variable_buffer_data_size) {
> +                    av_log(avctx, AV_LOG_ERROR, "Incomplete channel list\n");
> +                    return -1;
> +                } else {

Same.

> +                for (int i = 0; i < 2; i++) {

That will break gcc 2.95 compilation. Declare i normally.

> +                    if ((buf_end - buf) > variable_buffer_data_size) {
> +                        buf += variable_buffer_data_size;
> +                    } else {
> +                        av_log(avctx, AV_LOG_ERROR, "Incomplete header\n");
> +                        return -1;
> +                    }

Pointless () around buf_end - buf, also I still think this check is duplicated
all over the place and could be factored out at worst by using a macro.

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

As in the other code, this is more compact and IMO readable by packing the
execptional case into the lower indentation level, i.e.

> +    if (buf >= buf_end) {
> +        av_log(avctx, AV_LOG_ERROR, "Incomplete file\n");
> +        return -1;
> +    }
> +    buf++;

But since the input buffer is guaranteed to be padded by a few extra bytes, I think
this check is not really necessary at all.

> +        // 32-bit
> +        case 2:
> +            // Process the actual lines
> +            for (y = ymin; y <= ymax; y++) {
> +                uint16_t *ptr_x = (uint16_t*)ptr;
> +                if (buf_end - buf > 8) {
> +                    const uint32_t line_offset = bytestream_get_le64(&buf) + 8;
> +                    // Check if the buffer has the required bytes needed from the offset
> +                    if ((uint32_t)(buf_end - avpkt->data) >= line_offset + (xdelta) * current_channel_offset) {
> +                        const uint8_t *red_channel_buffer   = avpkt->data + line_offset + (xdelta) * red_channel_offset;
> +                        const uint8_t *green_channel_buffer = avpkt->data + line_offset + (xdelta) * green_channel_offset;
> +                        const uint8_t *blue_channel_buffer  = avpkt->data + line_offset + (xdelta) * blue_channel_offset;
> +
> +                        // Zero out the start if xmin is not 0
> +                        memset(ptr_x, 0, xmin*6);
> +                        ptr_x += xmin*3;
> +
> +                        for (x = 0; x < xdelta; x++) {
> +                            *ptr_x++ = exr_flt2uint(bytestream_get_le32(&red_channel_buffer));
> +                            *ptr_x++ = exr_flt2uint(bytestream_get_le32(&green_channel_buffer));
> +                            *ptr_x++ = exr_flt2uint(bytestream_get_le32(&blue_channel_buffer));
> +                        }
> +
> +                        // Zero out the end if xmax+1 is not w
> +                        memset(ptr_x, 0, (avctx->width - (xmax+1))*6);
> +                        ptr_x += (avctx->width - (xmax+1))*3;
> +
> +                        // Move to next line
> +                        ptr += stride;
> +                    }
> +                }
> +            }
> +            break;
> +        // 16-bit
> +        case 1:
> +            // Process the actual lines
> +            for (y = ymin; y <= ymax; y++) {
> +                uint16_t *ptr_x = (uint16_t*)ptr;
> +                if (buf_end - buf > 8) {
> +                    const uint32_t line_offset = bytestream_get_le64(&buf) + 8;
> +                    // Check if the buffer has the required bytes needed from the offset
> +                    if ((uint32_t)(buf_end - avpkt->data) >= line_offset + (xdelta) * current_channel_offset) {
> +                        const uint8_t *red_channel_buffer   = avpkt->data + line_offset + (xdelta) * red_channel_offset;
> +                        const uint8_t *green_channel_buffer = avpkt->data + line_offset + (xdelta) * green_channel_offset;
> +                        const uint8_t *blue_channel_buffer  = avpkt->data + line_offset + (xdelta) * blue_channel_offset;
> +
> +                        // Zero out the start if xmin is not 0
> +                        memset(ptr_x, 0, xmin*6);
> +                        ptr_x += xmin*3;
> +
> +                        for (x = 0; x < xdelta; x++) {
> +                            *ptr_x++ = exr_halflt2uint(bytestream_get_le16(&red_channel_buffer));
> +                            *ptr_x++ = exr_halflt2uint(bytestream_get_le16(&green_channel_buffer));
> +                            *ptr_x++ = exr_halflt2uint(bytestream_get_le16(&blue_channel_buffer));
> +                        }
> +
> +                        // Zero out the end if xmax+1 is not w
> +                        memset(ptr_x, 0, (avctx->width - (xmax+1))*6);
> +                        ptr_x += (avctx->width - (xmax+1))*3;
> +
> +                        // Move to next line
> +                        ptr += stride;
> +                    }
> +                }
> +            }
> +            break;

These two are almost the same except for a factor 2 in some formulas and the innermost loop.
I think the performance impact of merging these into one piece of code should be small enough
that it would be reasonable to avoid the duplication...




More information about the ffmpeg-devel mailing list