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

Reimar Döffinger Reimar.Doeffinger
Wed Jul 15 14:00:38 CEST 2009


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.



More information about the ffmpeg-devel mailing list