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

Stefano Sabatini stefano.sabatini-lala
Sat Apr 24 21:04:24 CEST 2010


On date Saturday 2010-04-24 17:59:46 +0200, Jimmy Christensen encoded:
[...]
> Index: libavcodec/exr.c
> ===================================================================
> --- libavcodec/exr.c	(revision 0)
> +++ libavcodec/exr.c	(revision 0)
> @@ -0,0 +1,483 @@
> +/*
> + * OpenEXR (.exr) image decoder

This is redundant (already in the @file doxy).

> + * Copyright (c) 2009 Jimmy Christensen
> + *
> + * This file is part of FFmpeg.
> + *
> + * FFmpeg is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License as published by the Free Software Foundation; either
> + * version 2.1 of the License, or (at your option) any later version.
> + *
> + * FFmpeg is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with FFmpeg; if not, write to the Free Software
> + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
> + */
> +
> +/**
> + * @file libavcodec/exr.c

"@file" alone should be enough

> + * OpenEXR decoder
> + * @author Jimmy Christensen
> + *
> + * For more information on the OpenEXR format, visit:
> + *  http://openexr.com/
> + *
> + * exr_flt2uint() and exr_halflt2uint() is credited to  Reimar D??ffinger

D??ffinger => unicode issue?

> + */
> +
> +#include "avcodec.h"
> +#include "bytestream.h"
> +
> +enum ExrCompr {
> +    EXR_RAW   = 0,
> +    EXR_RLE   = 1,
> +    EXR_ZIP1  = 2,
> +    EXR_ZIP16 = 3,
> +    EXR_PIZ   = 4,
> +    EXR_B44   = 6
> +};
> +
> +typedef struct EXRContext {
> +    AVFrame picture;
> +    int compr;
> +    int bits_per_color_id;
> +    int8_t channel_offsets[3]; // 0 = red, 1 = green and 2 = blue
> +} EXRContext;
> +
> +/**
> + * Convert from 32-bit float as uint32_t to uint16_t

"Converts" - please use third person here and below for consistency
sake.

Also add an empty newline between description and @params, here and
below, improve readability.

> + * @param v 32-bit float
> + * @return normalized 16-bit unsigned int
> + */
> +static inline uint16_t exr_flt2uint(uint32_t v)
> +{
> +    unsigned int exp = v >> 23;
> +    // "HACK": negative values result in exp<  0, so clipping them to 0
> +    // is also handled by this condition, avoids explicit check for sign bit.
> +    if (exp<= 127 + 7 - 24) // we would shift out all bits anyway
> +        return 0;
> +    if (exp >= 127)
> +        return 0xffff;
> +    v &= 0x007fffff;
> +    return (v + (1 << 23)) >> (127 + 7 - exp);
> +}
> +
> +/**
> + * Convert from 16-bit float as uint16_t to uint16_t
> + * @param v 16-bit float
> + * @return normalized 16-bit unsigned int
> + */
> +static inline uint16_t exr_halflt2uint(uint16_t v)
> +{
> +    int exp = v >> 10;
> +    if (v & 0x8000)
> +        return 0;
> +    if (!exp)
> +        return (v >> 9) & 1;
> +    if (exp >= 15)
> +        return 0xffff;
> +    v <<= 6;
> +    return (v + (1 << 16)) >> (15 - exp);
> +}
> +
> +/**
> + * Gets the size of the header variable
> + * @param **buf the current pointer location in the header where
> + * the variable data starts
> + * @param *buf_end pointer location of the end of the buffer
> + * @return size of variable data
> + */
> +static unsigned int get_header_variable_length(const uint8_t **buf,
> +                                               const uint8_t *buf_end)
> +{
> +    unsigned int variable_buffer_data_size = bytestream_get_le32(buf);
> +    if (variable_buffer_data_size >= buf_end - *buf)
> +        return 0;
> +    return variable_buffer_data_size;
> +}
> +
> +/**
> + * Checks if the variable name corresponds with it's data type
> + * @param *avctx the AVCodecContext
> + * @param **buf the current pointer location in the header where
> + * the variable name starts
> + * @param *buf_end pointer location of the end of the buffer
> + * @param *value_name name of the varible to check
> + * @param *value_type type of the varible to check
> + * @param minimum_length minimum length of the variable data
> + * @param variable_buffer_data_size variable length read from the header
> + * after it's checked
> + * @return zero if variable is invalid and 1 if good
> + */
> +static unsigned int check_header_variable(AVCodecContext *avctx,
> +                                              const uint8_t **buf,
> +                                              const uint8_t *buf_end,
> +                                              const char *value_name,
> +                                              const char *value_type,
> +                                              unsigned int minimum_length,
> +                                              unsigned int *variable_buffer_data_size)
> +{
> +    if (buf_end - *buf >= minimum_length && !strcmp(*buf, value_name)) {
> +        *buf += strlen(value_name)+1;
> +        if (!strcmp(*buf, value_type)) {
> +            *buf += strlen(value_type)+1;
> +            *variable_buffer_data_size = get_header_variable_length(buf, buf_end);
> +            if (!*variable_buffer_data_size)
> +                av_log(avctx, AV_LOG_ERROR, "Incomplete header\n");
> +            return 1;

AVERROR_INVALIDDATA;

> +        }
> +        *buf -= strlen(value_name)+1;
> +        av_log(avctx, AV_LOG_ERROR, "Unknown data type for header variable %s\n", value_name);
> +    }
> +    return 0;
> +}
> +
> +static int decode_frame(AVCodecContext *avctx,
> +                        void *data,
> +                        int *data_size,
> +                        AVPacket *avpkt)
> +{
> +    const uint8_t *buf      = avpkt->data;
> +    unsigned 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, version_flag;
> +    int w = 0;
> +    int h = 0;
> +    unsigned int xmin   = ~0;
> +    unsigned int xmax   = ~0;
> +    unsigned int ymin   = ~0;
> +    unsigned int ymax   = ~0;
> +    unsigned int xdelta = ~0;
> +
> +    unsigned int current_channel_offset = 0;
> +
> +    s->channel_offsets[0] = -1;
> +    s->channel_offsets[1] = -1;
> +    s->channel_offsets[2] = -1;
> +    s->bits_per_color_id = -1;
> +

> +    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;
> +    }
> +
> +    version_flag = bytestream_get_le32(&buf);
> +    if ((version_flag & 0x200) == 0x200) {
> +        av_log(avctx, AV_LOG_ERROR, "Tile based images are not supported\n");
> +        return -1;
> +    }
> +
> +    if (buf_end - buf < 10) {
> +        av_log(avctx, AV_LOG_ERROR, "Too short header to parse\n");
> +        return -1;

Please use meaningful AVERROR_ codes here and in the rest of the file.

[...]

Regards.
-- 
FFmpeg = Free and Fiendish Most Ponderous Evangelical Guru



More information about the ffmpeg-devel mailing list