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

Thomas Higdon thomas.p.higdon
Tue Aug 11 06:04:04 CEST 2009


Thanks for taking a look. Responses below inline.

On Mon, Aug 10, 2009 at 8:06 AM, Michael Niedermayer<michaelni at gmx.at> wrote:
> 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

Fixed.

>
> [...]
>> +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

Deleted.

>> +
>> + ? ?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
>

Fixed.

>> +
>> + ? ?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

Fixed.

>
>> + ? ?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
>

Fixed.

>> +
>> + ? ?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?

Coded deleted, see below.

>
>> +
>> + ? ?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

I hadn't realized I could set linesize. I've set linesize to be equal
to the amount of data in one row of pixels and made various code a bit
cleaner while eliminating the buffer_pos() function.

>
>> +
>> +/**
>> + * 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 ?

I'm not sure how to get the functionality I want from get_bits. I want
get_next_nibble() calls 0, 2, ... to get a byte from a buffer and
return the high bits, saving off the remaining 4 bits in that byte to
be used at an arbitrary later time, during which other byte-aligned
reads from the buffer can happen, starting at the following byte.
Calls 1,3,.. should just return the saved 4 bits and not consume
anything from the buffer. I can't tell how the functionality in
get_bits would help me accomplish this. It looks like get_bits is more
for the case when one is consuming arbitrary numbers of bits from a
buffer without intervening byte-aligned consumption.

>> 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

Fixed.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: yop-decode.diff
Type: text/x-patch
Size: 11487 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20090811/d99e795e/attachment.bin>



More information about the ffmpeg-devel mailing list