On Sat, Mar 29, 2008 at 11:47 AM, Michael Niedermayer
<michaelni gmx.at> wrote:
> > + 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
?
So that the code doesn't crash on invalid bitstreams.
> > + if (can_safely_read(&gb, 1)
&& !get_bits1(&gb)) {
>
> i think the can_safely_read is unneeded here.
Possibly, although it's cutting it a bit close; I count 59
bits of
potential overread without that check.
-Eli
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-develmplayerhq.hu
https://lists.mplayerhq.hu/mailman/listinfo/ffmpeg-devel
|
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) { { { } } };
> + 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
framen");
> +
> + *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()
failedn");
> + 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
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-develmplayerhq.hu
https://lists.mplayerhq.hu/mailman/listinfo/ffmpeg-devel
|
On Sat, Mar 29, 2008 at 01:28:51PM -0700, Eli Friedman
wrote:
> On Sat, Mar 29, 2008 at 11:47 AM, Michael Niedermayer
<michaelnigmx.at> wrote:
> > > + 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 ?
>
> So that the code doesn't crash on invalid bitstreams.
Id rather check for the index being invalid. Either way such
rounding
up belongs somewhere else, not outside the function
allocating stuff.
And there should be a comment explaining the reason for the
round up.
[...]
--
Michael GnuPG fingerprint:
9FF2128B147EF6730BADF133611EC787040B0FAB
I wish the Xiph folks would stop pretending they've got
something they
do not. Somehow I fear this will remain a wish. -- Måns
Rullgård
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-develmplayerhq.hu
https://lists.mplayerhq.hu/mailman/listinfo/ffmpeg-devel
|
