[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