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

Jimmy Christensen jimmy
Wed Jul 29 07:50:47 CEST 2009


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 (exp>= 127)
>> +        return 0xffff;
>> +    v&= 0x007fffff;
>> +        return (v+(1<<23))>>  (127+7-exp);
>> +}
> [...]
>> +static int decode_frame(AVCodecContext *avctx,
>> +                        void *data,
>> +                        int *data_size,
>> +                        AVPacket *avpkt)
>> +{
>> +    const uint8_t *buf = avpkt->data;
>> +    int buf_size       = avpkt->size;
>> +    const uint8_t *buf_end = buf + buf_size;
>> +
>> +    EXRContext *const s = avctx->priv_data;
>> +    AVFrame *picture  = data;
>> +    AVFrame *const p =&s->picture;
>> +    uint8_t *ptr;
>> +
>> +    int x, y, stride, magic_number;
>> +    int w = 0;
>> +    int h = 0;
>> +    int xmin   = -1;
>> +    int xmax   = -1;
>> +    int ymin   = -1;
>> +    int ymax   = -1;
>> +    int xdelta = -1;
>> +
>> +    int red_channel            = -1;
>> +    int green_channel          = -1;
>> +    int blue_channel           = -1;
>> +    int bits_per_color_id      = -1;
>
>> +    int red_channel_offset     = 0;
>> +    int green_channel_offset   = 0;
>> +    int blue_channel_offset    = 0;
>> +    int current_channel_offset = 0;
>
> nitpick:
>     int   red_channel_offset   = 0;
>     int green_channel_offset   = 0;
>     int  blue_channel_offset   = 0;
>
>

Will fix that.

>> +
>> +    static int bytes_per_color_table[3] = {1, 2, 4};
>
> missing const
>
>

Will fix that.

>> +
>> +    magic_number = bytestream_get_le32(&buf);
>> +    if (magic_number != 20000630) { // As per documentation of OpenEXR it's supposed to be int 20000630 little-endian
>> +        av_log(avctx, AV_LOG_ERROR, "Wrong magic number %d\n", magic_number);
>> +        return -1;
>> +    }
>> +
>> +    // Move to header start
>> +    buf += 4;
>> +
>
>> +    if (buf_end - buf<  10) {
>> +            av_log(avctx, AV_LOG_ERROR, "Too short header to parse\n");
>> +            return -1;
>> +        } else {
>> +        // Parse the header
>> +        while (buf[0] != 0x0) {
>
> there is somethig wrong with the indention
>
>

Hmmmm... didn't see that. Wonder how it got there.

>> +            int variable_buffer_data_size;
>> +            // Process the channel list
>> +            if (buf_end - buf>= 20&&  !strncmp(buf, "channels", 9)) {
>> +                buf += 9;
>> +                if (strncmp(buf, "chlist", 7)) {
>> +                    av_log(avctx, AV_LOG_ERROR, "Unknown type for channels\n");
>> +                    return -1;
>> +                }
>> +                buf += 7;
>> +                variable_buffer_data_size = bytestream_get_le32(&buf);
>> +                if (buf_end - buf<= variable_buffer_data_size) {
>> +                    av_log(avctx, AV_LOG_ERROR, "Incomplete channel list\n");
>> +                    return -1;
>
> have you considered negative variable_buffer_data_size ?
>
>

You're right it should be unsigned.

>> +                } else {
>> +                    const uint8_t *ptr_tmp = buf;
>> +                    int channel_iter = -1;
>> +                    int current_bits_per_color_id = 0;
>> +                    buf += variable_buffer_data_size;
>> +                    while (ptr_tmp + 1<  buf) {
>
> i think readability woud be better if there was an empty line in there
> somewhere
>
>

Added that now.

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

> [...]
>> +                // Skip variable name
>> +                while (buf++<  buf_end) {
>> +                    if (buf[0] == 0x0)
>> +                        break;
>> +                }
>> +                // Skip variable type
>> +                while (buf++<  buf_end) {
>> +                    if (buf[0] == 0x0)
>> +                        break;
>> +                }
>
> code duplication
>

Can add a for loop in front, but it needs to be done exactly twice. And 
without a loop, it's easier to comment which skips what.

> [...]
>> +    if (red_channel != -1&&  blue_channel != -1&&  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;
>> +            }
>
> it looks like these variables should be unsigned
>

You're right. Changed it back.

Attached new patch
-------------- next part --------------
A non-text attachment was scrubbed...
Name: OpenEXR-rev12.diff
Type: text/x-patch
Size: 22253 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20090729/cbcdb991/attachment.bin>



More information about the ffmpeg-devel mailing list