[FFmpeg-devel] PATCH BlackFin yuv2rgb color space conversion

[Marc Hoffman]

Michael Niedermayer writes:
 > Hi
 > 
 > On Wed, May 09, 2007 at 08:25:19PM -0400, Marc Hoffman wrote:
 > > Michael Niedermayer writes:
 > >  > Hi
 > >  > 
 > >  > On Tue, May 08, 2007 at 08:46:32PM -0400, Marc Hoffman wrote:
 > >  > > 
 > >  > > Blackfin optimized YUV420 to RGB CSC Color Space Converters.
 > >  > > 
 > >  > > This patch includes YUV2 -> RGB BGR for 565, 555 and 888 a.k.a. 24bit  
 > >  > > color.
 > >  > > 
 > >  > > Performance gain compared against -O3:
 > >  > > 
 > >  > > 2779809/1484290 187.28%
 > >  > > 
 > >  > > which normalized translates to ~33c/pel for the reference C vs ~17.5
 > >  > > c/pel for this optimized implementation.
 > >  > > 
 > >  > > Please review again, I hope I got everyones points if I didn't I'm  
 > >  > > sorry but there was a lot of feedback in the first round.
 > >  > 
 > >  > the mime type was application/octect stream which isnt correct for a patch
 > >  > 
 > >  > [...]
 > >  > > +    if (masks == 555) {
 > >  > > +        c->rmask = 0x001f * 0x00010001U;
 > >  > > +        c->gmask = 0x03e0 * 0x00010001U;
 > >  > > +        c->bmask = 0x7800 * 0x00010001U;
 > >  > > +    } else if (masks == 565) {
 > >  > > +        c->rmask = 0x001f * 0x00010001U;
 > >  > > +        c->gmask = 0x07e0 * 0x00010001U;
 > >  > > +        c->bmask = 0xf800 * 0x00010001U;
 > >  > > +    }
 > >  > 
 > >  > the r/bmask can be factored out of the if/else
 > > 
 > > The red is obvious but the blue is a bit different what are you
 > > thinking? 
 > 
 > not much today it seems
 > c->bmask = 0x7800
 > is wrong it should be 0x7C00 or 0xFC00
 > 
    c->rmask = 0x001f * 0x00010001U;
    if (masks == 555) {
        c->gmask = 0x03e0 * 0x00010001U;
        c->bmask = 0x7c00 * 0x00010001U;
    } else if (masks == 565) {
        c->gmask = 0x07e0 * 0x00010001U;
        c->bmask = 0xf800 * 0x00010001U;
    }

Is that better? Or would you perfer something else here? BTW I really
appreciate your time. That was such an extraordinary catch just from
reading my text. :)

I still prefer this as it looks better I think.  I do agree with your
less code makes it easier to review and maintain but this is already
fairly compact.  I will make it however you want, so if you want
something different let me know..

    if (masks == 555) {
        c->rmask = 0x001f * 0x00010001U;
        c->gmask = 0x03e0 * 0x00010001U;
        c->bmask = 0x7c00 * 0x00010001U;
    } else if (masks == 565) {
        c->rmask = 0x001f * 0x00010001U;
        c->gmask = 0x07e0 * 0x00010001U;
        c->bmask = 0xf800 * 0x00010001U;
    }

Thanks
Marc