[Ffmpeg-devel] [PATCH] C93 demuxer and decoder (GSoC qualification task)

[Michael Niedermayer]

Hi

On Thu, Apr 05, 2007 at 07:19:19PM +0300, Anssi Hannula wrote:
> Anssi Hannula wrote:
> > As my Google Summer of Code qualification task, attached is a patch
> > which adds C93 [1] demuxer and video decoder to FFmpeg. The samples from
> > samples.mplayerhq.hu can be properly played out with it.
> > 
> > I'm not too familiar with FFmpeg codebase yet, though, so there could be
> > some silly mistakes in the code ;). Please review it.
> > 
> > [1] http://wiki.multimedia.cx/index.php?title=C93
> 
> I had some time today, so I implemented the suggestions... Attached is a
> new version of the patch.
> 
> In addition to the suggested ones, I also made these changes:
> - moved addition of audio streams to c93_read_packet() as some .c93
> files do not have audio
> - set time_base of AVStream instead of AVCodecContext's, and set
> duration and start_time in AVStream, so that correct file duration is
> reported
> - always allocate the AVPacket with space for palette to avoid seeking
> backwards
> - fix palette endianness
> - some cleaning of the non-predictive modes
[...]
> +static inline int c93_copy_block(AVCodecContext *avctx, uint8_t *to,
> +        uint8_t *from, int offset, int height, int to_x, int to_y, int stride)
> +{
> +    int i;
> +    int width = height;
> +    int from_x = offset % WIDTH;
> +    int from_y = offset / WIDTH;
> +    int overflow = from_x + width - WIDTH;
> +
> +    if (!from) {
> +        /* silently ignoring predictive blocks in first frame */
> +        return 0;
> +    }
> +
> +    if (overflow >= width || from_y + height > HEIGHT) {

how can overflow >= width happen?


> +        av_log(avctx, AV_LOG_ERROR, "invalid offset %d during C93 decoding\n",
> +               offset);
> +        return -1;
> +    }
> +
> +    if (overflow > 0) {
> +        width -= overflow;
> +        for (i = 0; i < height; i++) {
> +            memcpy(&to[(to_y+i)*stride+to_x+width], &from[(from_y+i)*stride],
> +                   overflow);
> +        }

are you sure its not reading from the next line in case of overflow?


> +    }
> +
> +    for (i = 0; i < height; i++) {
> +        memcpy(&to[(to_y+i)*stride+to_x], &from[(from_y+i)*stride+from_x],
> +               width);
> +    }

the effects of to_y and to_x can not only be moved out of the loops but even
out of this function


[...]
> +    if (*buf++ & C93_HAS_PALETTE) {
> +        uint32_t *palette = (uint32_t *) newpic->data[1];
> +        c93->palette_changed = 1;
> +        for (i = 0; i < 256; i++) {
> +            palette[i] = bytestream_get_be24(&buf);
> +        }
> +    } else {
> +        buf += 256 * 3;
> +        if (c93->palette_changed) {
> +            memcpy(newpic->data[1], oldpic->data[1], 256 * 4);
> +            c93->palette_changed = 0;
> +        }

what is the palette_changed code good for?


[...]
> +                        for (i = 0; i < 4; i++)
> +                            cols[i] = *buf++;

bytestream_get_buffer()


[...]
> +    ret = av_new_packet(pkt, datasize + 768 + 1);
> +    if (ret < 0)
> +        return ret;
> +    pkt->data[0] = 0;
> +
> +    ret = get_buffer(pb, pkt->data + 768 + 1, datasize);
> +    if (ret < datasize) {
> +        av_free_packet(pkt);
> +        return AVERROR_IO;
> +    }
> +
> +    datasize = get_le16(pb); /* palette size */
> +    if (datasize) {
> +        if (datasize != 768) {
> +            av_log(s, AV_LOG_ERROR, "invalid palette size %u\n", datasize);
> +            av_free_packet(pkt);
> +            return AVERROR_INVALIDDATA;
> +        }
> +        pkt->data[0] |= C93_HAS_PALETTE;
> +        ret = get_buffer(pb, pkt->data + 1, datasize);
> +        if (ret < datasize) {
> +            av_free_packet(pkt);
> +            return AVERROR_IO;
> +        }
> +    }

if there is no palette pkt->size should be decreased by 768


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

No human being will ever know the Truth, for even if they happen to say it
by chance, they would not even known they had done so. -- Xenophanes
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20070405/e8d469a5/attachment.pgp>