[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