[FFmpeg-devel] [PATCH] Unfinished GSoC qual task (16bit VQA Video Decoder)

[Michael Niedermayer]

On Sat, May 02, 2009 at 01:06:24AM +0200, Adam Iglewski wrote:
> Michael Niedermayer pisze:
> [...]
>>> I would be grateful for hints how to resolve problem with
>>> decoding stereo audio (also described in my previous email).
>> there are 3 options (in no particular order)
>> A. extradata (this requires that there really is a global header in the
>>    file for audio)
>> B. codec_tag (this requres that there really is a codec tag for the
>>    audio)
>> C. a seperate codec_id
>
> I think option C is the simplest. It requires only small changes
> and adds almost no new code to FFmpeg:
>
> --- ffmpeg/libavcodec/adpcm.c	2009-04-29 23:54:38.000000000 +0200
> +++ ffmpeg_work/libavcodec/adpcm.c	2009-05-02 00:30:17.000000000 +0200
> @@ -1005,6 +1005,7 @@ static int adpcm_decode_frame(AVCodecCon
>          if (cs->step_index < 0) cs->step_index = 0;
>          if (cs->step_index > 88) cs->step_index = 88;
>
> +    case CODEC_ID_ADPCM_IMA_WS_V3:
>          m= (buf_size - (src - buf))>>st;
>          for(i=0; i<m; i++) {
>              *samples++ = adpcm_ima_expand_nibble(&c->status[0], src[i] & 
> 0x0F, 4);
>
> and
>
> --- ffmpeg/libavformat/westwood.c	2009-04-20 18:35:42.000000000 +0200
> +++ ffmpeg_work/libavformat/westwood.c	2009-05-02 00:28:54.000000000 +0200
>
> @@ -254,8 +258,10 @@ static int wsvqa_read_header(AVFormatCon
>          st->codec->codec_type = CODEC_TYPE_AUDIO;
>          if (AV_RL16(&header[0]) == 1)
>              st->codec->codec_id = CODEC_ID_WESTWOOD_SND1;
> -        else
> +        else if(AV_RL16(&header[14]))
>              st->codec->codec_id = CODEC_ID_ADPCM_IMA_WS;
> +        else
> +            st->codec->codec_id = CODEC_ID_ADPCM_IMA_WS_V3;
>
> plus of course adding codec_id to avcodec.h etc.
> Is this option acceptable?

yes


[...]
>> [...]
>>> @@ -561,6 +698,20 @@ static void vqa_decode_chunk(VqaContext              
>>> s->partial_countdown = s->partial_count;
>>>          }
>>>      }
>>> +
>>> +    if(vptr_chunk != -1) {
>>> +        chunk_size = AV_RB32(&s->buf[vptr_chunk + 4]);
>> validity check missing
>>> +        vptr_chunk += CHUNK_PREAMBLE_SIZE;
>>> +        vqa_decode_hc_video_chunk(s,&s->buf[vptr_chunk],chunk_size);
>>> +    }
>>> +
>>> +    if(vprz_chunk != -1) {
>>> +        chunk_size = AV_RB32(&s->buf[vprz_chunk + 4]);
>> same
>> also the code likely can be factorized
>
> By validity check you mean checking that AV_RB32 is not
> reading outside s->buf or checking if chunk_size isn't
> bigger that s->size ? I'm asking because there is a lot
> of similar code in this function/file and there are
> no checks like this.

all things that could segfault should be checked to avoid possible
segfaults. The exception would just be code where such checks would
cause a noticeable slowdown.


>
> As for refactoring this is part of the function that you
> wanted to be rewritten to process chunks as needed.
> So maybe for now this could be as is?

only if you promisse to do that rewrite afterwards.


[...]
> @@ -291,6 +308,95 @@ static void decode_format80(const unsign
>                  dest_index, dest_size);
>  }
>  
> +static inline void vqa_copy_hc_block(uint16_t *pixels,int stride,const uint16_t *codebook,
> +                                      int block_h)
> +{

> +    int pixel_y;
> +    for (pixel_y = 0; pixel_y < block_h; pixel_y++) {

while(block_h--){


> +        memcpy(pixels,codebook,8);
> +        pixels += stride;
> +        codebook += 4;
> +    }
> +}
> +
> +static void vqa_decode_hc_video_chunk(VqaContext *s,const unsigned char *src,unsigned int src_size)
> +{
> +    int block_x, block_y;          /* block width and height iterators */
> +    int blocks_wide, blocks_high;  /* width and height in 4x4|2 blocks */
> +    int block_inc;
> +    int index_shift;
> +    int i;
> +
> +    /* decoding parameters */
> +    uint16_t *pixels,*frame_end;
> +    uint16_t *codebook = (uint16_t *)s->codebook;
> +    int stride = s->frame.linesize[0] >> 1;
> +

> +    int vptr_action_code;

int type is shorter and does not seem worse


[...]
> +                case 0x0000:
> +                    blocks_done = vptr_action_code & 0x1fff;
> +                    block_x += blocks_done;
> +                    pixels += blocks_done * block_inc;
> +                    continue;
> +
> +                case 0x2000:
> +                case 0x4000:
> +                    vector_index = (vptr_action_code & 0xff) << index_shift;
> +                    blocks_done = ((vptr_action_code & 0x1f00)+0x0100) >> 7;
> +                    if((vptr_action_code & 0xe000)==0x4000)
> +                        blocks_done++;
> +                    break;
> +
> +                case 0x6000:
> +                    blocks_done=1;
> +                case 0xa000:
> +                    vector_index = (vptr_action_code & 0x1fff) << index_shift;
> +                    if((vptr_action_code & 0xe000)==0xa000)
> +                        blocks_done = *src++;
> +                    break;

if(type==0){
   ...
}else if(type < 3){
    vector_index = (code & 0xff) << index_shift;
    blocks_done  = (code>>7) + 1 + type;
}else if(type==3 || type==5){
    vector_index =  code         << index_shift;
    if(type==3) blocks_done= 1;
    else        blocks_done= *src++;
}


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

He who knows, does not speak. He who speaks, does not know. -- Lao Tsu
-------------- 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/20090505/307b6932/attachment.pgp>