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
_______________________________________________
libav-devel mailing list
[email protected]
https://lists.libav.org/mailman/listinfo/libav-devel
