[FFmpeg-devel] [PATCH] Kega Game Video (KGV1) decoder

[Michael Niedermayer]

On Tue, Mar 02, 2010 at 11:46:38AM -0600, Daniel Verkamp wrote:
> On Wed, Feb 3, 2010 at 9:02 PM, Michael Niedermayer <michaelni at gmx.at> wrote:
> > On Wed, Feb 03, 2010 at 11:49:32AM -0600, Daniel Verkamp wrote:
> >> On Wed, Feb 3, 2010 at 10:41 AM, Daniel Verkamp <daniel at drv.nu> wrote:
> >> > Hi,
> >> >
> >> > This is a video capture codec used by the Kega emulator.
> >> >
> >> > See http://wiki.multimedia.cx/index.php?title=Kega_Video for more information.
> >> >
> >> > Thanks,
> >> > -- Daniel Verkamp
> >> >
> >>
> >> Slightly changed version which (hopefully) fixes endianness issues;
> >> still only tested on LE.
> >>
> > [...]
> > are you missing checks for the outpt array size, if so please double
> > check your code that there are no further such issues and also try a
> > fuzzer
> >
> 
> Previous version was relying on padded output buffers for this
> purpose, but on closer investigation, this was insufficient anyway.
> New version has checks for every array/pointer dereference based on
> input data.
> 
> > [...]
> >> + ? ?dst = c->pic.data[0];
> >> + ? ?stride = c->pic.linesize[0];
> >> + ? ?for (i = 0; i < h; i++)
> >> + ? ? ? ?memcpy(dst + i * stride, c->cur + i * w, w * 2);
> >
> > return your internal buffer dont copy and disable CODEC_CAP_DR1
> >
> 
> I have attempted to do this, and it seems to work with ffmpeg and
> ffplay, but I couldn't find any other codecs that work this way, so it
> is mostly trial and error - please double check! Am I correct in
> assuming I can just set the output AVFrame's data[0] and linesize[0]
> (no get_buffer() use at all), or are there additional required fields?



>  The doxygen for AVCodec.decode is nonexistent, so it is not
> particularly clear.

improcing the dox is welcoeme


[...]
> +
> +static int decode_frame(AVCodecContext *avctx, void *data, int *data_size, AVPacket *avpkt)
> +{
> +    const uint8_t *buf = avpkt->data;
> +    const uint8_t *buf_end = buf + avpkt->size;
> +    KgvContext * const c = avctx->priv_data;
> +    int offsets[7];
> +    uint16_t *out, *prev;
> +    int outcnt = 0, maxcnt;
> +    int w, h, i;
> +
> +    if (avpkt->size < 2)
> +        return -1;
> +
> +    w = (buf[0] + 1) * 8;
> +    h = (buf[1] + 1) * 8;
> +    buf += 2;
> +
> +    if (avcodec_check_dimensions(avctx, w, h))
> +        return -1;
> +
> +    if (w != avctx->width || h != avctx->height)
> +        avcodec_set_dimensions(avctx, w, h);
> +
> +    maxcnt = w * h;
> +

> +    c->pic.buffer_hints = FF_BUFFER_HINTS_VALID | FF_BUFFER_HINTS_PRESERVE
> +                        | FF_BUFFER_HINTS_REUSABLE;

that should have no effect if you dont use *get_buffer()


> +
> +    out = av_realloc(c->cur, w * h * 2);
> +    if (!out)
> +        return -1;
> +    c->cur = out;
> +
> +    prev = av_realloc(c->prev, w * h * 2);
> +    if (!prev)
> +        return -1;
> +    c->prev = prev;
> +
> +    for (i = 0; i < 7; i++)
> +        offsets[i] = -1;
> +
> +    while (outcnt < maxcnt && buf_end - 2 > buf) {
> +        int code = AV_RL16(buf);
> +        buf += 2;
> +        if (!(code & 0x8000)) {
> +            out[outcnt++] = code; // rgb555 pixel coded directly
> +        } else {
> +            if ((code & 0x6000) == 0x6000) {
> +                // copy from previous frame
> +                int count = (code & 0x3FF) + 3;
> +                int oidx  = (code >>   10) & 7;
> +                int start;
> +
> +                if (offsets[oidx] == -1) {
> +                    if (buf_end - 3 < buf)
> +                        break;

> +                    offsets[oidx] = AV_RL24(buf);
> +                    if (offsets[oidx] < 0) {

how can a 24bit number produce a negative int ?


> +                        av_log(avctx, AV_LOG_ERROR, "negative offset\n");
> +                        offsets[oidx] = 0;
> +                        break;
> +                    }
> +                    buf += 3;
> +                }
> +
> +                start = (outcnt + offsets[oidx]) % maxcnt;
> +
> +                if (maxcnt - outcnt < count || maxcnt - start < count)
> +                    break;
> +
> +                for (i = 0; i < count; i++)
> +                    out[outcnt++] = prev[start + i];
> +
> +            } else {
> +                // copy from earlier in this frame
> +                int count;
> +                int offset = (code & 0x1FFF) + 1;
> +                uint16_t *inp;
> +                if (!(code & 0x6000)) {
> +                    count = 2;
> +                } else if ((code & 0x6000) == 0x2000) {
> +                    count = 3;
> +                } else {
> +                    if (buf_end - 1 < buf)
> +                        break;
> +                    count = *buf++ + 4;
> +                }
> +
> +                if (outcnt - offset < 0 || maxcnt - outcnt < count)
> +                    break;
> +
> +                inp = out + outcnt - offset;
> +                for (i = 0; i < count; i++)
> +                    out[outcnt++] = inp[i];
> +            }

these 2 look quite similar, i suspect they can be simplified by merging
them

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Everything should be made as simple as possible, but not simpler.
-- Albert Einstein
-------------- 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/20100302/80f2d585/attachment.pgp>