[FFmpeg-devel] [PATCH] Mimic decoder

Ramiro Polla ramiro
Mon Mar 17 23:49:31 CET 2008


Hello Michael,

Michael Niedermayer wrote:
> On Mon, Mar 17, 2008 at 04:01:05PM -0300, Ramiro Polla wrote:
> [...]
>>> [...]
>>>>             ctx->num_vblocks[i] = height >> (3 + !!i);
>>>>             ctx->num_hblocks[i] = width  >> (3 + !!i);
>>>>             if(i && height & 15)
>>>>                 ctx->num_vblocks[i]++;
>>> ctx->num_vblocks[i] = -((-height) >> (3 + !!i));
>> Hmm... wouldn't that be only if height was negative? It is always positive 
>> in the data.
> 
> it avoids the height & 15 check

Done. Now there are as many lines of comments as there were lines in the 
if statement =)

>>>>         }
>>>>     } else
>>>>     if(width != ctx->avctx->width || height != ctx->avctx->height) {
>>>>         av_log(avctx, AV_LOG_ERROR, "resolution changing is not 
>>>> supported\n");
>>>>         return -1;
>>>>     }
>>>>
>>>>     ctx->picture.data[0] = NULL;
>>>>     ctx->picture.reference = 1;
>>>>     if(avctx->get_buffer(avctx, &ctx->picture)) {
>>> the = NULL before get_buffer() looks very suspicious, you arent forgetting 
>>> to
>>> release them somewhere?
>> The decoder always get_buffer for a new picture at the start of 
>> decode_frame. That frame is kept to be used as back-reference for another 
>> 16 frames. At the end of decode_frame, it releases the last frame if it's 
>> no longer needed. decode_end then releases all frames.
> 
> remove the = NULL please

Done. Now the decoder only uses buf_ptrs and indexes. No more 
ctx->picture or ctx->prev_frame.

> [...]
> 
>>         const uint8_t *base_src = prev_frame.data[plane];
>>         uint8_t *base_dst = cur_frame.data[plane];
> 
> vertical align

Done.

>>         offset = 0;
>>         for(y = 0 ; y < ctx->num_vblocks[plane] ; y++) {
> 
>>             const uint8_t *src = base_src + offset;
>>             uint8_t *dst = base_dst + offset;
> 
> vertical align

Done.

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

> [...]
> 
>>     if(!(is_pframe && !ctx->prev_frame.data[0])) {
> 
> !(A && !B) == !A || B
> 
> or exchange the if/else and use if(A && !B)

Done.

Ramiro Polla
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: mimic.c
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20080317/7f021d4d/attachment.asc>



More information about the ffmpeg-devel mailing list