[FFmpeg-devel] Patch for libavcodec/exr.c implementing B44 compression technique

Derek Buitenhuis derek.buitenhuis at gmail.com
Wed Mar 18 15:20:57 CET 2015


On 3/18/2015 2:08 PM, greeshma wrote:
> The .diff file is also attached henceforth.Please have a look.

This is still not a git-format patch as required and asked for
by Michael.

Also, the style and formatting is all over the place - that is,
there is no consistent style or formatting at all. Please fix
this so it is in line with our development style (see the
contributing document on ffmpeg.org).

Review follows.

> + * @file
> + * OpenEXR decoder
> + * Digital Ltd. LLC
> + * All rights reserved.
> + *
> + * For more information on the OpenEXR format, visit:
> + *  http://openexr.com/

@file only goes in a file once.

Also, if you wrote this code, this copyright is both incorrect
and makes it non-free.

> static
> +void unpack14 (const unsigned char b[14], unsigned short s[16])

Do not use unsigned char. Use uint8_t. This applies to all further uses.

> +{
> +    //
> +    // Unpack a 14-byte block into 4 by 4 16-bit pixels.
> +    //
> +    unsigned short shift,bias;

Do not use unsigned short. Use uint16_t. This applies to all further uses.

> +    #if defined (DEBUG)
> +        assert (b[2] != 0xfc);
> +    #endif

This ifdef is pointless.

Please use one of the av_assert functions.

> +static int b44_uncompress(EXRContext *s, const uint8_t *src,int compressed_size, int uncompressed_size,
> +EXRThreadData *td)
> +{
> +    unsigned long dest_len = uncompressed_size;

Do not use long, and especially do not out ints in a long. This is not portable.

> +    uint8_t *out;
> +    int i, j;
> +    //int c;

Don't leave in commented out code.

> +            memcpy (ptr, inPtr, n);
> +            inPtr += n;
> +            inSize -= n;

There should probably be a buf size check somewhere, no?

Same for the rest of the buf stuff.

> +                    int num = (x + 3 < s->xdelta)? 4 * sizeof (unsigned short) : (s->xdelta - x) * sizeof (unsigned short);

NIT: We usually use sizeof(*thing) instead of sizeof(type_of_thing).

- Derek


More information about the ffmpeg-devel mailing list