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

[Michael Niedermayer]

On Sat, Mar 29, 2008 at 01:17:19AM -0700, Eli Friedman wrote:
> Per subject, third revision of Escape 124 decoder.

[...]
> +        uint32_t mask_bits = get_bits(gb, 4);
> +        uint32_t color0 = get_bits(gb, 15);
> +        uint32_t color1 = get_bits(gb, 15);

normal unsigned is enough for these


[...]
> +static SuperBlock read_superblock_from_buffer(
> +        uint16_t* frame_data, uint32_t stride,
> +        uint32_t superblock_row_index, uint32_t superblock_col_index) {
> +    unsigned y;
> +    SuperBlock sb;
> +    // The first frame defaults to black
> +    if (!frame_data)
> +        return (SuperBlock) { { { { 0 } } } };

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

As said in a previous review, vertical align these please.


> +    for(y=0; y<8; y++)
> +        memcpy(sb.pixels + y * 8, frame_data + y * stride, sizeof(uint16_t)*8);
> +    return sb;

This copies the data twice.


> +}
> +
> +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 y;
> +    frame_data += stride * 8 * superblock_row_index +
> +                  8 * superblock_col_index;
> +    for(y=0; y<8; y++)
> +        memcpy(frame_data + y * stride, sb.pixels + y * 8, sizeof(uint16_t)*8);
> +}

This has the same problem, it also copies the data twice.


> +
> +
> +static MacroBlock decode_macroblock(Escape124Context* s, GetBitContext* gb,
> +                                    int* codebook_index, int superblock_index) {
> +    // This funxtion reads a maximum of 22 bits; the callers
                  ^
typo


[...]
> +        block_index += (1 << s->codebooks[1]->depth) * superblock_index;

This can be done without the multiply.


[...]

> +static void insert_mb_into_sb(SuperBlock* sb, MacroBlock mb, unsigned index) {
> +    unsigned base = (index / 4) * 16 + (index % 4) * 2;
> +    sb->pixels[base] = mb.pixels[0];
> +    sb->pixels[base+1] = mb.pixels[1];
> +    sb->pixels[base+8] = mb.pixels[2];
> +    sb->pixels[base+9] = mb.pixels[3];

static inline void insert_mb_into_sb(SuperBlock* sb, MacroBlock mb, unsigned index) {
    static const uint8_t base_tab[]={...
    unsigned base = base_tab[index];
    uint32_t *dst= sb->pixels;

    dst[base  ]= ((uint32_t*)mb.pixels)[0]
    dst[base+4]= ((uint32_t*)mb.pixels)[1]


[...]
> +    frame_flags = get_bits_long(&gb, 32);
> +    frame_size = get_bits_long(&gb, 32);

vertical align


> +
> +    // Leave last frame unchanged
> +    // FIXME: Is this necessary?  I haven't seen it in any real samples
> +    if (!(frame_flags & 0x114) || !(frame_flags & 0x7800000)) {
> +        av_log(NULL, AV_LOG_DEBUG, "Skipping frame\n");
> +
> +        *data_size = sizeof(AVFrame);
> +        *(AVFrame*)data = s->frame;
> +
> +        return frame_size;
> +    }
> +
> +    for (i = 0; i < 3; i++) {
> +        if (frame_flags & (1 << (17 + i))) {

> +            if (i == 0) {
> +                // This is the most basic codebook: pow(2,depth) entries for
> +                // a depth-length key
> +                cb_depth = get_bits(&gb, 4);
> +                cb_size = 1 << cb_depth;
> +                cb_alloc_size = cb_size;
> +            } else if (i == 1) {
> +                // This codebook varies per superblock
> +                // FIXME: I don't think this handles integer overflow
> +                // properly
> +                cb_depth = get_bits(&gb, 4);
> +                cb_size = s->num_superblocks << cb_depth;
> +                cb_alloc_size = cb_size;
> +            } else {
> +                // This codebook can be cut off at places other than
> +                // powers of 2, leaving some of the entries undefined.
> +                cb_size = get_bits_long(&gb, 20);
> +                cb_depth = av_log2(cb_size - 1) + 1;
> +                cb_alloc_size = 1 << cb_depth;
> +            }

What is the use of having cb_alloc_size > cb_size ?
And theres some common code in the above which can be factored out



> +            av_free(s->codebooks[i]);
> +            s->codebooks[i] = unpack_codebook(&gb, cb_depth, cb_size,
> +                                              cb_alloc_size);
> +            if (!s->codebooks[i])
> +                return -1;
> +        }
> +    }
> +

> +    if (avctx->get_buffer(avctx, &new_frame)) {

You must set reference before calling get_buffer() if you read from the frame
later.


> +        av_log(avctx, AV_LOG_ERROR, "get_buffer() failed\n");
> +        return -1;
> +    }
> +
> +    new_frame_data = (uint16_t*)new_frame.data[0];
> +    new_stride = new_frame.linesize[0] / 2;
> +    old_frame_data = (uint16_t*)s->frame.data[0];
> +    old_stride = s->frame.linesize[0] / 2;
> +
> +    superblocks_per_row = avctx->width / 8;
> +

> +    cb_index = 1;
> +    superblock_index = 0;
> +    superblock_row_index = 0;
> +    superblock_col_index = 0;

initialization and declaration can be merged


> +
> +    do {
> +        uint32_t multi_mask = 0;
> +        // Note that this call will make us skip the rest of the blocks
> +        // if the frame prematurely ends
> +        uint32_t skip = decode_skip_count(&gb);
> +
> +        for (i = 0; i < skip && superblock_index < s->num_superblocks;
> +             i++, superblock_index++) {
> +            sb = read_superblock_from_buffer(old_frame_data, old_stride,
> +                                             superblock_row_index,
> +                                             superblock_col_index);
> +            write_superblock_to_buffer(new_frame_data, new_stride,
> +                                       superblock_row_index,
> +                                       superblock_col_index, sb);
> +            superblock_col_index++;
> +            if (superblock_col_index == superblocks_per_row) {
> +                superblock_row_index++;
> +                superblock_col_index = 0;
> +            }
> +        }
> +
> +        if (superblock_index >= s->num_superblocks)
> +            break;
> +
> +        sb = read_superblock_from_buffer(old_frame_data, old_stride,
> +                                         superblock_row_index,
> +                                         superblock_col_index);
> +
> +        while (can_safely_read(&gb, 1) && !get_bits1(&gb)) {
> +            uint32_t mask;
> +            mb = decode_macroblock(s, &gb, &cb_index, superblock_index);
> +            mask = get_bits(&gb, 16);
> +            multi_mask |= mask;
> +            for (i = 0; i < 16; i++) {
> +                if (mask & mask_matrix[i]) {
> +                    insert_mb_into_sb(&sb, mb, i);
> +                }
> +            }
> +        }
> +

> +        if (can_safely_read(&gb, 1) && !get_bits1(&gb)) {

i think the can_safely_read is unneeded here.


> +            uint32_t inv_mask;
> +            inv_mask = get_bits(&gb, 4);

declaration and initialization can be merged


> +            for (i = 0; i < 4; i++) {
> +                if (inv_mask & (1 << i)) {
> +                    multi_mask ^= 0xF << i*4;
> +                } else {
> +                    multi_mask ^= get_bits(&gb, 4) << i*4;
> +                }
> +            }
> +
> +            for (i = 0; i < 16; i++) {

> +                if (!can_safely_read(&gb, 1))
> +                    break;
> +                if (multi_mask & mask_matrix[i]) {

the can_safely_read() should be done after the mask check its redundant before.


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

Opposition brings concord. Out of discord comes the fairest harmony.
-- Heraclitus
-------------- 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/6980eb05/attachment.pgp>