[FFmpeg-devel] [PATCH] Escape 124 (RPL) decoder

[Michael Niedermayer]

On Thu, Mar 27, 2008 at 06:58:39PM -0700, Eli Friedman wrote:
> Per subject, decoder for Escape 124, one of the three known formats
> for video in RPL
> (http://www.wiki.multimedia.cx/index.php?title=Escape_124).  I've
> tested this on some samples from the original Tomb Raider, and the
> decoder appears to be working flawlessly.  As far as I can tell, the
> codec description on the wiki is accurate; however, it took me a while
> to realize that depth=0 is legal in the codebooks.
> 
> This implementation is extremely prone to crash on invalid streams due
> to reading past the end of the buffer.  Any suggestions on how to make
> this safer?

Add a few checks using get_bits_count() and gb.size_in_bits
of course dont add more than needed!


[...]
> +static uint32_t rice_decode(GetBitContext* gb) {
> +    uint32_t more_bits, value;
> +
> +    more_bits = get_bits1(gb);
> +    value = more_bits;
> +    if (!more_bits)
> +        return value;
> +
> +    more_bits = get_bits(gb, 3);
> +    value += more_bits;
> +    if (more_bits != (1 << 3) - 1)
> +        return value;
> +
> +    more_bits = get_bits(gb, 7);
> +    value += more_bits;
> +    if (more_bits != (1 << 7) - 1)
> +        return value;
> +
> +    more_bits = get_bits(gb, 12);
> +    value += more_bits;
> +    return value;
> +}

This can be simplified, also why is it called rice?


> +
> +static void copy_superblocks_to_frame(AVCodecContext *avctx) {

> +    uint32_t i, j, k;

unsigned int, there is no need for these to be exactly 32bit


> +    Escape124Context *s = avctx->priv_data;
> +    uint32_t superblock_row_counter = 0;
> +    uint32_t stride = s->frame.linesize[0] / 2;
> +    uint16_t* frame_data = (uint16_t*)s->frame.data[0];
> +
> +    for (i = 0; i < s->num_superblocks; i++) {
> +        for (j = 0; j < 4; j++) {
> +            for (k = 0; k < 4; k++) {
> +                MacroBlock* curBlock = &s->superblocks[i].blocks[j*4 + k];
> +                uint16_t* block_base =
> +                    frame_data + superblock_row_counter * 8 +
> +                    j * 2 * stride + k * 2;
> +                block_base[0] = curBlock->pixels[0];
> +                block_base[1] = curBlock->pixels[1];
> +                block_base[stride] = curBlock->pixels[2];
> +                block_base[stride + 1] = curBlock->pixels[3];
> +            }
> +        }
> +        superblock_row_counter++;
> +        if (superblock_row_counter == avctx->width / 8) {
> +            frame_data += stride * 8;
> +            superblock_row_counter = 0;
> +        }
> +    }
> +}

Why this odd decode into these weird arrays and then copy into the frame
with the above function? IMHO decode one superblock and then copy that into
the frame.

[...]
> +static int mask_matrix[] = {0x1,   0x2,   0x10,   0x20,
> +                            0x4,   0x8,   0x40,   0x80,
> +                            0x100, 0x200, 0x1000, 0x2000,
> +                            0x400, 0x800, 0x4000, 0x8000};

static const uint16_t

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

It is dangerous to be right in matters on which the established authorities
are wrong. -- Voltaire
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20080328/bc947fb4/attachment.pgp>