[FFmpeg-devel] [PATCH] [RFC] Second try at pixdesc.h:write_line()

[Michael Niedermayer]

On Sun, Apr 19, 2009 at 06:43:15PM +0200, Stefano Sabatini wrote:
> On date Sunday 2009-04-19 17:44:02 +0200, Michael Niedermayer encoded:
> > On Sun, Apr 19, 2009 at 05:20:57PM +0200, Stefano Sabatini wrote:
> [...]
> > > +/**
> > > + * Writes the values from \p src to the pixel format components \p c
> > > + * of an image line.
> > 
> > \p ...
> 
> Fixed.

thanks


> 
> > > + *
> > > + * @param data the array containing the pointers to the planes of the image
> > > + * @param linesizes the array containing the linesizes of the image
> > > + * @param desc the pixel format descriptor for the image
> > > + * @param x the horizontal coordinate of the first pixel to write
> > > + * @param y the vertical coordinate of the first pixel to write
> > > + * @param w the width of the line to write, that is the number of
> > > + * values to write to the image line
> > > + */
> > > +static inline void write_line(const uint16_t *src, uint8_t *data[4], const int linesize[4],
> > > +                              const AVPixFmtDescriptor *desc, int x, int y, int c, int w)
> > 
> > you are missing a param in the doxy
> 
> OK, though is almost redundant.

yes but x/y are too, it just felt odd that exactly one was ommited


>  
> > > +{
> > > +    AVComponentDescriptor comp = desc->comp[c];
> > > +    int plane = comp.plane;
> > > +    int depth = comp.depth_minus1+1;
> > > +    int step  = comp.step_minus1+1;
> > > +    int flags = desc->flags;
> > > +    uint32_t mask  = (1<<depth)-1;
> > > +
> > > +    if (flags & PIX_FMT_BITSTREAM) {
> > 
> > > +        int skip = x*step + comp.offset_plus1-1;
> > > +        uint8_t *p = data[plane] + y*linesize[plane] + (skip>>3);
> > > +        int shift = 8 - depth - (skip&7);
> > > +
> > > +        while (w--) {
> > > +            *p |= *src++ << shift;
> > > +            shift -= step;
> > > +            p -= shift>>3;
> > > +            shift &= 7;
> > > +        }
> > 
> > isnt this alot nicer than the redesigned bitstream writer? 
> 
> Well but I was looking for a more general solution, for which this one
> couldn't work.

but why? which pixel format can not be handled?



> 
> BTW, should be change accordingly read_line()?

i cant parse this sentance


>  
> > > +    } else {
> > > +        int shift = comp.shift;
> > > +        uint8_t *p = data[plane]+ y*linesize[plane] + x*step + comp.offset_plus1-1;
> > > +
> > > +        while (w--) {
> > > +            if (flags & PIX_FMT_BE) {
> > > +                uint16_t val = (AV_RB16(p) & ~(mask<<shift)) | (*src++<<shift);
> > > +                AV_WB16(p, val);
> > > +            } else {
> > > +                uint16_t val = (AV_RL16(p) & ~(mask<<shift)) | (*src++<<shift);
> > 
> > mask is useless, your initial buffer is supposed to be zeroed
> 
> Fixed.
>   
> [...]
>  
> > this is just for testing?
> > either way, there is no chance i would approve this for svn.
> > for fliping of common formats its too slow and for uncommon ones
> > its far too ugly, you can fliped the "unpacked" line
> 
> I see.

patch ok

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

I am the wisest man alive, for I know one thing, and that is that I know
nothing. -- 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/20090419/289e4c04/attachment.pgp>