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

Michael Niedermayer michaelni
Sat Mar 29 00:11:26 CET 2008


On Fri, Mar 28, 2008 at 02:55:17PM -0700, Eli Friedman wrote:
> Per subject, revision of Escape 124 decoder per review comments.
> Biggest change is that the decoder now puts each superblock into the
> frame as it is decoded.  Also added some checks to make sure the code
> doesn't read past the end of the buffer on invalid data, and a few
> other minor changes.
> 
> -Eli

> Index: escape124.c
> ===================================================================
> --- escape124.c	(revision 0)
> +++ escape124.c	(revision 0)
[...]
> +
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <string.h>
> +#include <unistd.h>

are all of these headers really needed ?


[...]
> +static CodeBook* unpack_codebook(GetBitContext* gb, uint32_t depth,
> +                                 uint32_t length, uint32_t alloc_length) {
> +    uint32_t i, j;

These dont need to be exactly 32 bit, so unsigned (int) seems more appropriate.


> +    CodeBook* cb;
> +
> +    if (alloc_length >= (INT_MAX - sizeof(CodeBook)) / sizeof(MacroBlock))
> +        return NULL;
> +    cb = av_malloc(alloc_length * sizeof(MacroBlock) + sizeof(CodeBook));
> +    if (!cb)
> +        return NULL;
> +
> +    if (!can_safely_read(gb, length * 34))
> +        return NULL;

memleak


[...]
> +static unsigned vlc_decode(GetBitContext* gb) {
> +    unsigned value = can_safely_read(gb, 1) && get_bits1(gb);
> +    if (!value || !can_safely_read(gb, 3))
> +        return value;
> +
> +    value += get_bits(gb, 3);
> +    if (value != (1 + ((1 << 3) - 1)) || !can_safely_read(gb, 7))
> +        return value;
> +
> +    value += get_bits(gb, 7);
> +    if (value != (1 + ((1 << 3) - 1)) + ((1 << 7) - 1) ||
> +        !can_safely_read(gb, 12))
> +        return value;
> +
> +    return value + get_bits(gb, 12);
> +}

The first check is enough as there are at least 8*FF_INPUT_BUFFER_PADDING_SIZE
bits "overallocated" at the end.
And several other checks in other functions are similarly redundant.


> +
> +static SuperBlock read_superblock_from_buffer(
> +        uint16_t* frame_data, uint32_t stride,
> +        uint32_t superblock_row_index, uint32_t superblock_col_index) {
> +    unsigned j, k;
> +    SuperBlock sb;

> +    frame_data += stride * 8 * superblock_row_index +
> +                  8 * superblock_col_index;

vertical align:
frame_data += stride * 8 * superblock_row_index +
                       8 * superblock_col_index;


> +    for (j = 0; j < 4; j++) {
> +        for (k = 0; k < 4; k++) {
> +            MacroBlock* curBlock = &sb.blocks[j*4 + k];
> +            uint16_t* block_base = frame_data + j * 2 * stride + k * 2;
> +            curBlock->pixels[0] = block_base[0];
> +            curBlock->pixels[1] = block_base[1];
> +            curBlock->pixels[2] = block_base[stride];
> +            curBlock->pixels[3] = block_base[stride + 1];
> +        }
> +    }
> +    return sb;
> +}
> +
> +static void write_superblock_to_buffer(
> +        uint16_t* frame_data, uint32_t stride,
> +        uint32_t superblock_row_index, uint32_t superblock_col_index,
> +        SuperBlock sb) {
> +    unsigned j, k;
> +    frame_data += stride * 8 * superblock_row_index +
> +                  8 * superblock_col_index;
> +    for (j = 0; j < 4; j++) {
> +        for (k = 0; k < 4; k++) {
> +            MacroBlock* curBlock = &sb.blocks[j*4 + k];
> +            uint16_t* block_base = frame_data + 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];
> +        }
> +    }
> +}

These 2 functions really should just be: (its simpler and faster)
for(y=0; y<8; y++)
    memcpy(dst + y*dst_stride, src + y*src_stride, sizeof(uint16_t)*8);


[...]

> +    if (64 + get_bits_count(&gb) > gb.size_in_bits)
> +        return -1;

can_safely_read()


[...]
> +    if (frame_flags & (1 << 17)) {
> +        // This is the most basic codebook: pow(2,depth) entries for 
> +        // a depth-length key
> +        if (!can_safely_read(&gb, 4))
> +            return -1;
> +        cb_depth = get_bits(&gb, 4);
> +        cb_size = (1 << cb_depth);
> +        av_free(s->codebooks[1]);
> +        s->codebooks[1] = unpack_codebook(&gb, cb_depth, cb_size, cb_size);
> +    }
> +
> +    if (frame_flags & (1 << 18)) {
> +        // This codebook varies per superblock
> +        // FIXME: I'm not sure if I'm handling the cb_depth=0 case
> +        // correctly
> +        if (!can_safely_read(&gb, 4))
> +            return -1;
> +        cb_depth = get_bits(&gb, 4);
> +        cb_size = (1 << cb_depth) * s->num_superblocks;
> +        av_free(s->codebooks[0]);
> +        s->codebooks[0] = unpack_codebook(&gb, cb_depth, cb_size, cb_size);
> +    }
> +
> +    if (frame_flags & (1 << 19)) {
> +        // This codebook can be cut off at places other than powers of 2,
> +        // leaving some of the entries undefined.
> +        if (!can_safely_read(&gb, 4))
> +            return -1;
> +        cb_size = get_bits_long(&gb, 20);
> +        cb_depth = av_log2(cb_size - 1) + 1;
> +        cb_alloc_size = 1 << cb_depth;
> +        av_free(s->codebooks[2]);
> +        s->codebooks[2] = unpack_codebook(&gb, cb_depth, cb_size, cb_alloc_size);
> +    }

these 3 should be in a loop instead of duplicating most code twice


> +
> +    if (!s->codebooks[0] || !s->codebooks[1] || !s->codebooks[2])
> +        return -1;
> +
> +    cb_index = 0;
> +    superblock_index = 0;
> +    superblock_row_index = 0;
> +    superblock_col_index = 0;
> +

> +    do {
> +        uint32_t multi_mask = 0;
> +        uint32_t skip = vlc_decode(&gb);
> +    

trailing whitespace


[...]

> +AVCodec escape124_decoder = {
> +    "escape124",
> +    CODEC_TYPE_VIDEO,
> +    CODEC_ID_ESCAPE124,
> +    sizeof(Escape124Context),
> +    escape124_decode_init,
> +    NULL,
> +    NULL,
> +    escape124_decode_frame,
> +    CODEC_CAP_DR1,
> +};

You are forgetting to deallocate all the stuff when the decoder is freed.


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

it is not once nor twice but times without number that the same ideas make
their appearance in the world. -- Aristotle
-------------- 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/20080329/73c52f38/attachment.pgp>



More information about the ffmpeg-devel mailing list