float components in xfermodes

float components in xfermodes

BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&issue=1623483002

TBR=mtklein

Committed: https://skia.googlesource.com/skia/+/395eabeb0e72334c45324874c6e009b54634df21

[reed1, 2016-01-29 21:31:54]

The CQ bit was checked by reed@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-01-29 21:32:47]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1634273002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1634273002/80001

[reed1, 2016-01-29 21:35:04]

reed@google.com changed reviewers:
+ herb@google.com, mtklein@google.com

[reed1, 2016-01-29 21:35:04]

more work to do on gm, but this is the bulk of the intended CL

[commit-bot: I haz the power, 2016-01-29 21:35:24]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-01-29 21:35:24]

Dry run: Try jobs failed on following builders:
  Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Debug-Trybot on client.skia (JOB_FAILED,
http://build.chromium.org/p/client.skia/builders/Test-Ubuntu-GCC-GCE-CPU-AVX2...)

[reed1, 2016-01-30 00:13:10]

The CQ bit was checked by reed@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-01-30 00:13:22]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1634273002/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1634273002/120001

[commit-bot: I haz the power, 2016-01-30 00:14:42]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-01-30 00:14:43]

Dry run: Try jobs failed on following builders:
  Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Debug-Trybot on client.skia (JOB_FAILED,
http://build.chromium.org/p/client.skia/builders/Test-Ubuntu-GCC-GCE-CPU-AVX2...)
  Build-Ubuntu-Clang-x86_64-Debug-Trybot on client.skia.compile (JOB_FAILED,
http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-Clang-x...)
  Build-Ubuntu-GCC-Arm7-Debug-Android-Trybot on client.skia.compile (JOB_FAILED,
http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-GCC-Arm...)
  Build-Ubuntu-GCC-Mips-Debug-Android-Trybot on client.skia.compile (JOB_FAILED,
http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-GCC-Mip...)

[reed1, 2016-01-30 00:25:32]

The CQ bit was checked by reed@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-01-30 00:25:41]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1634273002/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1634273002/140001

[reed1, 2016-01-30 00:29:49]

ptal

-- won't land until I disable invoking the blitter, since that kicks in when we
have a colorshader, and its not ready for primetime.

[reed1, 2016-01-30 00:49:03]

The CQ bit was checked by reed@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-01-30 00:49:15]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1634273002/180001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1634273002/180001

[reed1, 2016-01-30 00:57:50]

blitter disabled (w/ a private flag for dev testing)
PTAL

[reed1, 2016-01-30 00:57:54]

The CQ bit was unchecked by reed@google.com

[mtklein, 2016-01-30 01:19:02]

I am okay to continue with post-facto review if you want to keep your creative
pipeline flowing.

[reed1, 2016-01-30 02:00:45]

thanks mike.

I concede that there must be more compact ways to expand the various xfermodes
into N proc variants ( w/ and w/o srgb/opaque/coverage ). I'd be grateful if you
wanted to take a stab at that after this lands. I only coded the easy/common
ones.

[reed1, 2016-01-31 02:40:04]

Description was changed from

==========
float components in xfermodes

BUG=skia:
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========

to

==========
float components in xfermodes

BUG=skia:
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...

TBR=mtklein
==========

[reed1, 2016-01-31 02:40:09]

The CQ bit was checked by reed@google.com

[commit-bot: I haz the power, 2016-01-31 02:40:13]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1634273002/180001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1634273002/180001

[commit-bot: I haz the power, 2016-01-31 02:52:35]

Description was changed from

==========
float components in xfermodes

BUG=skia:
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...

TBR=mtklein
==========

to

==========
float components in xfermodes

BUG=skia:
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...

TBR=mtklein

Committed:
https://skia.googlesource.com/skia/+/395eabeb0e72334c45324874c6e009b54634df21
==========

[commit-bot: I haz the power, 2016-01-31 02:52:36]

Committed patchset #10 (id:180001) as
https://skia.googlesource.com/skia/+/395eabeb0e72334c45324874c6e009b54634df21

[mtklein, 2016-02-01 22:53:52]

not quite through everything, but done for the day

https://codereview.chromium.org/1634273002/diff/180001/include/core/SkXfermode.h
File include/core/SkXfermode.h (right):

https://codereview.chromium.org/1634273002/diff/180001/include/core/SkXfermod...
include/core/SkXfermode.h:232: enum PM4fFlags {
We may want to split these up / mark these as required and optional. 
SrcIsOpaque is an optional optimization.  DstIsSRGB is required for correctness.
 Seems a little weird to treat them the same.

https://codereview.chromium.org/1634273002/diff/180001/src/core/SkCoreBlitters.h
File src/core/SkCoreBlitters.h (right):

https://codereview.chromium.org/1634273002/diff/180001/src/core/SkCoreBlitter...
src/core/SkCoreBlitters.h:207: SkARGB32_Shader4f_Blitter& operator=(const
SkARGB32_Shader4f_Blitter&);
doesn't hurt to add "= delete" to these now.

https://codereview.chromium.org/1634273002/diff/180001/src/core/SkXfermode4f.cpp
File src/core/SkXfermode4f.cpp (right):

https://codereview.chromium.org/1634273002/diff/180001/src/core/SkXfermode4f....
src/core/SkXfermode4f.cpp:40: static Sk4f scale_255_round(const SkPM4f& pm4) {
Generally I like to scope one-shot helpers like this as lambdas now, here
nestled into pm4f_to_linear_32.  Seeing it here makes me think i'll find it used
more later on down the file.

https://codereview.chromium.org/1634273002/diff/180001/src/core/SkXfermode4f....
src/core/SkXfermode4f.cpp:78: SkPMColor tmp[N];
yeesh.  _1 variants are a really good idea.  We might want to backport the idea.

https://codereview.chromium.org/1634273002/diff/180001/src/core/SkXfermode4f....
src/core/SkXfermode4f.cpp:133: clear_linear_n(state, dst, nullptr, count,
coverage);
We might want to make the _1 and _n variants take the same arguments (i.e. const
SkPM4f*).  It won't matter for speed, and it'll let us write some things like
this a bit more compactly.  We'll only have to deal with 1 proc type too.

[reed1, 2016-02-02 14:20:54]

https://codereview.chromium.org/1634273002/diff/180001/include/core/SkXfermode.h
File include/core/SkXfermode.h (right):

https://codereview.chromium.org/1634273002/diff/180001/include/core/SkXfermod...
include/core/SkXfermode.h:232: enum PM4fFlags {
On 2016/02/01 22:53:52, mtklein wrote:
> We may want to split these up / mark these as required and optional. 
> SrcIsOpaque is an optional optimization.  DstIsSRGB is required for
correctness.
>  Seems a little weird to treat them the same.

I combined them just for convenience. Happy to consider other organizations.

https://codereview.chromium.org/1634273002/diff/180001/src/core/SkCoreBlitters.h
File src/core/SkCoreBlitters.h (right):

https://codereview.chromium.org/1634273002/diff/180001/src/core/SkCoreBlitter...
src/core/SkCoreBlitters.h:207: SkARGB32_Shader4f_Blitter& operator=(const
SkARGB32_Shader4f_Blitter&);
On 2016/02/01 22:53:52, mtklein wrote:
> doesn't hurt to add "= delete" to these now.

Clearly this was copy-paste from above. Is "= delete" something we should
manually add to all of the shader subclasses?

https://codereview.chromium.org/1634273002/diff/180001/src/core/SkXfermode4f.cpp
File src/core/SkXfermode4f.cpp (right):

https://codereview.chromium.org/1634273002/diff/180001/src/core/SkXfermode4f....
src/core/SkXfermode4f.cpp:40: static Sk4f scale_255_round(const SkPM4f& pm4) {
On 2016/02/01 22:53:52, mtklein wrote:
> Generally I like to scope one-shot helpers like this as lambdas now, here
> nestled into pm4f_to_linear_32.  Seeing it here makes me think i'll find it
used
> more later on down the file.

We can change. I find some value in naming these at times, as that documents the
(intent of) the transformation, something that is lost in a lambda. Perhaps the
need (or lack thereof) for micro documentation will change as we get more mature
in how we organize this new workflow.

https://codereview.chromium.org/1634273002/diff/180001/src/core/SkXfermode4f....
src/core/SkXfermode4f.cpp:133: clear_linear_n(state, dst, nullptr, count,
coverage);
On 2016/02/01 22:53:52, mtklein wrote:
> We might want to make the _1 and _n variants take the same arguments (i.e.
const
> SkPM4f*).  It won't matter for speed, and it'll let us write some things like
> this a bit more compactly.  We'll only have to deal with 1 proc type too.

I certainly pondered this. Can't tell if that helps readability at the call
sites or not. As it is, the caller is definitely aware that they are only giving
us 1 src. I think I prefer to error on the side of pedanticness for the moment,
and reconsider that once things are more fleshed-out. That said, I don't feel
terribly strongly either way.

[mtklein, 2016-02-02 14:24:11]

https://codereview.chromium.org/1634273002/diff/180001/src/core/SkXfermode4f.cpp
File src/core/SkXfermode4f.cpp (right):

https://codereview.chromium.org/1634273002/diff/180001/src/core/SkXfermode4f....
src/core/SkXfermode4f.cpp:40: static Sk4f scale_255_round(const SkPM4f& pm4) {
On 2016/02/02 14:20:54, reed1 wrote:
> On 2016/02/01 22:53:52, mtklein wrote:
> > Generally I like to scope one-shot helpers like this as lambdas now, here
> > nestled into pm4f_to_linear_32.  Seeing it here makes me think i'll find it
> used
> > more later on down the file.
> 
> We can change. I find some value in naming these at times, as that documents
the
> (intent of) the transformation, something that is lost in a lambda. Perhaps
the
> need (or lack thereof) for micro documentation will change as we get more
mature
> in how we organize this new workflow.

Lambdas can have names too...

auto scale_255_round = [](const SkPM4f& pm4) {
    return Sk4f::Load(pm4.fVec) * Sk4f(255) + Sk4f(0.5f);
};

Just suggesting to scope this tighter while it's only used in that tight scope.

[reed1, 2016-02-02 14:40:05]

On 2016/02/02 14:24:11, mtklein wrote:
>
https://codereview.chromium.org/1634273002/diff/180001/src/core/SkXfermode4f.cpp
> File src/core/SkXfermode4f.cpp (right):
> 
>
https://codereview.chromium.org/1634273002/diff/180001/src/core/SkXfermode4f....
> src/core/SkXfermode4f.cpp:40: static Sk4f scale_255_round(const SkPM4f& pm4) {
> On 2016/02/02 14:20:54, reed1 wrote:
> > On 2016/02/01 22:53:52, mtklein wrote:
> > > Generally I like to scope one-shot helpers like this as lambdas now, here
> > > nestled into pm4f_to_linear_32.  Seeing it here makes me think i'll find
it
> > used
> > > more later on down the file.
> > 
> > We can change. I find some value in naming these at times, as that documents
> the
> > (intent of) the transformation, something that is lost in a lambda. Perhaps
> the
> > need (or lack thereof) for micro documentation will change as we get more
> mature
> > in how we organize this new workflow.
> 
> Lambdas can have names too...
> 
> auto scale_255_round = [](const SkPM4f& pm4) {
>     return Sk4f::Load(pm4.fVec) * Sk4f(255) + Sk4f(0.5f);
> };
> 
> Just suggesting to scope this tighter while it's only used in that tight
scope.

Ah, that makes good sense.