SkLiteDL: thread the original canvas matrix through for SetMatrix::Draw().

SkLiteDL: thread the original canvas matrix through for SetMatrix::Draw().

The SkLiteDL is recorded in some identity space (imagine, SkMatrix::I()),
but played back in a different one (here named SkMatrix original).  Any
calls to setMatrix() need to be made relative to this new space.

All other ops already operate in relative coordinates.

This should let us not fiddle with setMatrix().

BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2247353003

Committed: https://skia.googlesource.com/skia/+/6480619f9ff4676a97edae41b2ca23eadeddcfbf

[mtklein_C, 2016-08-16 21:26:51]

Description was changed from

==========
SkLiteDL: thread the original canvas matrix through for SetMatrix::Draw().

The SkLiteDL is recorded in some identity space (imagine, SkMatrix::I()),
but played back in a different one (here named SkMatrix original).  Any
calls to setMatrix() need to be made relative to this new space.

All other ops already operate in relative coordinates.

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

to

==========
SkLiteDL: thread the original canvas matrix through for SetMatrix::Draw().

The SkLiteDL is recorded in some identity space (imagine, SkMatrix::I()),
but played back in a different one (here named SkMatrix original).  Any
calls to setMatrix() need to be made relative to this new space.

All other ops already operate in relative coordinates.

BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2247353003
==========

[mtklein_C, 2016-08-16 21:59:05]

The CQ bit was checked by mtklein@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-16 21:59:15]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[mtklein_C, 2016-08-16 22:12:38]

mtklein@chromium.org changed reviewers:
+ djsollen@google.com

[mtklein_C, 2016-08-16 22:19:59]

Description was changed from

==========
SkLiteDL: thread the original canvas matrix through for SetMatrix::Draw().

The SkLiteDL is recorded in some identity space (imagine, SkMatrix::I()),
but played back in a different one (here named SkMatrix original).  Any
calls to setMatrix() need to be made relative to this new space.

All other ops already operate in relative coordinates.

BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2247353003
==========

to

==========
SkLiteDL: thread the original canvas matrix through for SetMatrix::Draw().

The SkLiteDL is recorded in some identity space (imagine, SkMatrix::I()),
but played back in a different one (here named SkMatrix original).  Any
calls to setMatrix() need to be made relative to this new space.

All other ops already operate in relative coordinates.

This should let us not fiddle with setMatrix().

BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2247353003
==========

[commit-bot: I haz the power, 2016-08-16 22:21:28]

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

[commit-bot: I haz the power, 2016-08-16 22:21:29]

Dry run: This issue passed the CQ dry run.

[reed1, 2016-08-16 23:31:50]

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

[reed1, 2016-08-16 23:31:51]

https://codereview.chromium.org/2247353003/diff/1/src/core/SkLiteDL.cpp
File src/core/SkLiteDL.cpp (right):

https://codereview.chromium.org/2247353003/diff/1/src/core/SkLiteDL.cpp#newco...
src/core/SkLiteDL.cpp:131: void draw(SkCanvas* c, const SkMatrix& original) {
Crazy idea (to avoid having to pass 2 params to everyone)

- store a matrix* when you record SetMatrix, pointing to a single matrix shared
by all ops
- on playback, just jam the original in there, so SetMatrix::draw becomes

void draw(SkCanvas* c) { c->concat(SkMatrix::Concat(*original, matrix); }

[mtklein, 2016-08-16 23:44:47]

On 2016/08/16 23:31:51, reed1 wrote:
> https://codereview.chromium.org/2247353003/diff/1/src/core/SkLiteDL.cpp
> File src/core/SkLiteDL.cpp (right):
> 
>
https://codereview.chromium.org/2247353003/diff/1/src/core/SkLiteDL.cpp#newco...
> src/core/SkLiteDL.cpp:131: void draw(SkCanvas* c, const SkMatrix& original) {
> Crazy idea (to avoid having to pass 2 params to everyone)
> 
> - store a matrix* when you record SetMatrix, pointing to a single matrix
shared
> by all ops
> - on playback, just jam the original in there, so SetMatrix::draw becomes
> 
> void draw(SkCanvas* c) { c->concat(SkMatrix::Concat(*original, matrix); }

Yep, that'd work, but it would cut off any hope of thread safety.  Speed-wise,
it shouldn't hurt to pass the extra SkMatrix& through where unused.

It does look a bit ugly though.  There may also be a way to clean up all the
noise of the extra matrix param for each of these draws if I get sneaky about
looking at the type of T::draw() when building my draw_fn table.

I have at times been tempted to try something like a stack-relative load to
avoid the explicit need to pass the parameter, but as you might expect that's
turned out to be tricky to explain to a compiler.

[reed1, 2016-08-16 23:57:23]

boo hoo for thread-safety. lgtm

[mtklein, 2016-08-17 00:16:29]

On 2016/08/16 23:57:23, reed1 wrote:
> boo hoo for thread-safety. lgtm

FWIW, I am keeping in mind that we don't yet need thread safety.  But there's
really no runtime cost to this solution that keeps it viable.  Just a little
awkward.

[mtklein, 2016-08-17 00:17:29]

The CQ bit was checked by mtklein@google.com

[commit-bot: I haz the power, 2016-08-17 00:17:37]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-17 00:18:30]

Description was changed from

==========
SkLiteDL: thread the original canvas matrix through for SetMatrix::Draw().

The SkLiteDL is recorded in some identity space (imagine, SkMatrix::I()),
but played back in a different one (here named SkMatrix original).  Any
calls to setMatrix() need to be made relative to this new space.

All other ops already operate in relative coordinates.

This should let us not fiddle with setMatrix().

BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2247353003
==========

to

==========
SkLiteDL: thread the original canvas matrix through for SetMatrix::Draw().

The SkLiteDL is recorded in some identity space (imagine, SkMatrix::I()),
but played back in a different one (here named SkMatrix original).  Any
calls to setMatrix() need to be made relative to this new space.

All other ops already operate in relative coordinates.

This should let us not fiddle with setMatrix().

BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2247353003

Committed:
https://skia.googlesource.com/skia/+/6480619f9ff4676a97edae41b2ca23eadeddcfbf
==========

[commit-bot: I haz the power, 2016-08-17 00:18:31]

Committed patchset #1 (id:1) as
https://skia.googlesource.com/skia/+/6480619f9ff4676a97edae41b2ca23eadeddcfbf