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

Thomas Higdon thomas.p.higdon
Thu Aug 13 04:58:35 CEST 2009


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?

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

Fixed.

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

Converted to a lookup table.

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

Removed.

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

Fixed.

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

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



More information about the ffmpeg-devel mailing list