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

Michael Niedermayer michaelni
Wed Jul 29 00:46:41 CEST 2009


On Tue, Jul 21, 2009 at 08:45:10PM +0200, Jimmy Christensen wrote:
> New patch. Re-arranged some small things. Now supports variable channel 
> size for all other than the R, G and B channels which are expected to be of 
> the same size.
>
> Also added a few more checks for possible buffer overruns.
[...
> +static inline uint16_t exr_flt2uint(uint32_t v)
> +{
> +    int exp = v >> 23;

> +    if (v & 0x80000000)
> +        return 0;
> +    if (exp <= 127+7-24) // we would shift out all bits anyway
> +        return 0;

if v where signed then the first if would be unneded


> +    if (exp >= 127)
> +        return 0xffff;
> +    v &= 0x007fffff;
> +        return (v+(1<<23)) >> (127+7-exp);
> +}
[...]
> +static int decode_frame(AVCodecContext *avctx,
> +                        void *data,
> +                        int *data_size,
> +                        AVPacket *avpkt)
> +{
> +    const uint8_t *buf = avpkt->data;
> +    int buf_size       = avpkt->size;
> +    const uint8_t *buf_end = buf + buf_size;
> +
> +    EXRContext *const s = avctx->priv_data;
> +    AVFrame *picture  = data;
> +    AVFrame *const p = &s->picture;
> +    uint8_t *ptr;
> +
> +    int x, y, stride, magic_number;
> +    int w = 0;
> +    int h = 0;
> +    int xmin   = -1;
> +    int xmax   = -1;
> +    int ymin   = -1;
> +    int ymax   = -1;
> +    int xdelta = -1;
> +
> +    int red_channel            = -1;
> +    int green_channel          = -1;
> +    int blue_channel           = -1;
> +    int bits_per_color_id      = -1;

> +    int red_channel_offset     = 0;
> +    int green_channel_offset   = 0;
> +    int blue_channel_offset    = 0;
> +    int current_channel_offset = 0;

nitpick:
   int   red_channel_offset   = 0;
   int green_channel_offset   = 0;
   int  blue_channel_offset   = 0;


> +
> +    static int bytes_per_color_table[3] = {1, 2, 4};

missing const


> +
> +    magic_number = bytestream_get_le32(&buf);
> +    if (magic_number != 20000630) { // As per documentation of OpenEXR it's supposed to be int 20000630 little-endian
> +        av_log(avctx, AV_LOG_ERROR, "Wrong magic number %d\n", magic_number);
> +        return -1;
> +    }
> +
> +    // Move to header start
> +    buf += 4;
> +

> +    if (buf_end - buf < 10) {
> +            av_log(avctx, AV_LOG_ERROR, "Too short header to parse\n");
> +            return -1;
> +        } else {
> +        // Parse the header
> +        while (buf[0] != 0x0) {

there is somethig wrong with the indention


> +            int variable_buffer_data_size;
> +            // Process the channel list
> +            if (buf_end - buf >= 20 && !strncmp(buf, "channels", 9)) {
> +                buf += 9;
> +                if (strncmp(buf, "chlist", 7)) {
> +                    av_log(avctx, AV_LOG_ERROR, "Unknown type for channels\n");
> +                    return -1;
> +                }
> +                buf += 7;
> +                variable_buffer_data_size = bytestream_get_le32(&buf);
> +                if (buf_end - buf <= variable_buffer_data_size) {
> +                    av_log(avctx, AV_LOG_ERROR, "Incomplete channel list\n");
> +                    return -1;

have you considered negative variable_buffer_data_size ?


> +                } else {
> +                    const uint8_t *ptr_tmp = buf;
> +                    int channel_iter = -1;
> +                    int current_bits_per_color_id = 0;
> +                    buf += variable_buffer_data_size;
> +                    while (ptr_tmp + 1 < buf) {

i think readability woud be better if there was an empty line in there
somewhere


> +                        channel_iter++;
> +                        if (buf - ptr_tmp >= 19 && !strncmp(ptr_tmp, "R", 2)) {
> +                            ptr_tmp += 2;
> +                            red_channel = channel_iter;
> +                            current_bits_per_color_id = bytestream_get_le32(&ptr_tmp);
> +                            if (current_bits_per_color_id > 2) {
> +                                av_log(avctx, AV_LOG_ERROR, "Unkown color format : %d\n", bits_per_color_id);
> +                                return -1;
> +                            }
> +                            red_channel_offset = current_channel_offset;
> +                            current_channel_offset += bytes_per_color_table[current_bits_per_color_id];
> +                            bits_per_color_id = current_bits_per_color_id;
> +                            ptr_tmp += 12;
> +                            continue;
> +                        }
> +                        if (buf - ptr_tmp >= 19 && !strncmp(ptr_tmp, "G", 2)) {
> +                            ptr_tmp += 2;
> +                            green_channel = channel_iter;
> +                            current_bits_per_color_id = bytestream_get_le32(&ptr_tmp);
> +                            if (current_bits_per_color_id > 2) {
> +                                av_log(avctx, AV_LOG_ERROR, "Unkown color format : %d\n", bits_per_color_id);
> +                                return -1;
> +                            }
> +                            green_channel_offset = current_channel_offset;
> +                            current_channel_offset += bytes_per_color_table[current_bits_per_color_id];
> +                            ptr_tmp += 12;
> +                            continue;
> +                        }

code duplication

[...]
> +                // Skip variable name
> +                while (buf++ < buf_end) {
> +                    if (buf[0] == 0x0)
> +                        break;
> +                }
> +                // Skip variable type
> +                while (buf++ < buf_end) {
> +                    if (buf[0] == 0x0)
> +                        break;
> +                }

code duplication

[...]
> +    if (red_channel != -1 && blue_channel != -1 && green_channel != -1) {
> +        if (s->picture.data[0])
> +            avctx->release_buffer(avctx, &s->picture);
> +        if (avcodec_check_dimensions(avctx, w, h))
> +            return -1;
> +        if (w != avctx->width || h != avctx->height) {
> +            // Verify the xmin, xmax, ymin, ymax and xdelta before setting the actual image size

> +            if (xmin > w || xmin < 0 || xmax > w || xmax < 0 || ymin > h || ymin < 0 || ymax > h || ymax < 0 || xdelta < 0) {
> +                av_log(avctx, AV_LOG_ERROR, "Wrong sizing or missing size information\n");
> +                return -1;
> +            }

it looks like these variables should be unsigned

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

When the tyrant has disposed of foreign enemies by conquest or treaty, and
there is nothing more to fear from them, then he is always stirring up
some war or other, in order that the people may require a leader. -- Plato
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20090729/481f9649/attachment.pgp>



More information about the ffmpeg-devel mailing list