[FFmpeg-devel] [PATCH] VQA-highcolor (15 bit rgb555) decoder by Adam Iglewski

[Michael Niedermayer]

On Mon, Jan 13, 2014 at 10:28:30AM +0000, u-owvm at aetey.se wrote:
> Hello,
> 
> Adjusted to the current master:
> 
> VQA-highcolor (15 bit rgb555) decoder based on patches from June 2009
> by Adam Iglewski <szwagros at szwagros-desktop.(none)>.
> 
> Hope it may make it through this time.
> 
> Regards,
> Rl

[...]
> @@ -155,6 +154,11 @@ static av_cold int vqa_decode_init(AVCodecContext *avctx)
>      s->vector_height = s->avctx->extradata[11];
>      s->partial_count = s->partial_countdown = s->avctx->extradata[13];
>  
> +    if(!AV_RL16(&s->avctx->extradata[14]))
> +        avctx->pix_fmt = PIX_FMT_RGB555;
> +    else
> +        avctx->pix_fmt = PIX_FMT_PAL8;

AV_PIX_FMT_*

also this patch doesnt work on big endian (yes i tested it)


[...]
> +static void vqa_decode_hc_video_chunk(VqaContext *s, const unsigned char *src,
> +                                      unsigned int src_size, AVFrame *frame)
> +{
> +    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 = frame->linesize[0] >> 1;
> +
> +    int type,code;
> +    int vector_index = 0;
> +
> +    blocks_wide = s->width >> 2;
> +    blocks_high = s->height / s->vector_height;
> +    block_inc = 4;
> +    frame_end = (uint16_t *)frame->data[0] + s->height * stride + s->width;

stride can be negative, which would break this code


> +
> +    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 *)frame->data[0] + (block_y * s->vector_height * stride);
> +
> +        for(block_x=0; block_x < blocks_wide; ) {
> +            int blocks_done;
> +            code = bytestream_get_le16(&src);
> +            type = code >> 13;
> +            code &= 0x1fff;
> +
> +            if(!type) {
> +                    blocks_done = code;
> +                    block_x += blocks_done;
> +                    pixels += blocks_done * block_inc;
> +                    continue;
> +            } else if (type < 3) {
> +                    vector_index = (code & 0xff) << index_shift;
> +                    blocks_done = ((code & 0x1f00) >> 7) + 1 + type;
> +            } else if (type == 3 || type == 5) {
> +                    vector_index = code << index_shift;
> +                    if (type == 3)
> +                        blocks_done = 1;
> +                    else
> +                        blocks_done = *src++;
> +            } else {
> +                    av_log(s->avctx, AV_LOG_ERROR, " unknown type in VPTR chunk (%d)\n",type);
> +                    return;

this should return some error code like AVERROR_INVALIDDATA or AVERROR_PATCHWELCOME


> +            }
> +
> +            if(pixels + s->vector_height * stride + blocks_done * block_inc > frame_end) {  

this can overflow, consider that the frame would be at  the end of
the address space so pixels + some big number would return a small
pointer


[...]
> @@ -173,12 +183,39 @@ static int wsvqa_read_packet(AVFormatContext *s,
>  
>          skip_byte = chunk_size & 0x01;
>  
> -        if ((chunk_type == SND0_TAG) || (chunk_type == SND1_TAG) ||
> -            (chunk_type == SND2_TAG) || (chunk_type == VQFR_TAG)) {
> +        if (chunk_type == VQFL_TAG) {
> + 
> +            wsvqa->vqfl_chunk_size = chunk_size;
> +            wsvqa->vqfl_chunk_data = av_mallocz(chunk_size);
> +            if (!wsvqa->vqfl_chunk_data)
> +                return AVERROR(ENOMEM);
> +            ret = avio_read(pb, wsvqa->vqfl_chunk_data, chunk_size);
> +            if (ret != chunk_size)
> +                return AVERROR(EIO);
> +            if (skip_byte)
> +                avio_seek(pb, 1, SEEK_CUR);
> +            continue;
> +
> +        } else if ((chunk_type == SND0_TAG) || (chunk_type == SND1_TAG) ||
> +                   (chunk_type == SND2_TAG) || (chunk_type == VQFR_TAG)) {
> +
> +            if ((chunk_type == VQFR_TAG) && wsvqa->vqfl_chunk_size) {

> +                if (av_new_packet(pkt, chunk_size + wsvqa->vqfl_chunk_size))

the addition could overflow, allocating a too small packet


> +                    return AVERROR(EIO);
> +                ret = avio_read(pb, pkt->data, chunk_size);
> +                if (ret != chunk_size) {
> +                    av_free_packet(pkt);
> +                    return AVERROR(EIO);
> +                }
> +                memcpy(pkt->data + chunk_size,wsvqa->vqfl_chunk_data,wsvqa->vqfl_chunk_size);
> +                wsvqa->vqfl_chunk_size=0;

> +                av_free(wsvqa->vqfl_chunk_data);

av_freep() is safer to ensure that no stale pointers remain

[...]


-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Its not that you shouldnt use gotos but rather that you should write
readable code and code with gotos often but not always is less readable
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20140114/bcbb0ec3/attachment.asc>