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

Jimmy Christensen jimmy
Wed Jul 15 15:04:53 CEST 2009


On 2009-07-15 14:00, Reimar D?ffinger wrote:
> 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.
>

I misunderstood your comment, but you're absolutely right. Will change 
that. Thanks.

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

What I meant was an extra variable and ofcourse the check. The current 
code expects all channels to be of the same size and will look in the 
wrong place if there is an alpha channel of 8-bit before the 16-bit R, G 
and B channels.



More information about the ffmpeg-devel mailing list