[Ffmpeg-devel] PATCH: BlackFin Optimized Color Space Converter

[Michael Niedermayer]

Hi

On Mon, Apr 23, 2007 at 09:22:23AM -0400, Marc Hoffman wrote:
> Hi,
> 
> Michael Niedermayer writes:
>  > Hi
>  > 
>  > [...]
>  > > +
>  > > +#define SETCOF(idx,val) { c->coeffs[idx*2]=(val); c->coeffs[idx*2+1]=(val); }
>  > > +
>  > > +#define COF_CY    0
>  > > +#define COF_OY    1
>  > > +#define COF_CRV   2
>  > > +#define COF_RMASK 3
>  > > +#define COF_CGU   4
>  > > +#define COF_CGV   5
>  > > +#define COF_GMASK 6
>  > > +#define COF_CBU   7
>  > > +#define COF_BMASK 8
>  > 
>  > this is a mess, why not?
>  > c->coeff_cy
>  > c->coeff_oy
>  > ...
> 
> This is to keep things consistent with the vector which the assembler
> code runs over.  I thought about what your suggesting and thought it
> was clearer this way.  Because, I would need to still use a macro to
> set the elements because they are packed.

c->coeff_cy= cy*0x0101 or 0x010001 or whatever it was ...


> 
>  > 
>  > 
>  > [...]
>  > > +static
>  > > +int bfin_yuv420_rgb565_1 (SwsContext *c,
>  > > +                           unsigned char **in, int *instrides,
>  > > +                           int srcSliceY, int srcSliceH,
>  > > +                           unsigned char **oplanes, int *outstrides, int rgb)
>  > > +{
>  > > +    unsigned short *coeff = c->coeffs;
>  > > +    unsigned short *tmpY  = c->tmpY;
>  > > +    unsigned short *tmpU  = c->tmpU;
>  > > +    unsigned short *tmpV  = c->tmpV;
>  > > +    unsigned char *py,*pu,*pv,*op;
>  > 
>  > why do you load tmp* into local variables?
> 
> STYLE.

that style makes the code 3 lines longer and harder to read as when the
reader sees tmpU she has to check what that is (ahh its c->tmpU, is it
changed anywhere? any tmpU++ hmmm, no) so IMHO if you really insist on this
then at least add a const at the correct place ...


> 
>  > 
>  > [...]
>  > > +void yuv2rgb_bfin_init_tables (SwsContext *c, const int inv_table[4],
>  > > +                               int brightness,int contrast, int saturation)
>  > > +{
>  > > +    int cy,oy,crv,cbu,cgu,cgv;
>  > > +    av_log (c, AV_LOG_DEBUG, "b: %d, c: %d, s: %d\n", brightness, contrast, saturation);
>  > > +
>  > > +    cy      = ((0xffffLL) * contrast>>8 )>>9;
>  > > +    oy      = (-256*brightness)<<8;
>  > > +    crv     =   (inv_table[0]>>2)*(contrast>>16)*(saturation>>16);
>  > > +    cbu     =   (inv_table[1]>>2)*(contrast>>16)*(saturation>>16);
>  > > +    cgu     = -((inv_table[2]>>2)*(contrast>>16)*(saturation>>16));
>  > > +    cgv     = -((inv_table[3]>>2)*(contrast>>16)*(saturation>>16));
>  > 
>  > this looks wrong also its duplicate from sws_setColorspaceDetails()
> 
> Let me try and copy the values computed for X86 and see what
> happens. Thanks for pointing that out I just copied what I had done
> previously for altivec and adjusted for the way I do the internal
> computation with CRV CBU.

maybe altivec is wrong too i didnt check ...


> 
>  > 
>  > also non static functions need a prefix (sws_ maybe?)
> 
> Agreed I will rename but I was keeping this consistent with what we have already. 

writing wrong code so its consistently wrong is not good :)


[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Democracy is the form of government in which you can choose your dictator
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20070423/0a500784/attachment.pgp>