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

[Vitor Sessak]

Adam Iglewski wrote:
> Hi,
> 
> I was tracking (I really wanted to see videos from BladeRunner again :)) 
> the progress of this task and it looks like The Deep Explorer finally 
> gave up. I had some spare time so I thought I will give a try. So here 
> it is.

Nice! I'll give a quick look at it, before you get a full review...

> I'm sending 3 patches:
> 
> add_vqa_hc - adds decoding of HC video chunks. I'm not sure about parts
> that I added in vqa_decode_frame function. I don't know well
> ffmpeg internals so I based my changes on msvideo1 decoder.
> 
> mod_westwood_dmux - modifies demuxer to recognize new chunks in hc files
> 
> fix_adpcm_ws - fixes decoding of stereo audio.
> 
> Non patched ffmpeg plays distorted sound on files with 2 channels. 
> Description of SND2 chunk
> 
> http://wiki.multimedia.cx/index.php?title=VQA#SND2_chunk
> 
> says that in stereo files first half of chunk is for left channel and
> second half for right channel but the code doesn't match the
> description. But code for decoding nibbles for codec with id
> CODEC_ID_ADPCM_4XM does. So I just replaced relevant parts and now it
> plays fine 

Duplicating 4XM code is not ok (you could archive the same result of 
your patch just putting "case CODEC_ID_ADPCM_IMA_WS:" inside the 4XM 
case block). Also, did you test your code with our samples archive? It 
would surprise me that CODEC_ID_ADPCM_IMA_WS never worked for any file...

> +static inline void vqa_copy_hc_block(unsigned short *pixels,int pixel_ptr,int stride,unsigned short *codebook,
> +                                      int vector_index,int block_h)

I prefer using uint16_t instead of "unsigned short", since what we 
actually want is a 16-bit uint...

> +{
> +    int pixel_y;
> +    for (pixel_y = 0; pixel_y < block_h; pixel_y++) {
> +        pixels[pixel_ptr] = codebook[vector_index++];
> +        pixels[pixel_ptr+1] = codebook[vector_index++];

If you add one more whitespace it is more readable:

         pixels[pixel_ptr  ] = codebook[vector_index++];
         pixels[pixel_ptr+1] = codebook[vector_index++];

but I think here one memcpy would be even better...

> +
> +        pixel_ptr = ((block_y * s->vector_height) * stride)+block_x*block_inc;
> +        vptr_action_code = AV_RL16(&src[src_ptr]);
> +        src_ptr+=2;

Instead of using src_ptr and constructs like src[src_ptr++], you could 
just increment the src pointer. More precisely, instead of

   a= src[src_ptr++];

just

   a = *src++;

and instead of

   a= AV_RL16(&src[src_ptr]);
   src_ptr+=2;

just

   a = bytestream_get_le16(&src);

Also please read your code again to check if a malicious malformed VQA 
file would not make your code write to unallocated memory. This would be 
a serious security breach...

-Vitor