[Ffmpeg-devel] [PATCH] Tiertex .SEQ files support

[Michael Niedermayer]

Hi

On Sun, Sep 17, 2006 at 02:32:12PM +0200, Gregory Montoir wrote:
> 
> Hi,
> 
> The attached patch adds support for Tiertex .seq files demuxer / video
> decoding. As far as I know, this file format was only used in the DOS
> CDROM version of the game Flashback [1,2].
> 
> If there's interest, I can upload samples files for testing and/or
> reference.
> 
> Regards,
> Gregory
> 
> [1]
> http://www.mobygames.com/game/dos/flashback-the-quest-for-identity/screenshots/gameShotId,74055/
> [2]
> http://www.mobygames.com/game/dos/flashback-the-quest-for-identity/screenshots/gameShotId,74057/
> 

[...]
> +static unsigned char *seq_decode_unpack_rle_frame(unsigned char *src, unsigned char *dst)
> +{
> +    static const int code_table[] = { 0, 1, 2, 3, 4, 5, 6, 7, -8, -7, -6, -5, -4, -3, -2, -1 };
> +
> +    int i, sz;
> +    int code;
> +    int rle_code_table[65];
> +
> +    i = 0;
> +    sz = 0;
> +    do {
> +        code = *src++;
> +        rle_code_table[i++] = code_table[code & 15];
> +        sz += ABS(code_table[code & 15]);
> +        rle_code_table[i++] = code_table[code >> 4];
> +        sz += ABS(code_table[code >> 4]);
> +    } while (sz < 64);
> +    rle_code_table[i] = 0;

this code looks exploitable
and it seems this is equivakent to a loop with get_sbits() which would be 
much simpler


> +
> +    for (i = 0; rle_code_table[i] != 0; ++i) {
> +        code = rle_code_table[i];
> +        if (code < 0) {
> +            code = -code;
> +            memset(dst, *src++, code);
> +            dst += code;
> +        } else {
> +            memcpy(dst, src, code);
> +            dst += code;
> +            src += code;
> +        }
> +    }

this also looks exploitable


[...]
> +    if (flags & 2) {
> +        for (i = 0; i < 64; ++i) {
> +            code = LE_16(data); data += 2;
> +            for (b = 0; b < 8; ++b) {
> +                seq->operations_table[i * 8 + b] = code & 3;
> +                code >>= 2;
> +            }

this should use the bitstream reader too


[...]
> +static int seqvideo_decode_init(AVCodecContext *avctx)
> +{
> +    SeqVideoContext *seq = (SeqVideoContext *)avctx->priv_data;
> +
> +    seq->avctx = avctx;
> +    avctx->pix_fmt = PIX_FMT_PAL8;
> +    avctx->has_b_frames = 0;
> +
> +    seq->current_frame.data[0] = NULL;
> +    seq->previous_frame.data[0] = NULL;
> +
> +    memset(seq->palette, 0, sizeof(seq->palette));
> +    memset(seq->operations_table, 0, sizeof(seq->operations_table));
> +    memset(seq->unpack_buffer, 0, sizeof(seq->unpack_buffer));

you can generally assume that things are set to zero if they where allocated
by lavc, and if you allocate them then allocate them with av_mallocz() instead
of doing a memset(0) afterwards


> +
> +    return 0;
> +}
> +
> +static int seqvideo_decode_frame(AVCodecContext *avctx,
> +                                 void *data, int *data_size,
> +                                 uint8_t *buf, int buf_size)
> +{
> +
> +    SeqVideoContext *seq = (SeqVideoContext *)avctx->priv_data;
> +
> +    seq->current_frame.reference = 1;
> +    if (avctx->get_buffer(avctx, &seq->current_frame)) {
> +        av_log(seq->avctx, AV_LOG_ERROR, "tiertexseqvideo: get_buffer() failed to allocate a frame\n");
> +        return -1;
> +    }
> +
> +    if (seq->previous_frame.data[0])
> +        memcpy(seq->current_frame.data[0],
> +          seq->previous_frame.data[0],
> +          seq->avctx->height * seq->current_frame.linesize[0]);

you should set the correct FF_BUFFER_HINTS* then this memcpy shouldnt
be needed


[...]
> +static int seq_init_frame_buffers(SeqDemuxContext *seq, const unsigned char *frame_data)
> +{
> +    int i, sz;
> +    TiertexSeqFrameBuffer *seq_buffer;
> +
> +    frame_data += 256;
> +
> +    memset(seq->frame_buffers, 0, sizeof(seq->frame_buffers));

i think this is unneeded


[...]

> +static int seq_read_header(AVFormatContext *s, AVFormatParameters *ap)
> +{
> +    int i, rc;
> +    SeqDemuxContext *seq = (SeqDemuxContext *)s->priv_data;
> +    ByteIOContext *pb = &s->pb;
> +    AVStream *st;
> +
> +    if (get_buffer(pb, seq->current_frame_data, SEQ_FRAME_SIZE) != SEQ_FRAME_SIZE)
> +        return AVERROR_IO;

why do you read the data into a buffer instead of using get_le*() everywhere?
if theres no reason then please change it to use get_le*()


[...]

> +    av_set_pts_info(st, 33, 1, 90000);
[...]
> +    av_set_pts_info(st, 33, 1, 90000);

this is wrong, set the correct timebase


[...]
> +    if (seq->audio_buffer_full) {
> +           if (av_new_packet(pkt, seq->current_audio_pkt_size))
> +               return AVERROR_NOMEM;
> +
> +        pkt->stream_index = seq->audio_stream_index;

messed up indention

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

In the past you could go to a library and read, borrow or copy any book
Today you'd get arrested for mere telling someone where the library is