[FFmpeg-devel] [PATCH] Mimic decoder

[Michael Niedermayer]

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>