[FFmpeg-devel] [PATCH] Indeo5 decoder

[Maxim]

Michael Niedermayer schrieb:
> 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?
>   

It provides a possibility to calculate the scalefactors using integer
operations without floatig-point math and it's therefore faster...
Do you see any possibility to simplify that?

>>>> +
>>>> +
>>>> +/**
>>>> + *  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
>   

Ok, I'm agree with it...

>
>   
>> [...]
>>
>>     
>>>> +/**
>>>> + *  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.
>   

Aha, ok. Should I move them to "dsputils"?

Regards
Maxim