[FFmpeg-devel] [PATCH] Psygnosis YOP demuxer/decoder

[Michael Niedermayer]

On Sun, Aug 09, 2009 at 11:54:36AM -0400, Thomas Higdon wrote:
> Here is the decoder plus the latest version of the demuxer with
> current comments addressed.

[...]

> +/**
> + * Lookup table for painting macroblocks. Bytes 0-3 of each entry contain
> + * the macroblock positions to be painted. Byte 4 contains the number of bytes
> + * consumed on the input, equal to max(bytes 0-3) + 1.
> + */
> +int paint_lut[15][5] = { {0, 1, 2, 3, 4},

uint8_t


[...]
> +static av_cold int decode_init(AVCodecContext *avctx)
> +{
> +    YopDecContext *s = avctx->priv_data;
> +    int i,j;
> +
> +    s->avctx = avctx;
> +
> +    if (avctx->width & 1 || avctx->height & 1) {
> +        av_log(avctx, AV_LOG_ERROR, "yop needs even width and height\n");
> +        return -1;
> +    }
> +

> +    if (avcodec_check_dimensions(avctx, avctx->width, avctx->height) < 0) {
> +        return -1;
> +    }

you should only need that if you set width/height


> +
> +    avctx->pix_fmt = PIX_FMT_RGB24;
> +

> +    for (i = 0; i < 256; i++) {
> +        for (j = 0; j < RGB_COLORS; j++) {
> +            s->palette[i][j] = 0;
> +        }
> +    }

memset


> +
> +    s->num_pal_colors = AV_RL8(avctx->extradata);
> +    s->firstcolor_even = AV_RL8(avctx->extradata + 1);
> +    s->firstcolor_odd = AV_RL8(avctx->extradata + 2);
> +    s->sound_chunk_length = AV_RL16(avctx->extradata + 3);

vertical align


> +    s->tag_needs_next_byte = 1;
> +
> +    if (s->num_pal_colors + s->firstcolor_even > 256 ||
> +        s->num_pal_colors + s->firstcolor_odd > 256) {
> +        av_log(avctx, AV_LOG_ERROR,
> +               "palette parameters invalid: header probably corrupt\n");
> +    }

you probably want a return -1 here


> +
> +    return 0;
> +}
> +
> +
> +static av_cold int decode_close(AVCodecContext *avctx)
> +{
> +    YopDecContext *s = avctx->priv_data;
> +    if (s->frame.data[0]) {
> +        avctx->release_buffer(avctx, &s->frame);
> +    }
> +    return 0;
> +}
> +
> +
> +/**
> + * Calculate the buffer offset necessary to index into the destination
> + * buffer at a certain x and y offset from bufpos. This function wraps around
> + * in both the x and y directions.
> + * @param s context
> + * @param bufpos the current position in the destination buffer. Must be a
> + *    valid buffer position that's within the image frame.
> + * @param dx change in x
> + * @param dy change in y
> + */
> +static int buffer_pos(YopDecContext *s, int bufpos, int dx, int dy)
> +{
> +    int new_x, new_y;
> +    int new_bufpos;
> +
> +    int cur_x = bufpos % s->frame.linesize[0];
> +    int cur_y = bufpos / s->frame.linesize[0];
> +

> +    if (cur_x > s->avctx->width * RGB_COLORS) {
> +        av_log(0, AV_LOG_ERROR, "cur_x: %d beyond image edge\n", cur_x);
> +    }
> +    if (cur_y > s->avctx->height) {
> +        av_log(0, AV_LOG_ERROR, "cur_y: %d beyond image edge\n", cur_y);
> +    }

how can these happen?


> +
> +    new_x = (cur_x + dx) % (s->avctx->width * RGB_COLORS);
> +    new_y = (cur_y + dy) % s->avctx->height;
> +
> +    if (new_x < 0) {
> +        new_x += s->avctx->width * RGB_COLORS;
> +    }
> +    if (new_y < 0) {
> +        new_y += s->avctx->height;
> +    }
> +    new_bufpos = new_x + new_y * s->frame.linesize[0];
> +
> +    // Return the relative offset to bufpos rather than the absolute offset
> +    // within the output buffer.
> +    return new_bufpos - bufpos;
> +}




[...]
> +/**
> + * Copy a previously painted macroblock to the current_block.
> + * @param copy_tag The tag that was in the nibble.
> + */
> +static void copy_previous_block(YopDecContext *s, int copy_tag)
> +{
> +    int i, j;
> +    uint8_t *bufptr;
> +    int bufpos = (size_t) s->dstptr - (size_t) s->dstbuf;
> +
> +    // calculate position for the copy source
> +    bufptr = s->dstbuf + bufpos + buffer_pos(s, bufpos,
> +                                             copy_lut[copy_tag][0] * RGB_COLORS,
> +                                             copy_lut[copy_tag][1]);
> +    // Each pixel in the macroblock
> +    for (i = 0; i < 4; i++) {
> +        int offset = macroblock_pixel_offset(s, i, bufpos);
> +        // Each color in the pixel
> +        for (j = 0; j < RGB_COLORS; j++) {
> +            *(s->dstptr + offset + j) = *(bufptr + offset + j);
> +        }
> +    }
> +}

this code is a mess
make linesize = width if it helps but dont run such code per pixel


> +
> +/**
> + * Return the next nibble in sequence, consuming a new byte on the input
> + * only if necessary.
> + */
> +static int get_next_nibble(YopDecContext *s)
> +{
> +    int high_nibble, low_nibble;
> +    if (s->tag_needs_next_byte) {
> +        s->current_tag_byte = *s->srcptr++;
> +        high_nibble = s->current_tag_byte >> 4;
> +        s->tag_needs_next_byte = 0;
> +        return high_nibble;
> +    } else {
> +        low_nibble = s->current_tag_byte & 0x0f;
> +        s->tag_needs_next_byte = 1;
> +        return low_nibble;
> +    }
> +}

duplicating get_bits ?


[...]
> Index: libavcodec/yop.h
> ===================================================================
> --- libavcodec/yop.h	(revision 0)
> +++ libavcodec/yop.h	(revision 0)
> @@ -0,0 +1,44 @@
> +/**
> + * @file libavcodec/yop.h
> + * Psygnosis YOP codec header
> + *
> + * Copyright (C) 2009 Thomas P. Higdon <thomas.p.higdon at gmail.com>
> + *
> + * This file is part of FFmpeg.
> + *
> + * FFmpeg is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License as published by the Free Software Foundation; either
> + * version 2.1 of the License, or (at your option) any later version.
> + *
> + * FFmpeg is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with FFmpeg; if not, write to the Free Software
> + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
> + */
> +
> +#define RGB_COLORS 3
> +
> +typedef struct YopDecContext {
> +    AVFrame frame;
> +    AVCodecContext *avctx;
> +    AVPacket *avpkt;
> +
> +    int num_pal_colors;
> +    int firstcolor_even;
> +    int firstcolor_odd;
> +    int sound_chunk_length;
> +    int tag_needs_next_byte;
> +    int current_tag_byte;
> +    int frame_data_length;
> +
> +    uint8_t *srcptr;
> +    uint8_t *dstptr;
> +    uint8_t *dstbuf;
> +
> +    uint8_t palette[256][3];
> +} YopDecContext;

belongs in the .c file if there is just one .c file for the decoder

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

The bravest are surely those who have the clearest vision
of what is before them, glory and danger alike, and yet
notwithstanding go out to meet it. -- Thucydides
-------------- 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/20090810/c67aa4dc/attachment.pgp>