On Sat, Jul 28, 2012 at 01:31:33PM -0400, Derek Buitenhuis wrote:
> On 28/07/2012 3:44 AM, Kostya Shishkov wrote:
> >> + for (k = prefix << sym_shift; k < prefix + 1 << sym_shift;
> >> k++) {
> >> > + code_table[2 * k] = symbol;
> >> > + code_table[2 * k + 1] = i + 1;
> >> > + }
> > In theory you could collect (symbol, i+1, prefix) values and use them to
> > initialise our VLC structure instead of making LUT yourself (exactly like
> > binary does ;)
>
> If you think it's worth the time, I could try, but those functions look
> not-so-friendly.
Probably not, but it's worth commenting what you're doing here and later.
Or you can reply to someone asking what's being done here "oh, isn't it
obvious?".
> >> + if (num_codes)
> >> + num_lens = num_lens_orig;
> >
> > not needed (num_codes is not modified) - RE leftover?
>
> Yeah. Removed.
>
> >> + /* Stash the first pixel */
> >> + pred = *top_left = *top_left + code_table[2 * show_bits(gb, 14)];
> >
> > ahem, someone forgot to _skip_ bits here
>
> Uh... I think I did, indeed.
That's because first value is used to update pred and loop runs
for(i = 1; i < width; i++).
But it can be simplified to
pred = *top_left;
for (i = 0; i < ctx->avctx->width; i++) {
pred += code_table[2 * show_bits(gb, 14)];
skip = code_table[2 * show_bits(gb, 14) + 1];
skip_bits(gb, skip);
dst[0] = pred;
dst += 3;
}
*top_left = dst[-3 * avctx->width];
or something
> >> + pred[0] = 128;
> >> + pred[1] = 128;
> >> + pred[2] = -128;
> >
> > perchance, they are all 0x80 ?
>
> Indeed, you are correct, sir.
>
> >> + info_tag = AV_RL32(src);
> >> + if (info_tag == MKTAG('I', 'N', 'F', 'O')) {
> >> + info_offset = AV_RL32(src + 4) + 8;
> >> + src += info_offset;
> >
> > check that you skip reasonable number of bytes
>
> Covered in Måns' and Ronald's reviews.
See how obvious it is,
> >> + ctx->swapped_buf = av_malloc(avctx->width * avctx->height * 3);
> >
> > Are you sure it's always enough? Max code length is 14 bits and some space
> > is
> > needed for code descriptions. It's better to be pessimistic here than crash
> > on
> > too big input frame later.
>
> You think it's possible to be larger than the raw frame? o.o
Just think of some evil person faking large frame. And boom goes your decoder.
That's why we usually just realloc it depending on input frame size.
> >> + /* Allocate space for our code tables */
> >> + for (i = 0; i < 3; i++) {
> >> + ctx->code_table[i] = av_malloc(32768);
> >
> > You can just move them to context instead of allocating dynamically.
>
> I figured allocating 3 arrays of 32768 on the stack probably wasn't a
> good idea...
Stack? I said CLLCContext, which is allocated by lavc.
_______________________________________________
libav-devel mailing list
[email protected]
https://lists.libav.org/mailman/listinfo/libav-devel
