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

[Reimar Döffinger]

On Wed, Jul 15, 2009 at 01:05:31PM +0200, Jimmy Christensen wrote:
> On 2009-07-15 10:22, Reimar D?ffinger wrote:
>>> +                        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 = bytestream_get_le32(&ptr_tmp);
>>> +                                ptr_tmp += 16;
>>> +                                continue;
>>> +                            }
>>
>> As you pointed out to me, you do have the continues here because they are necessary.
>> But you seemed to forget to add them to the checks in the outer loop? Both are
>> exactly the same cases...
>
> Hmm.. if you mean if I could replace "ptr_tmp + 1 <  buf" with "buf -  
> ptr_tmp >= 19" it won't work like intended. Channels can be called  
> something more than 1 character. Ofcourse you can argue that a channel  
> does have to have atleast 1 character. So you are right that it can be  
> replaced with "buf - ptr_tmp >= 19".

Huh? I am talking about the continue statements and nothing else.
In the inner loop you have
if (!strcmp(...)) {
    ...
    continue;
}

Because as you realized otherwise it won't work if the variable are in
a different order.
But exactly the same issue applies to the outer loop, e.g. if displayWindow
came before dataWindow.

>>> +                    // Check if the buffer has the required bytes needed from the offset
>>> +                    if ((uint32_t)(buf_end - avpkt->data)>= line_offset + (8 + (channel_iter + 1) * 4 * xdelta)) {
>>> +                        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;
>>
>> First, the way you use channel_iter here is not at all obvious.
>> I'd suggest FFMAX3(red_channel, green_channel, blue_channel)
>> Secondly, changing the order and the placement of () in the check compared to the
>> later calculations makes it needlessly hard to find out how they are related.
>> Thirdly, why the cast to uint32_t? You have to avoid an overflow in the calculation,
>> so if anything you should cast the right side to some 64 bit value.
>> And also what's with the () around (xdelta)?
>
> Hmmm... you're right, After I thought about it I realized that it  
> doesn't matter which channels are after the red, green and blue  
> channels. channel_iter was to indicate how large the line for the  
> channels would be. Eg. if there are 8 channels, the size of the line  
> would be 8 * channel size * xdelta. Even that is not sufficient since  
> channels can be of different sizes, so will need to create a variable in  
> the header parser indicating the total sizes of the channels.

No you certainly do not need an extra variable, that's actually a sure way
you get a broken check. You need to make sure any data you try access is
inside the buffer, you necessarily _must_ have all information necessary
to check this right at the place where the access is done because otherwise
the CPU wouldn't have enough information to calculate the address either.