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

[Marc Hoffman]

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.

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

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

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

Agreed I will rename but I was keeping this consistent with what we have already. 

so I was just following along with the same naming convention as:

void yuv2rgb_altivec_init_tables (SwsContext *c, const int inv_table[4],int brightness,int contrast, int saturation)


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

Cool I want to use this we should change the Altivec CSC as well to use this.

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

I copied this from color-space text.

Thanks for your review. I will dig into the coeff setup codes and see
if I can utilize the X86 values.

Marc