Remove opaque versions of Color32_D565

Remove opaque versions of Color32_D565 

Removes the opaque-only versions of this function from the factory since
they will never be used. Opaque source colors are handled in
SkRGB16_Opaque_Blitter instead, which doesn't use the factory function.

Signed-off-by: Henrik Smiding <henrik.smiding@intel.com>;

Committed: https://skia.googlesource.com/skia/+/e6b1a60758aa16c0456ff8e1cf717c369e4e84b0

[henrik.smiding, 2015-02-04 12:18:32]

henrik.smiding@intel.com changed reviewers:
+ joakim.landberg@intel.com, mtklein@google.com

[henrik.smiding, 2015-02-04 12:19:58]

Extracted from https://codereview.chromium.org/892623002/

[mtklein, 2015-02-04 13:54:31]

This LGTM as documentation, however I think it may be that we're landing dead
code.

I added an SkASSERT(false) inside the new Color32_D565.  It doesn't trigger when
running DM or nanobench.

I believe this is because SkRGB16_Opaque_Blitter is making this decision to use
sk_memset16 or sk_dither_memset16 one level up, and so doesn't need to ever look
up or call a ColorProc16.  Seems sensible: I don't anticipate any
platform-specific proc will do better than the already platform-specific
memset16.

Might be instead of landing this, we want to land a change like this instead:

   NULL,  // SkRGB16_Opaque_Blitter will never look up this proc.
   Color32A_D565,
   NULL,  // SkRGB16_Opaque_Blitter will never look up this proc.
   Color32A_D565,  // TODO: implement dither

[henrik.smiding, 2015-02-04 19:06:08]

On 2015/02/04 13:54:31, mtklein wrote:
> This LGTM as documentation, however I think it may be that we're landing dead
> code.
> 
> I added an SkASSERT(false) inside the new Color32_D565.  It doesn't trigger
when
> running DM or nanobench.
> 
> I believe this is because SkRGB16_Opaque_Blitter is making this decision to
use
> sk_memset16 or sk_dither_memset16 one level up, and so doesn't need to ever
look
> up or call a ColorProc16.  Seems sensible: I don't anticipate any
> platform-specific proc will do better than the already platform-specific
> memset16.
> 
> Might be instead of landing this, we want to land a change like this instead:
> 
>    NULL,  // SkRGB16_Opaque_Blitter will never look up this proc.
>    Color32A_D565,
>    NULL,  // SkRGB16_Opaque_Blitter will never look up this proc.
>    Color32A_D565,  // TODO: implement dither

True, but then why not just ignore that alpha flag as well?
At the end of SkBlitRow_D16.cpp, in ColorFactory16, there's already a shift to
ignore the global alpha flag.
We could just shift by 2, and ignore both alpha flags. Only dither left?
---
    // we ignore kGlobalAlpha_Flag, so shift down
    flags >>= 1;
---

[mtklein, 2015-02-04 19:24:08]

On 2015/02/04 19:06:08, henrik.smiding wrote:
> On 2015/02/04 13:54:31, mtklein wrote:
> > This LGTM as documentation, however I think it may be that we're landing
dead
> > code.
> > 
> > I added an SkASSERT(false) inside the new Color32_D565.  It doesn't trigger
> when
> > running DM or nanobench.
> > 
> > I believe this is because SkRGB16_Opaque_Blitter is making this decision to
> use
> > sk_memset16 or sk_dither_memset16 one level up, and so doesn't need to ever
> look
> > up or call a ColorProc16.  Seems sensible: I don't anticipate any
> > platform-specific proc will do better than the already platform-specific
> > memset16.
> > 
> > Might be instead of landing this, we want to land a change like this
instead:
> > 
> >    NULL,  // SkRGB16_Opaque_Blitter will never look up this proc.
> >    Color32A_D565,
> >    NULL,  // SkRGB16_Opaque_Blitter will never look up this proc.
> >    Color32A_D565,  // TODO: implement dither
> 
> True, but then why not just ignore that alpha flag as well?
> At the end of SkBlitRow_D16.cpp, in ColorFactory16, there's already a shift to
> ignore the global alpha flag.
> We could just shift by 2, and ignore both alpha flags. Only dither left?
> ---
>     // we ignore kGlobalAlpha_Flag, so shift down
>     flags >>= 1;
> ---

We ignore the global-alpha flag because it's meaningless when blending with a
constant-color source.  The global alpha is there as an additional alpha-only
layer to apply to the blend when the source varies.

You want to also ignore the alpha in the SkColor that we're blending over the
565 destination?  Sort of defeats the point.

[reed1, 2015-02-04 22:33:54]

On 2015/02/04 19:24:08, mtklein wrote:
> On 2015/02/04 19:06:08, henrik.smiding wrote:
> > On 2015/02/04 13:54:31, mtklein wrote:
> > > This LGTM as documentation, however I think it may be that we're landing
> dead
> > > code.
> > > 
> > > I added an SkASSERT(false) inside the new Color32_D565.  It doesn't
trigger
> > when
> > > running DM or nanobench.
> > > 
> > > I believe this is because SkRGB16_Opaque_Blitter is making this decision
to
> > use
> > > sk_memset16 or sk_dither_memset16 one level up, and so doesn't need to
ever
> > look
> > > up or call a ColorProc16.  Seems sensible: I don't anticipate any
> > > platform-specific proc will do better than the already platform-specific
> > > memset16.
> > > 
> > > Might be instead of landing this, we want to land a change like this
> instead:
> > > 
> > >    NULL,  // SkRGB16_Opaque_Blitter will never look up this proc.
> > >    Color32A_D565,
> > >    NULL,  // SkRGB16_Opaque_Blitter will never look up this proc.
> > >    Color32A_D565,  // TODO: implement dither
> > 
> > True, but then why not just ignore that alpha flag as well?
> > At the end of SkBlitRow_D16.cpp, in ColorFactory16, there's already a shift
to
> > ignore the global alpha flag.
> > We could just shift by 2, and ignore both alpha flags. Only dither left?
> > ---
> >     // we ignore kGlobalAlpha_Flag, so shift down
> >     flags >>= 1;
> > ---
> 
> We ignore the global-alpha flag because it's meaningless when blending with a
> constant-color source.  The global alpha is there as an additional alpha-only
> layer to apply to the blend when the source varies.
> 
> You want to also ignore the alpha in the SkColor that we're blending over the
> 565 destination?  Sort of defeats the point.

Hmmm. alpha in the src is meaningful even if the dst is opaque (i.e. 565).

[henrik.smiding, 2015-02-05 11:37:31]

On 2015/02/04 22:33:54, reed1 wrote:
> On 2015/02/04 19:24:08, mtklein wrote:
> > On 2015/02/04 19:06:08, henrik.smiding wrote:
> > > On 2015/02/04 13:54:31, mtklein wrote:
> > > > This LGTM as documentation, however I think it may be that we're landing
> > dead
> > > > code.
> > > > 
> > > > I added an SkASSERT(false) inside the new Color32_D565.  It doesn't
> trigger
> > > when
> > > > running DM or nanobench.
> > > > 
> > > > I believe this is because SkRGB16_Opaque_Blitter is making this decision
> to
> > > use
> > > > sk_memset16 or sk_dither_memset16 one level up, and so doesn't need to
> ever
> > > look
> > > > up or call a ColorProc16.  Seems sensible: I don't anticipate any
> > > > platform-specific proc will do better than the already platform-specific
> > > > memset16.
> > > > 
> > > > Might be instead of landing this, we want to land a change like this
> > instead:
> > > > 
> > > >    NULL,  // SkRGB16_Opaque_Blitter will never look up this proc.
> > > >    Color32A_D565,
> > > >    NULL,  // SkRGB16_Opaque_Blitter will never look up this proc.
> > > >    Color32A_D565,  // TODO: implement dither
> > > 
> > > True, but then why not just ignore that alpha flag as well?
> > > At the end of SkBlitRow_D16.cpp, in ColorFactory16, there's already a
shift
> to
> > > ignore the global alpha flag.
> > > We could just shift by 2, and ignore both alpha flags. Only dither left?
> > > ---
> > >     // we ignore kGlobalAlpha_Flag, so shift down
> > >     flags >>= 1;
> > > ---
> > 
> > We ignore the global-alpha flag because it's meaningless when blending with
a
> > constant-color source.  The global alpha is there as an additional
alpha-only
> > layer to apply to the blend when the source varies.
> > 
> > You want to also ignore the alpha in the SkColor that we're blending over
the
> > 565 destination?  Sort of defeats the point.
> 
> Hmmm. alpha in the src is meaningful even if the dst is opaque (i.e. 565).

I'm talking about ignoring if the alpha in the src SkColor is opaque or not,
once we're in the SkRGB16_Blitter.

The result from SkBlitRow::ColorFactory16 is only used from the SkRGB16_Blitter,
which in turn is only created if the src SkColor is non opaque. The blitter is
chosen in SkBlitter_ChooseD565.
If the source color is opaque it will end up in SkRGB16_Opaque_Blitter anyway.
ColorFactory16 will still be called, but fColorProc16 will never be used.

I might be missing something, but you suggested that I could NULL the opaque
functions in the function table, in comment #3, since they can never be selected
anyway. So why not just remove them from the struct and only choose between
dither or no dither?

I guess it's a design choice in the end.

[mtklein, 2015-02-05 14:28:47]

On 2015/02/05 11:37:31, henrik.smiding wrote:
> On 2015/02/04 22:33:54, reed1 wrote:
> > On 2015/02/04 19:24:08, mtklein wrote:
> > > On 2015/02/04 19:06:08, henrik.smiding wrote:
> > > > On 2015/02/04 13:54:31, mtklein wrote:
> > > > > This LGTM as documentation, however I think it may be that we're
landing
> > > dead
> > > > > code.
> > > > > 
> > > > > I added an SkASSERT(false) inside the new Color32_D565.  It doesn't
> > trigger
> > > > when
> > > > > running DM or nanobench.
> > > > > 
> > > > > I believe this is because SkRGB16_Opaque_Blitter is making this
decision
> > to
> > > > use
> > > > > sk_memset16 or sk_dither_memset16 one level up, and so doesn't need to
> > ever
> > > > look
> > > > > up or call a ColorProc16.  Seems sensible: I don't anticipate any
> > > > > platform-specific proc will do better than the already
platform-specific
> > > > > memset16.
> > > > > 
> > > > > Might be instead of landing this, we want to land a change like this
> > > instead:
> > > > > 
> > > > >    NULL,  // SkRGB16_Opaque_Blitter will never look up this proc.
> > > > >    Color32A_D565,
> > > > >    NULL,  // SkRGB16_Opaque_Blitter will never look up this proc.
> > > > >    Color32A_D565,  // TODO: implement dither
> > > > 
> > > > True, but then why not just ignore that alpha flag as well?
> > > > At the end of SkBlitRow_D16.cpp, in ColorFactory16, there's already a
> shift
> > to
> > > > ignore the global alpha flag.
> > > > We could just shift by 2, and ignore both alpha flags. Only dither left?
> > > > ---
> > > >     // we ignore kGlobalAlpha_Flag, so shift down
> > > >     flags >>= 1;
> > > > ---
> > > 
> > > We ignore the global-alpha flag because it's meaningless when blending
with
> a
> > > constant-color source.  The global alpha is there as an additional
> alpha-only
> > > layer to apply to the blend when the source varies.
> > > 
> > > You want to also ignore the alpha in the SkColor that we're blending over
> the
> > > 565 destination?  Sort of defeats the point.
> > 
> > Hmmm. alpha in the src is meaningful even if the dst is opaque (i.e. 565).
> 
> I'm talking about ignoring if the alpha in the src SkColor is opaque or not,
> once we're in the SkRGB16_Blitter.
> 
> The result from SkBlitRow::ColorFactory16 is only used from the
SkRGB16_Blitter,
> which in turn is only created if the src SkColor is non opaque. The blitter is
> chosen in SkBlitter_ChooseD565.
> If the source color is opaque it will end up in SkRGB16_Opaque_Blitter anyway.
> ColorFactory16 will still be called, but fColorProc16 will never be used.
> 
> I might be missing something, but you suggested that I could NULL the opaque
> functions in the function table, in comment #3, since they can never be
selected
> anyway. So why not just remove them from the struct and only choose between
> dither or no dither?
> 
> I guess it's a design choice in the end.

Oh, I see.  Either way works for me.

[henrik.smiding, 2015-02-08 18:01:39]

New patchsets have been uploaded after l-g-t-m from mtklein@google.com

[henrik.smiding, 2015-02-08 20:08:42]

I removed the opaque optimization and trimmed the function list/array.

Still can't get trybots to work, though. Not sure why.

[mtklein, 2015-02-08 23:56:51]

The CQ bit was checked by mtklein@google.com

[mtklein, 2015-02-08 23:56:51]

lgtm

[commit-bot: I haz the power, 2015-02-08 23:57:42]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/901593002/20001

[commit-bot: I haz the power, 2015-02-09 00:03:29]

Committed patchset #2 (id:20001) as
https://skia.googlesource.com/skia/+/e6b1a60758aa16c0456ff8e1cf717c369e4e84b0

[mtklein, 2015-02-09 00:16:23]

After looking at the work required to update the Color32A_D565 algorithm, I
don't think I'm going to do it.  The visual benefits of upgrading the precision
looked fairly minor (the current algorithm isn't _so_ terrible), and the fact
that we've already got a NEON version of this code makes it awkward to switch. 
So if you'd like to, please do have a swing at an SSE version.  I should caution
you though that, to our knowledge, no one is using the 565 backend, so this is
not a high-impact area to be working on.  I suspect the best you'll see are
improved benchmarks, but no real world win--- unless you've got some secret user
of the 565 backend we don't know about.  I'd still recommend targeting SSE2 if
possible, and to favor simple clear code.

[mtklein, 2015-02-09 00:17:48]

On 2015/02/09 00:16:23, mtklein wrote:
> After looking at the work required to update the Color32A_D565 algorithm, I
> don't think I'm going to do it.  The visual benefits of upgrading the
precision
> looked fairly minor (the current algorithm isn't _so_ terrible), and the fact
> that we've already got a NEON version of this code makes it awkward to switch.

> So if you'd like to, please do have a swing at an SSE version.  I should
caution
> you though that, to our knowledge, no one is using the 565 backend, so this is
> not a high-impact area to be working on.  I suspect the best you'll see are
> improved benchmarks, but no real world win--- unless you've got some secret
user
> of the 565 backend we don't know about.  I'd still recommend targeting SSE2 if
> possible, and to favor simple clear code.

I have also noticed that the existing code seems to autovectorize fairly well.

[reed1, 2015-02-09 14:45:31]

reed@google.com changed reviewers:
+ reed@google.com

[reed1, 2015-02-09 14:45:31]

I'm not sure the proper explanation is that the current client doesn't call
this.

SkBlitRow is an api, and it is better when it behaves "correctly" regardless of
the current client.

After this CL, is the API contract still met? If I call this with opaque colors,
will it draw correctly? If so, great. Perhaps the comment could be changed to be
: no need for the addition code specializing on opaque alpha at this time...

[henrik.smiding, 2015-02-09 14:48:54]

On 2015/02/09 00:17:48, mtklein wrote:
> On 2015/02/09 00:16:23, mtklein wrote:
> > After looking at the work required to update the Color32A_D565 algorithm, I
> > don't think I'm going to do it.  The visual benefits of upgrading the
> precision
> > looked fairly minor (the current algorithm isn't _so_ terrible), and the
fact
> > that we've already got a NEON version of this code makes it awkward to
switch.
> 
> > So if you'd like to, please do have a swing at an SSE version.  I should
> caution
> > you though that, to our knowledge, no one is using the 565 backend, so this
is
> > not a high-impact area to be working on.  I suspect the best you'll see are
> > improved benchmarks, but no real world win--- unless you've got some secret
> user
> > of the 565 backend we don't know about.  I'd still recommend targeting SSE2
if
> > possible, and to favor simple clear code.
> 
> I have also noticed that the existing code seems to autovectorize fairly well.

Ok, I will continue with the actual optimization, and answer your review
comments to start with.
It will be massive. The comments, not the code.

[henrik.smiding, 2015-02-12 12:32:57]

On 2015/02/09 14:45:31, reed1 wrote:
> I'm not sure the proper explanation is that the current client doesn't call
> this.
> 
> SkBlitRow is an api, and it is better when it behaves "correctly" regardless
of
> the current client.
> 
> After this CL, is the API contract still met? If I call this with opaque
colors,
> will it draw correctly? If so, great. Perhaps the comment could be changed to
be
> : no need for the addition code specializing on opaque alpha at this time...

Opaque color would still work, just not as fast as the dedicated opaque blitter.
Will change the comment in my next patch.