[FFmpeg-devel] [PATCH] Mimic decoder

[Michael Niedermayer]

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


>
>>>         }
>>>     } 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


[...]

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

vertical align


>         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

>             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

also i think a few empty lines somewhere could improve readability


[...]

>     if(!(is_pframe && !ctx->prev_frame.data[0])) {

!(A && !B) == !A || B

or exchange the if/else and use if(A && !B)


[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Everything should be made as simple as possible, but not simpler.
-- Albert Einstein
-------------- 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/20080317/e28598df/attachment.pgp>