[FFmpeg-devel] [PATCH] Indeo5 decoder

[Maxim]

Michael Niedermayer schrieb:
> On Mon, Apr 06, 2009 at 08:41:57PM +0200, Maxim wrote:
>   
>> Michael Niedermayer schrieb:
>>     
>>> On Wed, Apr 01, 2009 at 02:43:04AM +0200, Maxim wrote:
>>>   
>>>       
>>>> Diego Biurrun schrieb:
>>>>     
>>>>         
>>>>> Have you checked this works standalone, i.e. there are no other
>>>>> dependencies?
>>>>>
>>>>> Try disabling all decoders and encoders and just enabling indeo5 please.
>>>>>   
>>>>>       
>>>>>           
>>>> Yes, it does...
>>>>
>>>> Attached is an updated patch that fixes all things you pointed to.
>>>> The following things were changed as well:
>>>> - the inverse transform contains algebraic simplifications showed by
>>>> Michael in his earlier post
>>>> - a step was added to the inverse transform that tests for col/rows
>>>> containing zero coeffs and sets the corresponding output to zero instead
>>>> of performing the inverse transform on zeroes
>>>> - simplifications of the motion compensation functions
>>>>
>>>> Please any further reviews (Michael?)
>>>>     
>>>>         
>> An updated patch attached fixing the most things showed by patcheck...
>>
>>
>>     

[...]

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

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

[...]

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

Regards
Maxim