[FFmpeg-devel] [PATCH] Mimic decoder
Michael Niedermayer
michaelni
Tue Mar 18 01:34:09 CET 2008
On Mon, Mar 17, 2008 at 07:49:31PM -0300, Ramiro Polla wrote:
[...]
>>> unsigned int num_rows = 8;
>>> /* The last row of blocks in chrominance for 160x120
>>> resolution
>>> * is half the normal height and must be accounted for. */
>>> if(is_chroma &&
>>> y + 1 == ctx->num_vblocks[plane] && ctx->avctx->height &
>>> 15)
>>> num_rows = 4;
>>> for(x = 0; x < ctx->num_hblocks[plane]; x++) {
>>> /* Check for a change condition in the current block.
>>> * !pframes always change.
>>> * Luma plane checks for get_bits1 == 0, and chroma
>>> planes
>>> * check for get_bits1 ==1. */
>>> if(!is_pframe || get_bits1(&ctx->gb) == is_chroma) {
>>> /* Luma only: Is the new content the same as it was
>>> in one
>>> * of the 15 last frames preceding the previous?
>>> * Chroma planes don't use backreferences. */
>>> if(is_chroma || !is_pframe || !get_bits1(&ctx->gb)) {
>>> if(!vlc_decode_block(ctx, ctx->dct_block,
>>> num_coeffs, qscale))
>>> return 0;
>>> ctx->dsp.idct_put(dst, stride, ctx->dct_block);
>>> } else {
>>> unsigned int backref;
>>> uint8_t *p;
>>> /* Yes: read the backreference (4 bits) and copy.
>>> */
>>> backref = get_bits(&ctx->gb, 4);
>>> p = ctx->flipped_ptrs[(ctx->ptr_index + backref)
>>> & 15].data[0];
>> declaration and initalization can be merged
>
> Done.
>
>> also i think a few empty lines somewhere could improve readability
>
> I added some whitespaces. But I've grown so used to this code that it was
> more readable for me before. The nitpickers are welcome to comment on this
> =)
I think it would be alot more readable without the comments. They just say
what is obvious from the code ...
[...]
> AVFrame flipped_ptrs[16];
why is this not AVPicture ?
[...]
> AVPicture cur_frame;
> AVPicture prev_frame;
>
> prepare_avpic(ctx, &cur_frame , (AVPicture*) &ctx->buf_ptrs[ctx->cur_index] );
> prepare_avpic(ctx, &prev_frame, (AVPicture*) &ctx->buf_ptrs[ctx->prev_index]);
why arent these simply copied from flipped_ptrs ?
>
> for(plane = 0; plane < 3; plane++) {
> const int is_chroma = !!plane;
> const int qscale = av_clip(10000-quality,is_chroma?1000:2000,10000)<<2;
> const int stride = cur_frame.linesize[plane];
> int offset = 0;
> const uint8_t *base_src = prev_frame.data[plane];
> uint8_t *base_dst = cur_frame. data[plane];
> for(y = 0 ; y < ctx->num_vblocks[plane] ; y++) {
> const uint8_t *src = base_src + offset;
> uint8_t *dst = base_dst + offset;
> unsigned int num_rows = 8;
>
> /* The last row of blocks in chrominance for 160x120 resolution
> * is half the normal height and must be accounted for. */
> if(is_chroma &&
> y + 1 == ctx->num_vblocks[plane] && ctx->avctx->height & 15)
> num_rows = 4;
Whats the sense of num_rows ? its only honored in some cases others just
write 8 (which should be fine due to buffer padding i think).
[...]
> }
> src += 8;
> dst += 8;
> }
> offset += stride << 3;
src += 8*(stride - ctx->num_hblocks[plane]);
dst += 8*(stride - ctx->num_hblocks[plane]);
[...]
> /* chroma planes have 1 more vblock for 160x120 samples
> * this obfuscation saves an if() */
> ctx->num_vblocks[i] = -((-height) >> (3 + !!i));
The "obfuscation" just rounds to +inf and is really the correct way to round
unless you know its a multiple of the block size.
anyway feel free to commit after dealing with the above issues.
[...]
--
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
I hate to see young programmers poisoned by the kind of thinking
Ulrich Drepper puts forward since it is simply too narrow -- Roman Shaposhnik
-------------- 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/20080318/5a25d10f/attachment.pgp>
More information about the ffmpeg-devel
mailing list