[FFmpeg-devel] Fwd: [PATCH] Psygnosis YOP demuxer

[Michael Niedermayer]

On Sat, Mar 27, 2010 at 01:00:38PM +0100, Reimar D?ffinger wrote:
> On Sat, Mar 27, 2010 at 07:39:14AM +0530, Mohamed Naufal wrote:
> > +static const 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},
> > +                                        };
> 
> First byte is always 0, removing it will simplify address calculations.

true, ive missed that


> It _might_ also be faster to e.g. compress all 4 values into e.g.
> a single uint16_t value,

i doubt that


[...]

> > +    if (avctx->get_buffer(avctx, &s->frame) < 0) {
> > +        av_log(avctx, AV_LOG_ERROR, "get_buffer() failed\n");
> > +        return -1;
> > +    }
> > +
> > +    s->frame.linesize[0] = avctx->width;
> 
> I strongly object to this, this is outside of any
> documented API.

> The CODEC_CAP_DR1 documentation indicates that you must set it if

this codec does not set CODEC_CAP_DR1


> you use get_buffer, but changing linesize is incompatible with DR.
> You either need to respect the linesize as it is or do the buffer
> management without get_buffer.

if CODEC_CAP_DR1 is not set get_buffer == our internal get_buffer
so assumtations can be made that it behaves like it does.


> Or, if we really want this to be a valid way of doing it, improve
> that CODEC_CAP_DR1 documentation and add a comment that this codec
> intentionally does not set CODEC_CAP_DR1 because it can not work
> with it.

our documentation is shit but you coul interpret it as
CODEC_CAP_DR1 -> codec uses get_buffer
but not neccassarily <-

anyway it would be great if we could avoid the linesize overriding but i
dont think this is possible with this codec, i think it depends on having
line stored with linesize==width (or it would need some messy special case
handling close to the image borders, which does not seem like a good
idea)


> 
> > +    palette      = (uint32_t *)s->frame.data[1];
> > +
> > +    for (i = 0; i < s->num_pal_colors; i++, s->srcptr += 3)
> > +        palette[i + firstcolor] = (s->srcptr[0] << 18) |
> > +                                  (s->srcptr[1] << 10) |
> > +                                  (s->srcptr[2] << 2);
> 
> I think you should initialize the whole palette area to some value,
> even the unused parts.

get_buffer() does, if iam not mistaken

anyway, thanks for helping with reviewing, more eyes are always better

[...]

-- 
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/20100327/697e1207/attachment.pgp>