make pm4f be RGBA always, not pmcolor order

make pm4f be RGBA always, not pmcolor order

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

Committed: https://skia.googlesource.com/skia/+/93bb080503eec5df5674a1bcfe4baaceced799ff

[reed1, 2016-03-07 14:32:11]

Description was changed from

==========
make pm4f be RGBA always, not pmcolor order

BUG=skia:
==========

to

==========
make pm4f be RGBA always, not pmcolor order

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

[reed1, 2016-03-07 15:58:14]

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

[reed1, 2016-03-07 15:58:15]

ptal

This does not affect the order of SkColor4f (which is still ARGB). If we want to
revisit that, I'd like to do that separately.

[reed1, 2016-03-07 15:59:18]

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

[commit-bot: I haz the power, 2016-03-07 15:59:29]

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

[commit-bot: I haz the power, 2016-03-07 16:09:06]

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

[commit-bot: I haz the power, 2016-03-07 16:09:07]

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...)

[herb_g, 2016-03-07 16:11:04]

https://codereview.chromium.org/1774523002/diff/60001/src/core/SkPM4f.h
File src/core/SkPM4f.h (right):

https://codereview.chromium.org/1774523002/diff/60001/src/core/SkPM4f.h#newco...
src/core/SkPM4f.h:15: return Sk4f(x[2], x[1], x[0], x[3]);
How about SkNx_shuffle?

return SkNx_shuffle<2, 1, 0, 3>(x);

https://codereview.chromium.org/1774523002/diff/60001/src/core/SkPM4f.h#newco...
src/core/SkPM4f.h:20: return Sk4f(x[2], x[1], x[0], x[3]);
Why not call swizzle_rb here?

https://codereview.chromium.org/1774523002/diff/60001/src/core/SkPM4f.h#newco...
src/core/SkPM4f.h:40: A = SK_A32_SHIFT/8,
Remove dead code?

[mtklein, 2016-03-07 16:13:49]

https://codereview.chromium.org/1774523002/diff/40001/src/core/SkPM4f.h
File src/core/SkPM4f.h (right):

https://codereview.chromium.org/1774523002/diff/40001/src/core/SkPM4f.h#newco...
src/core/SkPM4f.h:15: return Sk4f(x[2], x[1], x[0], x[3]);
We'll have better opportunity to improve this later if we write this as:
    return SkNx_shuffle<2,1,0,3>(x);

https://codereview.chromium.org/1774523002/diff/40001/src/core/SkPM4f.h#newco...
src/core/SkPM4f.h:20: return Sk4f(x[2], x[1], x[0], x[3]);
return swizzle_rb(x);  ?

https://codereview.chromium.org/1774523002/diff/40001/src/core/SkPM4f.h#newco...
src/core/SkPM4f.h:28: return Sk4f(x[2], x[1], x[0], x[3]);
ditto

https://codereview.chromium.org/1774523002/diff/40001/src/core/SkPM4f.h#newco...
src/core/SkPM4f.h:52: SkPM4f rgba() const { return *this; }
It seems weird for these to return SkPM4f.  Only rgba() really returns an
SkPM4f.

Is it possible to eliminate these entirely in favor of the to4f() versions? 
Those make sense to me, and will probably be faster than something like
Sk4f::Load(pmf.pmorder()).

[f(malita), 2016-03-07 16:30:18]

On 2016/03/07 15:58:15, reed1 wrote:
> ptal
> 
> This does not affect the order of SkColor4f (which is still ARGB). If we want
to
> revisit that, I'd like to do that separately.

One advantage to the current pm4f order == PMC order is that we can have a
shared 4f core impl which writes to both pm4f and pmc dests without different
swizzling.  The 4f linear gradient takes advantage of this to support both
shadeSpan & shadeSpan4f efficiently.  We could always pre-swizzle the interval
colors to match the dest swizzle, but that feels a bit icky as we're lying about
the type/byteorder, plus there's no guarantee that the blitter will call a
particular shadeSpan version after the context is created.

Do you have a suggestion for how to avoid adding extra swizzles at the very last
stage?

[reed1, 2016-03-07 16:54:56]

We can still discuss whether to go with PM-order or Known-order.

I have been leaning toward known-order because it makes *some* sites more
readable, and because it appears that future frame-buffer formats (e.g. 10-10-10
or half-float) seem to be exclusively RGBA, so that the notion of supporting a
non-rgba swizzle is becoming a 32bit exception to the rule (at least from an
external perspective).

[f(malita), 2016-03-07 17:08:13]

On 2016/03/07 16:54:56, reed1 wrote:
> We can still discuss whether to go with PM-order or Known-order.
> 
> I have been leaning toward known-order because it makes *some* sites more
> readable, and because it appears that future frame-buffer formats (e.g.
10-10-10
> or half-float) seem to be exclusively RGBA, so that the notion of supporting a
> non-rgba swizzle is becoming a 32bit exception to the rule (at least from an
> external perspective).

Sounds good at a high level, I'm trying to work out some perf implications for
the current 4f gradient impl.

  -> onCreateContext -> build intervals using pm4f colors
     ...
  -> shadeSpan     -> pmc dest
  -> shadeSpan4f   -> pm4f dest

It would be nice to pre-swizzle colors in onCreateContext to match dest, but
there are no guarantees that the blitter will call a particular shade flavor.

option 1: swizzle as we write to dest. I assume this extra swizzle would have a
noticeable impact - am I wrong?

optuon 2: swizzle as we iterate over intervals and load the interval colors.
Sound more manageable, but still potentially noticeable for lots of intervals.

option 3: ?

[mtklein, 2016-03-07 17:12:38]

Sk4f bgra = SkNx_shuffle<2,1,0,3>(rgba) is about as cheap as you can get without
being free.

[f(malita), 2016-03-07 17:17:39]

On 2016/03/07 17:12:38, mtklein wrote:
> Sk4f bgra = SkNx_shuffle<2,1,0,3>(rgba) is about as cheap as you can get
without
> being free.

Thanks, sounds like option 2 should be feasible then.

[reed1, 2016-03-07 17:39:07]

https://codereview.chromium.org/1774523002/diff/40001/src/core/SkPM4f.h
File src/core/SkPM4f.h (right):

https://codereview.chromium.org/1774523002/diff/40001/src/core/SkPM4f.h#newco...
src/core/SkPM4f.h:15: return Sk4f(x[2], x[1], x[0], x[3]);
On 2016/03/07 16:13:49, mtklein wrote:
> We'll have better opportunity to improve this later if we write this as:
>     return SkNx_shuffle<2,1,0,3>(x);

Done.

https://codereview.chromium.org/1774523002/diff/40001/src/core/SkPM4f.h#newco...
src/core/SkPM4f.h:20: return Sk4f(x[2], x[1], x[0], x[3]);
On 2016/03/07 16:13:49, mtklein wrote:
> return swizzle_rb(x);  ?

Done.

https://codereview.chromium.org/1774523002/diff/40001/src/core/SkPM4f.h#newco...
src/core/SkPM4f.h:28: return Sk4f(x[2], x[1], x[0], x[3]);
On 2016/03/07 16:13:49, mtklein wrote:
> ditto

Done.

https://codereview.chromium.org/1774523002/diff/40001/src/core/SkPM4f.h#newco...
src/core/SkPM4f.h:52: SkPM4f rgba() const { return *this; }
On 2016/03/07 16:13:49, mtklein wrote:
> It seems weird for these to return SkPM4f.  Only rgba() really returns an
> SkPM4f.
> 
> Is it possible to eliminate these entirely in favor of the to4f() versions? 
> Those make sense to me, and will probably be faster than something like
> Sk4f::Load(pmf.pmorder()).

Moved to call-site

[reed1, 2016-03-07 17:39:23]

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

[commit-bot: I haz the power, 2016-03-07 17:39:33]

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

[commit-bot: I haz the power, 2016-03-07 17:57:06]

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

[commit-bot: I haz the power, 2016-03-07 17:57:07]

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

[mtklein, 2016-03-07 18:01:14]

https://codereview.chromium.org/1774523002/diff/80001/src/core/SkColorMatrixF...
File src/core/SkColorMatrixFilterRowMajor255.cpp (right):

https://codereview.chromium.org/1774523002/diff/80001/src/core/SkColorMatrixF...
src/core/SkColorMatrixFilterRowMajor255.cpp:128: return
swizzle_rgba_to_pmorder(to_4f(c)) * Sk4f(1.0f/255);
Let's not forget to go to one call, e.g. swizzle_if_bgra().

https://codereview.chromium.org/1774523002/diff/80001/src/core/SkPM4f.h
File src/core/SkPM4f.h (right):

https://codereview.chromium.org/1774523002/diff/80001/src/core/SkPM4f.h#newco...
src/core/SkPM4f.h:15: return Sk4f(x[2], x[1], x[0], x[3]);
You're killing me...

    return SkNx_shuffle<2,1,0,3>(x);

https://codereview.chromium.org/1774523002/diff/80001/src/core/SkPM4f.h#newco...
src/core/SkPM4f.h:56: Sk4f to4f() const { return Sk4f::Load(fVec); }
Looks great to me now.  Thanks.

[reed1, 2016-03-07 18:47:38]

https://codereview.chromium.org/1774523002/diff/80001/src/core/SkColorMatrixF...
File src/core/SkColorMatrixFilterRowMajor255.cpp (right):

https://codereview.chromium.org/1774523002/diff/80001/src/core/SkColorMatrixF...
src/core/SkColorMatrixFilterRowMajor255.cpp:128: return
swizzle_rgba_to_pmorder(to_4f(c)) * Sk4f(1.0f/255);
On 2016/03/07 18:01:14, mtklein wrote:
> Let's not forget to go to one call, e.g. swizzle_if_bgra().

Fixed in newer version.

[reed1, 2016-03-08 14:11:30]

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

[commit-bot: I haz the power, 2016-03-08 14:11:36]

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

[reed1, 2016-03-08 14:11:51]

florin, are we still ok to land this and *then* fix gradients?

[commit-bot: I haz the power, 2016-03-08 14:13:05]

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

[commit-bot: I haz the power, 2016-03-08 14:13:06]

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...)
  Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Release-Shared-Trybot on client.skia
(JOB_FAILED,
http://build.chromium.org/p/client.skia/builders/Test-Ubuntu-GCC-GCE-CPU-AVX2...)

[f(malita), 2016-03-08 14:25:18]

On 2016/03/08 14:11:51, reed1 wrote:
> florin, are we still ok to land this and *then* fix gradients?

yes, I have that CL ready: https://codereview.chromium.org/1770153002/

[f(malita), 2016-03-08 14:31:39]

lgtm

[mtklein, 2016-03-08 14:43:36]

lgtm too

[reed1, 2016-03-08 17:43:54]

The CQ bit was checked by reed@google.com

[commit-bot: I haz the power, 2016-03-08 17:44:02]

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

[commit-bot: I haz the power, 2016-03-08 18:09:20]

Description was changed from

==========
make pm4f be RGBA always, not pmcolor order

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

to

==========
make pm4f be RGBA always, not pmcolor order

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

Committed:
https://skia.googlesource.com/skia/+/93bb080503eec5df5674a1bcfe4baaceced799ff
==========

[commit-bot: I haz the power, 2016-03-08 18:09:21]

Committed patchset #8 (id:140001) as
https://skia.googlesource.com/skia/+/93bb080503eec5df5674a1bcfe4baaceced799ff