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

[Jimmy Christensen]

On 2009-09-08 20:23, Reimar D?ffinger wrote:
> On Tue, Sep 08, 2009 at 07:47:51PM +0200, Reimar D?ffinger wrote:
>> On Tue, Sep 08, 2009 at 03:30:26PM +0200, Jimmy Christensen wrote:
>>> +    switch (s->bits_per_color_id) {
>>> +    // 32-bit
>>> +    case 2:
>>> +        avctx->pix_fmt = PIX_FMT_RGB48LE;
>>> +        break;
>>> +    // 16-bit
>>> +    case 1:
>>> +        avctx->pix_fmt = PIX_FMT_RGB48LE;
>>> +        break;
>>
>> I missed that, this obviously can't be right, you write the uint16_t
>> values directly, thus in native format - not little-endian.
>
> I.e. PIX_FMT_RGB48
> Also it should of course not duplicate the code, i.e.
>> case 1: // 16-bit half-float input
>> case 2: // 32-bit float input
>>      avctx->pix_fmt = PIX_FMT_RGB48;
>>      break;
>
> And then I'd say the comments should be removed and instead proper
> enums/defines be used instead of 1, 2...
>

Fixed in rev17.

>>> +                    for (x = 0; x<  xdelta; x++) {
>>> +                        *ptr_x++ = exr_flt2uint(bytestream_get_le32(&red_channel_buffer));
>>> +                        *ptr_x++ = exr_flt2uint(bytestream_get_le32(&green_channel_buffer));
>>> +                        *ptr_x++ = exr_flt2uint(bytestream_get_le32(&blue_channel_buffer));
>>> +                    }
>>
>> Not that this must used code suitable for unaligned access, thus e.g. on
>
> Ouch. I meant:
> Note: bytestream_get_le32 must use code suitable for unaligned access.
>
>> Sparc
>> *ptr_x++ = exr_flt2uint(le2me_32(*(uint32_t *)red_channel_buffer));
>> will be faster.
>
> But I just realize that line_offset is in bytes, so you can't do that
> optimization.
>

Will leave the code how it is now. Someone with sparc should probably 
test this and submit a patch.

> Also in the same area some more comments:
>> +        uint16_t *ptr_x = (uint16_t*)ptr;
>
> Missing space before *
>

Fixed in rev17.

>> +        if (buf_end - buf>  8) {
>> +            const uint64_t line_offset = bytestream_get_le64(&buf) + 8;
>> +            // Check if the buffer has the required bytes needed from the offset
>> +            if ((uint32_t)(buf_end - avpkt->data)>= line_offset + xdelta * current_channel_offset) {
>
> Silently skipping the data is not very nice and user-friendly.
> Particularly since it will give very strange artifacts since you do not
> increase ptr this means a single bad line_offset will shift all of the
> remaining image one line up...

Made so that if the line_offset is above buffer_end it will generate a 
black line and moved increasing ptr with stride out of the if function.

> Also, what is that (uint32_t) cast supposed to do?
> It breaks files>  4GB on 64 bit systems without a good reason.

I once had a warning on compile time due to the other side of the 
comparison being uint32_t. No warning now though, so removed the typecast.

Thanks for the review.