[FFmpeg-devel] [PATCH] Mimic decoder

[Ramiro Polla]

Hello,

Michael Niedermayer wrote:
> 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 ...

Shuffled comments around some more. Nitpickers are welcome to suggest 
more changes.

> [...]
> 
>>     AVFrame         flipped_ptrs[16];
> 
> why is this not AVPicture ?

Done.

> [...]
>>     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 ?

flipped_ptrs are now used directly.

>>     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).

Removed. get_buffers gave me enough padding.

> [...]
>>                 }
>>                 src += 8;
>>                 dst += 8;
>>             }
> 
>>             offset += stride << 3;
> 
> src += 8*(stride - ctx->num_hblocks[plane]);
> dst += 8*(stride - ctx->num_hblocks[plane]);

Done.

> [...]
>>             /* 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.

Committed.

Thanks for the review. Also, many thanks to mru and iive and all the 
other people that helped at #ffmpeg-devel at freenode.net. Specially iive 
for teaching me a bunch of idct-related stuff from FFmpeg. Oh, and 
thanks to Ole Andr? Vadla Ravn?s for the original REing.

Ramiro Polla