[FFmpeg-devel] [PATCH] Indeo5 decoder

[Michael Niedermayer]

On Tue, Apr 07, 2009 at 10:52:34AM +0200, Maxim wrote:
> Michael Niedermayer schrieb:
> > On Mon, Apr 06, 2009 at 08:41:57PM +0200, Maxim wrote:
[...]
> >> +
> >> +
> >> +/**
> >> + *  Build static indeo5 dequantization tables.
> >> + */
> >> +static av_cold void build_dequant_tables(void)
> >> +{
> >> +    int         mat, i, lev;
> >> +    uint32_t    q1, q2, sf1, sf2;
> >> +
> >> +    for (mat = 0; mat < 5; mat++) {
> >> +        /* build 8x8 intra/inter tables for all 24 quant levels */
> >> +        for (lev = 0; lev < 24; lev++) {
> >> +            sf1 = ivi5_scale_quant_8x8_intra[mat][lev];
> >> +            sf2 = ivi5_scale_quant_8x8_inter[mat][lev];
> >> +
> >> +            for (i = 0; i < 64; i++) {
> >> +                q1 = (ivi5_base_quant_8x8_intra[mat][i] * sf1) >> 8;
> >> +                q2 = (ivi5_base_quant_8x8_inter[mat][i] * sf2) >> 8;
> >> +                deq8x8_intra[mat][lev][i] = av_clip(q1, 1, 255);
> >> +                deq8x8_inter[mat][lev][i] = av_clip(q2, 1, 255);
> >>     
> >
> > 1..255 but they arent uint8_t 
> > av_clip() seems useless  and the whole table precalc maybe as well
> >   
> 
> They were made uint16_t in order to achieve a compatibility with Indeo4
> that uses 9bits dequant tables...
> The table precalculation should help avoiding huge static tables...

let me clarify my question, what is gained by merging a multiply and shift
into the table?
is it faster? if so then by how much?


> 
> >> +
> >> +
> >> +/**
> >> + *  Decode indeo5 GOP (Group of pictures) header.
> >> + *  This header is present in key frames only.
> >> + *  It defines parameters for all frames in a GOP.
> >> + *
> >> + *  @param ctx      [in,out] ptr to the decoder context
> >> + *  @param avctx    [in] ptr to the AVCodecContext
> >> + *  @return         result code: 0 = OK, -1 = error
> >> + */
> >> +static int decode_gop_header(IVI5DecContext *ctx, AVCodecContext *avctx)
> >> +{
> >> +    int             result, i, p, tile_size, pic_size_indx, mb_size, blk_size, blk_size_changed = 0;
> >> +    IVIBandDesc     *band, *band1, *band2;
> >> +    IVIPicConfig    pic_conf;
> >> +
> >> +    ctx->gop_flags = get_bits(&ctx->gb, 8);
> >> +
> >> +    /* get size of the GOP header if present */
> >> +    ctx->gop_hdr_size = (ctx->gop_flags & 1) ? get_bits(&ctx->gb, 16) : 0;
> >> +
> >>     
> > +}
> >
> >   
> >> +    /* get 32-bit lock word in the case of password protected video */
> >> +    ctx->is_protected = !!(ctx->gop_flags & 0x20);
> >>     
> >
> > !! is useless
> > is_protected itself is probably redundant
> >   
> 
> Ok, this flag will be surely needed later in order to add a support for
> encrypted content. I try to get the basic stuff accepted first...

does is_protected ever differ from ctx->gop_flags & 0x20 ?
no, then its redundant


> 
> [...]
> 
> >> +/**
> >> + *  8x8 block motion compensation with adding delta.
> >> + *
> >> + *  @param  buf     [in,out] pointer to the block in the current frame buffer containing delta
> >> + *  @param  ref_buf [in]     pointer to the corresponding block in the reference frame
> >> + *  @param  pitch   [in]     pitch for moving to the next y line
> >> + *  @param  mc_type [in]     interpolation type
> >> + */
> >> +static void mc_8x8_delta(int16_t *buf, int16_t *ref_buf, uint32_t pitch, int mc_type)
> >> +{
> >> +    int     i, j;
> >> +    int16_t *wptr;
> >> +
> >> +    switch (mc_type) {
> >> +        case 0: /* fullpel (no interpolation) */
> >> +            for (i = 0; i < 8; i++, buf += pitch, ref_buf += pitch) {
> >> +                buf[0] += ref_buf[0];
> >> +                buf[1] += ref_buf[1];
> >> +                buf[2] += ref_buf[2];
> >> +                buf[3] += ref_buf[3];
> >> +                buf[4] += ref_buf[4];
> >> +                buf[5] += ref_buf[5];
> >> +                buf[6] += ref_buf[6];
> >> +                buf[7] += ref_buf[7];
> >> +            }
> >> +            break;
> >> +        case 1: /* horizontal halfpel interpolation */
> >> +            for (i = 0; i < 8; i++, buf += pitch, ref_buf += pitch) {
> >> +                for (j = 0; j < 8; j++)
> >> +                    buf[j] += (ref_buf[j] + ref_buf[j+1]) >> 1;
> >> +            }
> >> +            break;
> >> +        case 2: /* vertical halfpel interpolation */
> >> +            wptr = ref_buf + pitch;
> >> +            for (i = 0; i < 8; i++, buf += pitch, wptr += pitch, ref_buf += pitch) {
> >> +                for (j = 0; j < 8; j++)
> >> +                    buf[j] += (ref_buf[j] + wptr[j]) >> 1;
> >> +            }
> >> +            break;
> >> +        case 3: /* vertical and horizontal halfpel interpolation */
> >> +            wptr = ref_buf + pitch;
> >> +            for (i = 0; i < 8; i++, buf += pitch, wptr += pitch, ref_buf += pitch) {
> >> +                for (j = 0; j < 8; j++)
> >> +                    buf[j] += (ref_buf[j] + ref_buf[j+1] + wptr[j] + wptr[j+1]) >> 2;
> >> +            }
> >> +            break;
> >> +    }
> >> +}
> >>     
> >
> > so should MC
> >   
> 
> I'm sorry, how this comment should be interpreted?

like the previous you snipped, that is that this belongs in a seperate file.

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

I wish the Xiph folks would stop pretending they've got something they
do not.  Somehow I fear this will remain a wish. -- M?ns Rullg?rd
-------------- 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/20090407/a6caa503/attachment.pgp>