[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