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

[Reimar Döffinger]

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

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

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

Missing space before *

> +        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...
Also, what is that (uint32_t) cast supposed to do?
It breaks files > 4GB on 64 bit systems without a good reason.