[FFmpeg-devel] [PATCH]wmv3 encoder

Michael Niedermayer michaelni
Mon Jun 25 22:40:07 CEST 2007


Hi

On Mon, Jun 25, 2007 at 09:23:12PM +0200, Denis Fortin wrote:
> Hi,
> The following patch adds a wmv3 encoder (i removed the vc1 encoder 
> because it doesn't generate an elementary stream).
> I don't think i can split the patch without breaking compilation between 
>  two patches.
> 
> Please comment (particularly the vc1enc.h include in mpegvideo.c).
[...]

> @@ -4585,6 +4598,10 @@
>          if (ENABLE_WMV2_ENCODER)
>              ff_wmv2_encode_mb(s, s->block, motion_x, motion_y);
>          break;
> +    case CODEC_ID_WMV3:
> +        if (ENABLE_WMV3_ENCODER)
> +            ff_vc1_encode_mb(s,s->block,motion_x,motion_y);
> +        break;
>      case CODEC_ID_H261:
>          if (ENABLE_H261_ENCODER)
>              ff_h261_encode_mb(s, s->block, motion_x, motion_y);

nitpick:
please align the () and arguments vertically to match the 2 surrounding
function calls


[...]
> +/**
> + * Unquantize a block
> + *
> + * @param s Encoder context
> + * @param block Block to quantize
> + * @param n index of block
> + * @param qscale quantizer scale
> + */
> +void vc1_unquantize_c(MpegEncContext *s, DCTELEM *block, int n, int qscale)

ff prefix is missing


> +{
> +    VC1Context * const t= s->avctx->priv_data;
> +    int i, level, nCoeffs, q;
> +    ScanTable scantable;
> +
> +    if(s->pict_type == I_TYPE)
> +        scantable = s->intra_scantable;
> +    else {
> +        scantable = s->inter_scantable;

> +        if( P_TYPE == s->pict_type )
> +            for(i=0;i<64;i++)
> +                block[i] += 128;

this looks wrong


[...]
> +        if (s->mb_intra){
> +            for(i=0;i<64;i++)
> +                block[i] -= 128;
> +        }

this can be done simpler after the transform


> +    }
> +
> +    s->dsp.vc1_fwd_trans_8x8(block);
> +
> +    if (n < 4)
> +        q = s->y_dc_scale;
> +    else
> +        q = s->c_dc_scale;
> +
> +    block[0] /= q;
> +    q = 2 * qscale + t->halfpq;
> +
> +    for(i=63;i>=start_i;i--) {
> +        j = scantable[i];
> +        level =  (block[j] - t->pquantizer*(FFSIGN(block[j]) * qscale)) / q;
> +        if(level){
> +            last_non_zero = i;
> +            break;
> +        }
> +    }

if it wherent a encoder for an obscure and useless format i would reject this
inefficient mess, but as it is i dont mind ...


[...]

> +    if( t->pqindex<=8 ) {
> +        ff_code012(&s->pb, s->rl_chroma_table_index%3);//TRANSACFRM (UV)
> +        ff_code012(&s->pb, s->rl_table_index%3); //TRANSACFRM2 (Y)
> +    } else {
> +        ff_code012(&s->pb, s->rl_chroma_table_index);//TRANSACFRM (UV)
> +        ff_code012(&s->pb, s->rl_table_index); //TRANSACFRM2 (Y)
> +    }

ff_code012 can only encode 0-2 so what is this if good for the %3 case should
work for both


[...]
> +    avctx->extradata_size = 32;
> +    avctx->extradata = av_mallocz(avctx->extradata_size + 10);

+ 10 ?


[...]
> +fail:
> +    return 1;

whats the fail label good for?

[...]
> Index: libavcodec/vc1.c
> ===================================================================
> --- libavcodec/vc1.c	(r??vision 9426)
> +++ libavcodec/vc1.c	(copie de travail)
> @@ -2278,7 +2278,7 @@
>   * @{
>   */
>  
> -static inline int vc1_coded_block_pred(MpegEncContext * s, int n, uint8_t **coded_block_ptr)
> +inline int vc1_coded_block_pred(MpegEncContext * s, int n, uint8_t **coded_block_ptr)
>  {
>      int xy, wrap, pred, a, b, c;
>  
> Index: libavcodec/vc1.h
> ===================================================================
> --- libavcodec/vc1.h	(r??vision 9426)
> +++ libavcodec/vc1.h	(copie de travail)
> @@ -304,4 +304,6 @@
>      int bi_type;
>  } VC1Context;
>  
> +inline int vc1_coded_block_pred(MpegEncContext * s, int n, uint8_t **coded_block_ptr);
> +

completely unacceptable gnu shit, go and read the c spec about inline


>  #endif // AVCODEC_VC1_H
> Index: libavcodec/vc1dsp.c
> ===================================================================
> --- libavcodec/vc1dsp.c	(r??vision 9426)
> +++ libavcodec/vc1dsp.c	(copie de travail)
[...]
> +        dest[0]  = 12 * (t1 + t3 + t5 + t7);
> +        dest[8]  = 16 * t2 + 15 * t4 + 9 * t6 + 4 * t8;
> +        dest[16] = 16 * (t1 - t7) + 6 * ( t3 - t5);
> +        dest[24] = 15 * t2 - 4 * t4 - 16 * t6 - 9 * t8;
> +        dest[32] = 12 * ( t1 - t3 - t5 + t7);
> +        dest[40] = 9 * t2 - 16 * t4 + 4 * t6 + 15 * t8;
> +        dest[48] = 6 * (t1 - t7) - 16 * (t3 - t5);
> +        dest[56] = 4 * t2 - 9 * t4 + 15 * t6 - 16 * t8;

vertical align please


> +
> +        dest++;
> +        src++;
> +    }
> +
> +    dest = block;
> +    src  = block;
> +
> +    norm_mat = norm_8;
> +    for(i=0 ; i < 8 ; i++){
> +        t1 = dest[0] + dest[7];
> +        t2 = dest[0] - dest[7];
> +        t3 = dest[1] + dest[6];
> +        t4 = dest[1] - dest[6];
> +
> +        t5 = dest[2] + dest[5];
> +        t6 = dest[2] - dest[5];
> +        t7 = dest[3] + dest[4];
> +        t8 = dest[3] - dest[4];
> +
> +        src[0] = ( (12 * (t1 + t3 + t5 + t7)) << 6 ) / (norm_mat[0]);
> +        src[1] = ( (16 * t2 + 15 * t4 + 9 * t6 + 4 * t8) << 6) / (norm_mat[1]);
> +        src[2] = ( (16 * (t1 - t7) + 6 * ( t3 - t5)) << 6) / (norm_mat[2]);
> +        src[3] = ( (15 * t2 - 4 * t4 - 16 * t6 - 9 * t8) << 6) / (norm_mat[3]);
> +        src[4] = ( (12 * ( t1 - t3 - t5 + t7)) << 6) / (norm_mat[4]);
> +        src[5] = ( (9 * t2 - 16 * t4 + 4 * t6 + 15 * t8) << 6) / (norm_mat[5]);
> +        src[6] = ( (6 * (t1 - t7) - 16 * (t3 - t5)) << 6) / (norm_mat[6]);
> +        src[7] = ( (4 * t2 - 9 * t4 + 15 * t6 - 16 * t8) << 6) / (norm_mat[7]);
> +
> +        dest+=8;
> +        src+=8;
> +        norm_mat+=8;
> +    }
> +}
> +
> +
> +void ff_vc1_inv_trans_put(uint8_t *dest, int linesize, DCTELEM block[64])
> +{
> +    int i,j;
> +    uint8_t *cm = ff_cropTbl + MAX_NEG_CROP;
> +
> +    vc1_inv_trans_8x8_c(block);
> +
> +    /* read the pixels */
> +    for(i=0;i<8;i++) {
> +        dest[0] = cm[block[0]];
> +        dest[1] = cm[block[1]];
> +        dest[2] = cm[block[2]];
> +        dest[3] = cm[block[3]];
> +        dest[4] = cm[block[4]];
> +        dest[5] = cm[block[5]];
> +        dest[6] = cm[block[6]];
> +        dest[7] = cm[block[7]];
> +
> +        dest += linesize;
> +        block += 8;
> +    }
> +}

duplicate of put_pixels_clamped*

all encoder specific things MUST be in files seperate from the decoder
so that compilation of them can be skiped if the user doesnt want the
encoder


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

The educated differ from the uneducated as much as the living from the
dead. -- Aristotle 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20070625/cf85191c/attachment.pgp>



More information about the ffmpeg-devel mailing list