[FFmpeg-devel] [PATCH][RFC] Lagarith Decoder.

Reimar Döffinger Reimar.Doeffinger
Fri Aug 21 09:59:43 CEST 2009


On Thu, Aug 20, 2009 at 11:45:53PM -0600, Nathan Caldwell wrote:
> +        while ((l->prob[val + 1] <= low_scaled) && (val < 255))
> +            val++;

Since I think prob is monotonous you could use a binary search the 8
steps of which could be completely unrolled. It might well be slower
though (if low_scaled is usually small - to avoid this you could
try to precalculate a low_scaled threshold to switch between the two
methods).
Anyway, those are a few () more than necessary.

> +static uint8_t lag_calc_zero_run(int8_t x)
> +{
> +#if 0
> +    x <<= 1;
> +    if (x > 255)
> +        return ~x;
> +    return x;
> +#endif

if you want to leave that in, at least add and use int x2 = (uint8_t)x;
instead of x, since it won't work like that.

> +    for (i = 0; !(prevbit && bit); i++) {
> +        prevbit = bit;
> +        bit = get_bits1(gb);
> +        if (bit && !prevbit)
> +            bits += series[i];
> +    }

This is still an endless loop (well, until it crashes because it reached
the end of the buffer without noticing).

> +    if (bits > 31) {
> +        /* This is most likely an error, just use the final 32 bits */
> +        skip_bits_long(gb, bits - 31);
> +        bits = 31;
> +    }

Allowing an arbitrary skip by a user-defined value, even far outside the buffer?
At least it's not a put_bit_context otherwise that would be great for
exploits.

> +    /* Scale probabilities so cumulative probability is an even power of 2. */
> +    scale_factor = cumul_prob ? av_log2(cumul_prob - 1) + 1 : 0;

Didn't you just check that cumul_prob is > 0?

> +    if (cumul_prob & (cumul_prob - 1)) {

Also since you now check for power-of-two or not this can be simplified to:
scale_factor = av_log2(cumul_prob);
if (cumul_prob & (cumul_prob - 1)) {
    scale_factor++;
...

> +        if (esc_count) {
> +            length = AV_RL32(src + 1);
> +            if (length < width * height)
> +                offset += 4;
> +            else
> +                length = width * height;
> +        } else
> +            length = width * height;

length = width * height;
if (esc_count && AV_RL32(src + 1) < length) {
    length = AV_RL32(src + 1);
    offset += 4;
}

> +        init_get_bits(&gb, src + offset, src_size * 8);
> +
> +        if (lag_read_prob_header(&rac, &gb) < 0)
> +            return -1;
> +
> +        lag_rac_init(&rac, &gb, length - stride);
> +
> +        for (i = 0; i < height; i++)
> +            read +=
> +                lag_decode_line(l, &rac, dst + (i * stride), width,
> +                                stride, step, esc_count);
> +
> +        if (read > length)
> +            av_log(l->avctx, AV_LOG_WARNING,
> +                   "Read more bytes than length (%d of %d)\n", read,
> +                   length);

Maybe you should instead make sure that length < size and that you don't
overread? Not crashing at every little bit broken file is generally
considered good.

> +        /* Plane is a solid run of 0 bytes */
> +        memset(dst, 0, stride * height);

I'm not completely sure if you're supposed to overwrite the padding.
Avoiding it might be better (i.e. looping over each line independently).

> +    frametype = buf[0];
> +
> +    offset_gu = AV_RL32(buf + 1);
> +    offset_bv = AV_RL32(buf + 5);

*buf++;
bytestream_get_le32(&buf);
bytestream_get_le32(&buf);
might look nicer.



More information about the ffmpeg-devel mailing list