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

[Vitor Sessak]

Adam Iglewski wrote:
> Hi,
> 
> Sorry for the delay. Updated (and improved I hope) patch attached.
> 
> Michael Niedermayer pisze:
> [...]
>>>>>  +static inline void vqa_copy_hc_block(unsigned short *pixels,int 
>>>>> pixel_ptr,int stride,unsigned short *codebook,
>>>>> +                                      int vector_index,int block_h)
>>>>> +{
>>>>> +    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++];
>>>>> +        pixels[pixel_ptr+2] = codebook[vector_index++];
>>>>> +        pixels[pixel_ptr+3] = codebook[vector_index++];
>>>>> +        pixel_ptr += stride;
>>>>> +    }
>>>>> +}
>>>> vector_index is redundant, so is pixel_ptr
>>> Fixed. And as Vitor suggested now using memcpy.
>>
>> memcpy might be a bad choice, did you benchmark this one?
>> personally id cast to uint64_t and do a single assignment ...
>>
> 
> I did benchmark by putting START_TIMER after opening brace
> and STOP_TIMER("vqa_copy_hc_block") just before closing brace
> of this function. I hope this is the correct way to benchmark
> things in FFmpeg? If true it looks like memcpy solution is
> fastest.

I gave a look at it and gcc optimized the memcpy for a 64-bit assignment...

 > But I couldn't find any in VQA samples directory. Assuming that such
 > files do exists what is the correct way to fix this? Adding new codec
 > id? If not - how demuxer can pass some additional info to codec? Using
 > extradata field? In this case it would be simple flag meaning that
 > stereo samples are ordered in one way or the other.

According to libavformat/apc.c, samples at 
http://samples.mplayerhq.hu/game-formats/cryo/ should be using 
CODEC_ID_IMA_WS too...

> --- ffmpeg/libavcodec/vqavideo.c	2009-04-20 20:37:46.000000000 +0200
> +++ ffmpeg_work/libavcodec/vqavideo.c	2009-04-27 21:27:32.000000000 +0200
> @@ -70,6 +70,7 @@

A few nitpicks...

> +
> +    for(block_y=0; block_y < blocks_high; block_y ++)
> +    {

inconsistent brace placement

> +        pixels = (uint16_t *)s->frame.data[0] + (block_y * s->vector_height * stride);
> +        for(block_x=0; block_x < blocks_wide; )
> +        {

ditto

> +            if(pixels + s->vector_height * stride + blocks_done * block_inc > frame_end) {
> +                av_log(s->avctx, AV_LOG_ERROR, "  VQA video: too many blocks in frame.\n");

There is no point in adding "VQA video: ", there is already the context 
for that (yes, the whole file uses it, patch welcome).

-Vitor