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

[Michael Niedermayer]

Hi

On Mon, Apr 23, 2007 at 06:31:07AM -0400, Marc Hoffman wrote:
Content-Description: message body text
> Diego Biurrun writes:
>  > On Sun, Apr 22, 2007 at 11:15:38PM -0400, Marc Hoffman wrote:
>  > Content-Description: message body text
>  > > 
>  > > The BlackFin Video Color Space Converters I420 YV12 to RGB formats
>  > > 565, 555, and 888.
>  > > 
>  > 
>  > This license header is wrong, where did you copy and paste it from?  The
>  > relevant version of the LGPL is 2.1, not 2.
> 
> I have no idea where that header keeps comming from. Sorry about that
> I'm going to pay close attention on that issue next time maybe its an
> emacs bug which recognizes the LGPL but prefers GPL.  Because I
> thought I took it from yuv2rgb_altivec.c codes, anyways continue the
> review please.

[...]
> +
> +#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
...


[...]
> +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?

[...]
> +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()

also non static functions need a prefix (sws_ maybe?)

[...]

> +SwsFunc yuv2rgb_init_bfin (SwsContext *c)
> +{
> +    switch(c->dstFormat) {
> +
> +    case PIX_FMT_RGB555:
> +        av_log(c, AV_LOG_INFO, "BlackFin: Color Space RGB555\n");
> +        return bfin_yuv420_rgb555;
> +    case PIX_FMT_BGR555:
> +        av_log(c, AV_LOG_INFO, "BlackFin: Color Space BGR555\n");
> +        return bfin_yuv420_bgr555;
> +    case PIX_FMT_RGB565:
> +        av_log(c, AV_LOG_INFO, "BlackFin: Color Space RGB565\n");
> +        return bfin_yuv420_rgb565;
> +    case PIX_FMT_BGR565:
> +        av_log(c, AV_LOG_INFO, "BlackFin: Color Space BGR565\n");
> +        return bfin_yuv420_bgr565;
> +    case PIX_FMT_RGB24:
> +        av_log(c, AV_LOG_INFO, "BlackFin: Color Space RGB24\n");
> +        return bfin_yuv420_rgb24;
> +    case PIX_FMT_BGR24:
> +        av_log(c, AV_LOG_INFO, "BlackFin: Color Space BGR24\n");
> +        return bfin_yuv420_bgr24;

i think theres a generic function to convert PIX_FMT to a string
which could be used here instead o hardcoding 6 strings

[...]
> +  In both these cases, you have to clamp the output values to keep them in
> +  the [0-255] range. Rumour has it that the valid range is actually a subset
> +  of [0-255] (I've seen an RGB range of [16-235] mentioned) but clamping the
> +  values into [0-255] seems to produce acceptable results to me.

the RGB range is always 0-255 never 16-235, its the yuv ranges which can vary


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

Why not whip the teacher when the pupil misbehaves? -- Diogenes of Sinope
-------------- 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/4bc94547/attachment.pgp>