[FFmpeg-devel] PATCH BlackFin yuv2rgb color space conversion

[Michael Niedermayer]

Hi

On Wed, May 09, 2007 at 09:29:57PM -0400, Marc Hoffman wrote:
> 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;
>     }

iam fine with either

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

Those who are too smart to engage in politics are punished by being
governed by those who are dumber. -- Plato 
-------------- 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/20070510/38e2ecac/attachment.pgp>