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

[Michael Niedermayer]

On Thu, Apr 30, 2009 at 12:26:52AM +0200, Adam Iglewski wrote:
> Hi,
>
> I'm sending updated patch as promised with changes addressing issues 
> pointed by Michael and Vitor and fixes as described in my previous email. 
> I've also added a check for unknown/unsupported 'action code'
> in which case reading from stream is stopped.
>

> 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



[...]

> +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 vector_index = 0;
> +
> +    blocks_wide = s->width >> 2;
> +    blocks_high = s->height / s->vector_height;
> +    block_inc = 4;
> +    frame_end = (uint16_t *)s->frame.data[0] + s->height * stride + s->width;
> +
> +    if (s->vector_height == 4)
> +        index_shift = 4;
> +    else
> +        index_shift = 3;
> +
> +    for(block_y=0; block_y < blocks_high; block_y ++) {
> +        pixels = (uint16_t *)s->frame.data[0] + (block_y * s->vector_height * stride);
> +
> +        for(block_x=0; block_x < blocks_wide; ) {
> +            int blocks_done;
> +            vptr_action_code = bytestream_get_le16(&src);
> +
> +            switch(vptr_action_code & 0xe000) {
> +
> +                /* Skip Count blocks. Count is (Val & 0x1fff). */
> +                case 0x0000:
> +                    blocks_done = vptr_action_code & 0x1fff;
> +                    block_x += blocks_done;
> +                    pixels += blocks_done * block_inc;
> +                    continue;
> +
> +                /* Write block number (Val & 0xff) Count times.
> +                 * Count is (((Val/256) & 0x1f)+1)*2
> +                 */
> +                case 0x2000:
> +                    vector_index = (vptr_action_code & 0xff) << index_shift;
> +                    blocks_done = (((vptr_action_code>>8) & 0x1f)+1) << 1;
> +                    break;
> +
> +                /* Write block number (Val & 0xff) and then write Count blocks
> +                 * getting their indexes by reading next Count bytes from
> +                 * the VPTR chunk. Count is (((Val/256) & 0x1f)+1)*2

defining what count is makes no sense its clear in the code already
actually i think the whole comments are useless


> +                 */
> +                case 0x4000:
> +                    vector_index = (vptr_action_code & 0xff) << index_shift;

> +                    blocks_done = ((((vptr_action_code>>8) & 0x1f)+1) << 1)+1;

this can be simplified


> +                    break;
> +

> +                /* Write block (Val & 0x1fff).*/
> +                case 0x6000:
> +                    vector_index = (vptr_action_code & 0x1fff) << index_shift;
> +                    blocks_done=1;
> +                    break;
> +
> +                /* Write block (Val & 0x1fff) Count times.
> +                 * Count is the next byte from the VPTR chunk
> +                 */
> +                case 0xa000:
> +                    vector_index = (vptr_action_code & 0x1fff) << index_shift;
> +                    blocks_done = *src++;
> +                    break;

can be merged


> +
> +                default:
> +                    av_log(s->avctx, AV_LOG_ERROR, " unknown action code in VPTR chunk (%x)\n",(vptr_action_code & 0xe000));
> +                    return;
> +            }
> +
> +            if(pixels + s->vector_height * stride + blocks_done * block_inc > frame_end) {
> +                av_log(s->avctx, AV_LOG_ERROR, " too many blocks in frame.\n");
> +                return;
> +            }
> +
> +            for(i=0; i < blocks_done; i++) {
> +                if(i && ((vptr_action_code & 0xe000) == 0x4000)) {

> +                    vector_index = *src++;
> +                    vector_index <<= index_shift;

can be merged

[...]
> @@ -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


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

The educated differ from the uneducated as much as the living from the
dead. -- Aristotle 
-------------- 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/20090430/74ac6848/attachment.pgp>