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

[Jimmy Christensen]

On 2009-07-06 15:54, Reimar D?ffinger wrote:
> On Sun, Jul 05, 2009 at 10:30:16PM +0200, Jimmy Christensen wrote:
>> +    //int buf_end        = (buf + buf_size);
>
> ?

Hmmm.. missed a comment there. Will ofcourse remove it.


>
>> +    int red_channel        .= -1;
>
> I don't think that'll work with that . in there.
>

How did that get in there?

>> +        if (buf_end - buf>= 20&&  !strncmp(buf, "channels", 9)) {
>> +            buf += 9;
>> +            if (!strncmp(buf, "chlist", 7)) {
>> +                buf += 7;
>
> A function would probably make sense, e.g.
> static int compare_str(const uint8_t **buf, const uint8_t *buf_end, const char *str) {
>      int len = strlen(str) + 1;
>      if (buf_end - *buf<  len || strncmp(buf, str, len))
>          return 0;
>      *buf += len;
>      return 1;
> }
>

Yes, would make it a function when I was sure that the method was correct.

>> +                variable_buffer_data_size = bytestream_get_le32(&buf);
>> +                channel_iter = -1;
>> +                if((buf + variable_buffer_data_size)<  buf_end) {
>
> buf + variable_buffer_data_size can overflow.
> The unsafe data (e.g. the thing you read from the file and haven't
> verified) should always stand alone.
> Also the () is useless.
>

Will try to make it better.

>> +                    const uint8_t *ptr_tmp = buf;
>> +                    if((buf_end - buf)>  variable_buffer_data_size) {
>
> What's supposed to be the difference between this check and the one
> above??
> Also you should decide on which part to put right and which left, not
> change it around IMO.

You're right. Will change it.

>
>> +                    while(ptr_tmp + 1<  (buf)) {
>> +                        channel_iter++;
>> +                        if((buf - ptr_tmp)>= 19&&  !strncmp(ptr_tmp, "R", 2)) {
>> +                            ptr_tmp += 2;
>> +                            red_channel = channel_iter;
>> +                            bits_per_color_id = AV_RL32(ptr_tmp);
>> +                            ptr_tmp += 16;
>> +                            continue;
>> +                        }
>> +
>> +                        if((buf - ptr_tmp)>= 19&&  !strncmp(ptr_tmp, "G", 2)) {
>> +                            ptr_tmp += 2;
>> +                            green_channel = channel_iter;
>> +                            ptr_tmp += 16;
>> +                            continue;
>> +                        }
>> +
>> +                        if((buf - ptr_tmp)>= 19&&  !strncmp(ptr_tmp, "B", 2)) {
>> +                            ptr_tmp += 2;
>> +                            blue_channel = channel_iter;
>> +                            ptr_tmp += 16;
>> +                            continue;
>> +                        }
>> +
>> +                        // Process other channels
>> +                        if(ptr_tmp<  buf) {
>> +                            while(ptr_tmp[0] != 0x0) {
>> +                                ptr_tmp++;
>> +                            }
>> +                            ptr_tmp += 17;
>> +                        }
>
> Both inside this loop as well as in the outer loop you can't handle the
> "else" case this way. If someone stores the data in the order "B", "G",
> "R", your "process other channels" part will throw away the "G".
>
No, and I know this since the files I use are actually stored as "B", 
"G", "R". Remember there is a continue inside each if. I know it's ugly.

>> +        // Process unknown variables
>> +        if((buf_end - buf)>  9) {
>> +            // Skip variable name
>> +            while(buf[0] != 0x0&&  buf<  buf_end) {
>
> If buf[0] is 0, then that indicates the end of the header, it's not an
> unknown variable.
> Also first accessing the data and the checking if it is in bounds makes
> little sense, even though due to padding it probably won't cause issues.
>

Will make it inside another if.

>> +            // Skip variable type
>> +            while(buf[0] != 0x0&&  buf<  buf_end) {
>> +                buf++;
>> +            }
>> +            buf++;
>
> Also it can be shortened to
> while (buf<  buf_end&&  *buf++) /**/;
>

Thanks. Will change that.

>
>> +                variable_buffer_data_size = bytestream_get_le32(&buf);
>> +                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;
>> +                }
>
> That's at least the second place where I see this code. A function might
> be a good idea.
>

See above comment.

>> +                uint16_t* ptr_x = (uint16_t*)ptr;
>
> The * is placed inconsistently.

Will change it.

>
>> +                const uint32_t line_offset = AV_RL64(buf);
>
> bytestream_get_le64

Will use that instead.

>
>> +                const uint8_t *red_channel_buffer   = avpkt->data + line_offset + 8 + (xdelta)*4*red_channel;
>> +                const uint8_t *green_channel_buffer = avpkt->data + line_offset + 8 + (xdelta)*4*green_channel;
>> +                const uint8_t *blue_channel_buffer  = avpkt->data + line_offset + 8 + (xdelta)*4*blue_channel;
>
> You aren't checking that these actually point anywhere valid.
> Try adding setting "line_offset = 0x4fffffff;" and you should see what I
> mean.

In this revision I focused more on the header parser. Will ofcourse make 
more checks here.

>
>> +                    *ptr_x++ = exr_flt2uint(AV_RL32(red_channel_buffer));
>> +                    red_channel_buffer += 4;
>> +                    *ptr_x++ = exr_flt2uint(AV_RL32(green_channel_buffer));
>> +                    green_channel_buffer += 4;
>> +                    *ptr_x++ = exr_flt2uint(AV_RL32(blue_channel_buffer));
>> +                    blue_channel_buffer += 4;
>
> bytestream_get_le32 for all of these.

Will use that instead.

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

True, was a copy from earlier in the code so didn't think about that.

> Incrementing ptr_x is pointless.
>
>> +        // 16-bit
>> +        case 1:
>
> basically the same comments.
>

-- 
Best Regards
Jimmy Christensen
Developer
Ghost A/S