[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