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

Michael Niedermayer michaelni
Fri Aug 14 17:16:50 CEST 2009


On Wed, Aug 12, 2009 at 10:58:35PM -0400, Thomas Higdon wrote:
> On Tue, Aug 11, 2009 at 11:02 AM, Michael Niedermayer<michaelni at gmx.at> wrote:
> > On Tue, Aug 11, 2009 at 12:04:04AM -0400, Thomas Higdon wrote:
> >> Thanks for taking a look. Responses below inline.
> >> >
> >> > [...]
> >> >> +/**
> >> >> + * 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.
> >
> > you can set linesize by allocating your own picture with the linesize you
> > want, you cannot change the linesize of a picture allocated by get_buffer()
> 
> Ok, now I'm allocating an AVFrame data[0] using av_mallocz and setting
> my own linesize[0]. Things appear to work. Is that what you were
> talking about?

yes, approximately


[...]
> +/**
> + * Paint a macroblock using the pattern in paint_lut.
> + * @param s codec context
> + * @param tag The tag that was in the nibble.
> + */
> +static void paint_block(YopDecContext *s, int tag)
> +{
> +    int i, j;
> +
> +    for (i = 0; i < 4; i++) {
> +        int color_num = *(s->srcptr + paint_lut[tag][i]);
> +        int offset = s->block_offset_lut[i];
> +        for (j = 0; j < RGB_COLORS; j++) {
> +            *(s->dstptr + offset + j) = s->palette[color_num][j];
> +        }
> +    }

thats too much unneeded stuff done per pixel
for example unrolling the inner loop needs 3 lines, same as the loop


> +    // The number of src bytes consumed is in the last part of the lut entry.
> +    s->srcptr += paint_lut[tag][4];
> +}
> +
> +/**
> + * 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;
> +
> +    // calculate position for the copy source
> +    bufptr = s->dstptr + copy_lut[copy_tag][0] * RGB_COLORS +
> +                s->frame.linesize[0] * copy_lut[copy_tag][1];
> +    // Each pixel in the macroblock
> +    for (i = 0; i < 4; i++) {
> +        int offset = s->block_offset_lut[i];
> +        // Each color in the pixel
> +        for (j = 0; j < RGB_COLORS; j++) {
> +            *(s->dstptr + offset + j) = *(bufptr + offset + j);
> +        }

same

[...]
> +/**
> + * Take s->dstptr to the next macroblock in sequence.
> + */
> +static void next_macroblock(YopDecContext *s)
> +{
> +    int bufpos = (size_t) s->dstptr - (size_t) s->dstbuf;
> +    int rowpos = bufpos % s->frame.linesize[0];

% is slow, and i dont see why it would be needed for each mb


[...]
> +/**
> + * Update the palette based on the frame number. Consume all of the palette
> + * bytes on the input.
> + */
> +void update_palette(YopDecContext *s)
> +{
> +    int firstcolor;
> +    int i, j;
> +
> +    if (s->avctx->frame_number & 1) {
> +        firstcolor = s->firstcolor_odd;
> +    } else {
> +        firstcolor = s->firstcolor_even;
> +    }

s->firstcolor[frame_number & 1]
seems simpler


> +
> +    for (i = 0; i < s->num_pal_colors; i++) {
> +        for (j = 0; j < RGB_COLORS; j++) {
> +            // shift left by 2 because the encoded palette is 6-bit color
> +            s->palette[i + firstcolor][j] = *s->srcptr++ << 2;
> +        }
> +    }
> +}
> +
> +static int decode_frame(AVCodecContext *avctx, void *data, int *data_size,
> +                        AVPacket *avpkt)
> +{
> +    YopDecContext *s = avctx->priv_data;
> +    int current_nibble;
> +
> +    s->srcptr = avpkt->data;
> +    s->avpkt = avpkt;
> +
> +    if (!s->frame.data[0]) {
> +      s->frame.data[0] = av_mallocz(avctx->width * RGB_COLORS * avctx->height);
> +    }

indentiom depth is inconsistent


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

I wish the Xiph folks would stop pretending they've got something they
do not.  Somehow I fear this will remain a wish. -- M?ns Rullg?rd
-------------- 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/20090814/0a411230/attachment.pgp>



More information about the ffmpeg-devel mailing list