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

Michael Niedermayer michaelni
Tue Aug 11 17:02:14 CEST 2009


On Tue, Aug 11, 2009 at 12:04:04AM -0400, Thomas Higdon wrote:
> Thanks for taking a look. Responses below inline.
[...]
> >
> >> +
> >> + ? ?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.

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


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

hmm, well, lets forget about get_bits() then

[...]
> +/**
> + * 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.
> + */
> +uint8_t paint_lut[15][5] = { {0, 1, 2, 3, 4},
> +                             {0, 1, 2, 0, 3},
> +                             {0, 1, 2, 1, 3},
> +                             {0, 1, 2, 2, 3},
> +                             {0, 1, 0, 2, 3},
> +                             {0, 1, 0, 0, 2},
> +                             {0, 1, 0, 1, 2},
> +                             {0, 1, 1, 2, 3},
> +                             {0, 0, 1, 2, 3},
> +                             {0, 0, 1, 0, 2},
> +                             {0, 1, 1, 0, 2},
> +                             {0, 0, 1, 1, 2},
> +                             {0, 0, 0, 1, 2},
> +                             {0, 0, 0, 0, 1},
> +                             {0, 1, 1, 1, 2},
> +                           };
> +
> +/**
> + * Lookup table for copying macroblocks. Each entry contains the respective
> + * x and y pixel offset for the copy source.
> + */
> +int copy_lut[16][2] = { {-4, -4},
> +                        {-2, -4},
> +                        { 0, -4},
> +                        { 2, -4},
> +                        {-4, -2},
> +                        {-4,  0},
> +                        {-3, -3},
> +                        {-1, -3},
> +                        { 1, -3},
> +                        { 3, -3},
> +                        {-3, -1},
> +                        {-2, -2},
> +                        { 0, -2},
> +                        { 2, -2},
> +                        { 4, -2},
> +                        {-2,  0},
> +                      };
> +

static const int8_t


[...]

> +/**
> + * Calculate the offset necessary to index each pixel in a macroblock.
> + * @param s context
> + * @param pixel the pixel number from 0-3
> + * @param bufpos the current position in the destination buffer.
> + */
> +static int macroblock_pixel_offset(YopDecContext *s, int pixel)
> +{
> +    switch (pixel) {
> +    case 0:
> +        return 0;
> +    case 1:
> +        return RGB_COLORS;
> +    case 2:
> +        return s->frame.linesize[0];
> +    case 3:
> +        return s->frame.linesize[0] + RGB_COLORS;
> +    default:
> +        av_log(0, AV_LOG_ERROR,
> +               "Invalid value to macroblock_pixel_offset: %d\n", pixel);
> +        return 0;
> +    }
> +}

this is unaceptable slow in the inner loops, a table or unrolling the loops
would be better


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

unneeded


> +    if (s->tag_needs_next_byte) {
[...]
> +        s->tag_needs_next_byte = 0;
[...]
> +    } else {
[...]
> +        s->tag_needs_next_byte = 1;
[...]
s->tag_needs_next_byte ^= 1;



> +}
> +
> +/**
> + * 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];
> +
> +    // If we are advancing to the next row of macroblocks
> +    if (rowpos == s->frame.linesize[0] - 2 * RGB_COLORS) {
> +        s->dstptr += s->frame.linesize[0] + 2 * RGB_COLORS;
> +    } else {
> +        s->dstptr += 2 * RGB_COLORS;
> +    }

+ 2 * RGB_COLORS can be factored out


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

The greatest way to live with honor in this world is to be what we pretend
to be. -- Socrates
-------------- 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/20090811/d32ef7f6/attachment.pgp>



More information about the ffmpeg-devel mailing list