[Ffmpeg-devel] [RFC] VC3/DNxHD decoder

[Michael Niedermayer]

Hi

On Sun, Mar 18, 2007 at 08:32:43PM +0100, Baptiste Coudurier wrote:
[...]
> >> +    DECLARE_ALIGNED_8(ScanTable, scantable);
> >> +    const CIDEntry *cid_table;
> >> +} DNXHDContext;
> >> +
> >> +static const CIDEntry cid_table[] = {
> >> +    { 1238, 1920, 1080, 0, 917504, 4, 8,
> >> +      dnxhd_1238_luma_weigth, dnxhd_1238_chroma_weigth,
> >> +      dnxhd_1238_dc_codes, dnxhd_1238_dc_bits,
> >> +      dnxhd_1238_ac_codes, dnxhd_1238_ac_bits, dnxhd_1238_ac_level,
> >> +      dnxhd_1238_ac_run_flag, dnxhd_1238_ac_index_flag,
> >> +      dnxhd_1238_run_codes, dnxhd_1238_run_bits, dnxhd_1238_run_level },
> >> +/*     { 1243, 1920, 1080, 1, 917504, 4, 8, */
> >> +/*       dnxhd_1243_luma_weigth, dnxhd_1243_chroma_weigth, */
> >> +/*       dnxhd_1238_dc_codes, dnxhd_1238_dc_bits, */
> >> +/*       dnxhd_1238_ac_codes, dnxhd_1238_ac_bits, dnxhd_1238_ac_level, */
> >> +/*       dnxhd_1238_ac_run_flag, dnxhd_1238_ac_index_flag, */
> >> +/*       dnxhd_1238_run_codes, dnxhd_1238_run_bits, dnxhd_1238_run_level }, */
> >> +};
> > 
> > id suggest that all values which are always the same no matter which cid
> > should be hardcoded as that should be faster
> 
> What do you mean by hardcoded ?

#define INDEX_BITS 4
INDEX_BITS

instead of

ctx->index_bits

for example


[...]
> >> +    //av_log(ctx->avctx, AV_LOG_DEBUG, "dc %d, diff %d\n", dc, diff);
> >> +    for (i = 1; i < 65; i++) {
> >> +        index = get_vlc2(&ctx->gb, ctx->ac_vlc.table, DNXHD_VLC_BITS, 2);
> >> +        //av_log(ctx->avctx, AV_LOG_DEBUG, "index %d\n", index);
> >> +        if (index == 4) { /* EOB */
> >> +            //av_log(ctx->avctx, AV_LOG_DEBUG, "EOB\n");
> >> +            break;
> >> +        }
> >> +
> >> +        if (i > 63) {
> >> +            av_log(ctx->avctx, AV_LOG_ERROR, "ac tex damaged %d, %d\n", n, i);
> >> +            return -1;
> >> +        }
> >> +
> >> +        sign = get_bits1(&ctx->gb) ? -1 : 1;
> >> +
> >> +        level = ctx->ac_level[index];
> >> +        if (ctx->ac_index_flag[index]) {
> >> +            level += get_bits(&ctx->gb, ctx->index_bits)<<6;
> >> +        }
> >> +
> >> +        if (ctx->ac_run_flag[index]) {
> > 
> > index= get_vlc()
> > ctx->ac_level[index]
> > ctx->ac_index_flag[index]
> > ctx->ac_run_flag[index]
> > 
> > is quite inefficint as it needs 3 reads for the pointers to the tables
> > 3 more reads with base+index to get the actual 3 values
> > 
> > by chaning the tables this could be done for example like
> > level= get_vlc()
> > level & 0x400
> > level & 0x800
> > level &=0x3FF
> > 
> > you avoid all thouse reads
> 
> Ok, I got it, I need to change values in ac_level table right ?

you need to change the order of the elements in the len and bits tables
passed into init_vlc()


> I'll do it after svn inclusion is it ok ?

ok


[...]
> > you could also try to merge the (2*level+1) into the level table though it
> > would complicat the ctx->ac_index_flag[index]==1 case a little but it still
> > might be faster as i think ctx->ac_index_flag[index]==1 is rare
> 
> Humm, I'll try to optimize it further after svn inclusion, is it ok ?

ok

[...]
> +typedef struct {
> +    AVCodecContext *avctx;
> +    AVFrame picture;
> +    GetBitContext gb;
> +    int cid; ///< compression id
> +    unsigned int width, height;
> +    unsigned int mb_width, mb_height;
> +    uint32_t mb_scan_index[68]; /* max for 1080p */
> +    int cur_field; ///< current interlaced field
> +    int index_bits; ///< length of index value

vertically aligning the comments makes them more readable IMHO

    int cid;                            ///< compression id
    unsigned int width, height;
    unsigned int mb_width, mb_height;
    uint32_t mb_scan_index[68];         /* max for 1080p */
    int cur_field;                      ///< current interlaced field
    int index_bits;                     ///< length of index value


[...]
> +static int dnxhd_decode_macroblock(DNXHDContext *ctx, int x, int y)
> +{
> +    int dct_linesize_luma   = ctx->picture.linesize[0];
> +    int dct_linesize_chroma = ctx->picture.linesize[1];
> +    uint8_t *dest_y, *dest_u, *dest_v;
> +    int dct_offset;
> +    int qscale, i;
> +
> +    ctx->dsp.clear_blocks(ctx->blocks[0]);
> +    ctx->dsp.clear_blocks(ctx->blocks[2]); // FIXME change clear blocks to take block amount
> +
> +    qscale = get_bits(&ctx->gb, 11);
> +    skip_bits1(&ctx->gb);
> +    //av_log(ctx->avctx, AV_LOG_DEBUG, "qscale %d\n", qscale);
> +
> +    for (i = 0; i < 8; i++) {
> +        dnxhd_decode_dct_block(ctx, ctx->blocks[i], i, qscale);

the <0 return is ignored, not a big issue but still, id either make functions
return void or check their return value and pass the error up

besides these patch ok

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

it is not once nor twice but times without number that the same ideas make
their appearance in the world. -- 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/20070318/2dd66bc0/attachment.pgp>