[libav-devel] [PATCH] lavc: add OpenEXR decoder

Diego Biurrun diego at biurrun.de
Thu Mar 13 17:44:16 CET 2014


On Mon, Mar 10, 2014 at 09:28:53PM +0100, Vittorio Giovara wrote:
> --- /dev/null
> +++ b/libavcodec/exr.c
> @@ -0,0 +1,1346 @@
> +/**
> + * Get 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

Please vertically align the parameter descriptions.

> +/**
> + * Checks if the variable name corresponds with it's data type

Check if the variable name corresponds to its data type.

> +static int rle_uncompress(const uint8_t *src, int compressed_size,
> +                          int uncompressed_size, EXRThreadData *td)
> +{
> +    int8_t *d       = (int8_t *) td->tmp;
> +    const int8_t *s = (const int8_t *) src;

I wonder why you use signed types here.

> +#define SHORT_ZEROCODE_RUN  59
> +#define LONG_ZEROCODE_RUN   63
> +#define SHORTEST_LONG_RUN   (2 + LONG_ZEROCODE_RUN - SHORT_ZEROCODE_RUN)
> +#define LONGEST_LONG_RUN    (255 + SHORTEST_LONG_RUN)
> +
> +static int huf_unpack_enc_table(GetByteContext *gb,
> +                                int32_t im, int32_t iM, uint64_t *hcode)
> +{
> +    GetBitContext gbit;
> +
> +    init_get_bits8(&gbit, gb->buffer, bytestream2_get_bytes_left(gb));
> +
> +    for (; im <= iM; im++) {
> +        uint64_t l = hcode[im] = get_bits(&gbit, 6);
> +
> +        if (l == LONG_ZEROCODE_RUN) {
> +            int zerun = get_bits(&gbit, 8) + SHORTEST_LONG_RUN;
> +
> +            if (im + zerun > iM + 1)
> +                return AVERROR_INVALIDDATA;
> +
> +            while (zerun--)
> +                hcode[im++] = 0;
> +
> +            im--;
> +        } else if (l >= (uint64_t) SHORT_ZEROCODE_RUN) {

Do you cast here to avoid a warning?  Declaring the constant unsigned
would be cleaner IMO.

> +            pl->p = av_realloc(pl->p, pl->lit * sizeof(int));

sizeof(pl_lit) maybe?

> +static int piz_uncompress(EXRContext *s, const uint8_t *src, int ssize,
> +                          int dsize, EXRThreadData *td)
> +{
> +    GetByteContext gb;
> +    uint16_t maxval, min_non_zero, max_non_zero;
> +    uint16_t *ptr, *tmp = (uint16_t *) td->tmp;
> +    int8_t *out;
> +    int ret, i, j;
> +
> +    if (!td->bitmap)
> +        td->bitmap = av_malloc(BITMAP_SIZE);
> +    if (!td->lut)
> +        td->lut = av_malloc(1 << 17);
> +    if (!td->bitmap || !td->lut)
> +        return AVERROR(ENOMEM);

Uh, if the first malloc succeeds, but the second fails, you leak memory.

> +    ret = huf_uncompress(&gb, tmp, dsize / sizeof(int16_t));

sizeof(type) is ugly here, but I'm not sure what it should be replaced by.

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

nit: excessive alignment

> +    if (uncompress(td->tmp, &dest_len, src, compressed_size) != Z_OK ||
> +        dest_len != uncompressed_size)
> +        return AVERROR_INVALIDDATA;
> +
> +    out = td->uncompressed_data;
> +    for (i = 0; i < s->ysize; i++)
> +        for (c = 0; c < s->nb_channels; c++) {
> +            EXRChannel *channel = &s->channels[c];
> +            const uint8_t *ptr[4];
> +            uint32_t pixel = 0;
> +
> +            switch (channel->pixel_type) {
> +            case EXR_FLOAT:
> +                ptr[0] = in;
> +                ptr[1] = ptr[0] + s->xdelta;
> +                ptr[2] = ptr[1] + s->xdelta;
> +                in     = ptr[2] + s->xdelta;
> +
> +                for (j = 0; j < s->xdelta; ++j) {
> +                    uint32_t diff = (*(ptr[0]++) << 24) |
> +                                    (*(ptr[1]++) << 16) |
> +                                    (*(ptr[2]++) << 8);

Don't we have some get_bits or similar helper function for this?

> +            default:
> +                av_assert1(0);

Asserting here feels dangerous.  Are you sure this cannot be reached by
damaged or crafted input?

> +    if (data_size < uncompressed_size) {
> +        av_fast_padded_malloc(&td->uncompressed_data, &td->uncompressed_size, uncompressed_size);
> +        av_fast_padded_malloc(&td->tmp, &td->tmp_size, uncompressed_size);

nit: long line

> +    if (magic_number != 20000630) {
> +        /* As per documentation of OpenEXR, it is supposed to be
> +           int 20000630 little-endian */

nit: Precede the second line of the comment by *.

> +    // Parse the header
> +    while (buf < buf_end && buf[0]) {
> +            while (channel_list_end - buf >= 19) {
> +                s->channels = av_realloc(s->channels,
> +                                         ++s->nb_channels * sizeof(EXRChannel));
> +                if (!s->channels)
> +                    return AVERROR(ENOMEM);
> +            }
> +
> +            /* Check if all channels are set with an offset or if the channels
> +             * are causing an overflow  */
> +
> +            if (FFMIN3(s->channel_offsets[0],
> +                       s->channel_offsets[1],
> +                       s->channel_offsets[2]) < 0) {
> +                return AVERROR_INVALIDDATA;

Who frees the memory if you error out here?

> +        } else if (check_header_variable(avctx, &buf, buf_end,
> +                                         "lineOrder", "lineOrder", 25,
> +                                         &variable_buffer_data_size) < 0) {
> +            av_log(avctx, AV_LOG_DEBUG, "line order : %d.\n", *buf);

PRIu8 I think.

> +    if (s->compr != EXR_RAW) {
> +        m = av_fast_realloc(s->thread_data, &s->thread_data_size, thread_data_size);
> +        if (!m)
> +            return AVERROR_INVALIDDATA;
> +        s->thread_data = m;
> +        memset(s->thread_data + prev_size, 0, s->thread_data_size - prev_size);
> +    }
> +
> +    if ((ret = ff_thread_get_buffer(avctx, &frame, 0)) < 0)
> +        return ret;

Who takes care of freeing the memory?

Diego


More information about the libav-devel mailing list