Consolidate SkCanvas matrix virtuals.

Consolidate SkCanvas matrix virtuals.

Remove didTranslate, didScale, didRotate & didSkew, and rely on
didConcat instead. Subclasses can sniff the matrix type if they want to
differentiate.

(work in progress)

R=reed@google.com, robertphillips@google.com

Committed: http://code.google.com/p/skia/source/detail?r=13940

[f(malita), 2014-03-18 14:51:42]

First stab at reducing the number of matrix-related virtuals.

Since we don't currently track rotation & skew bits, this removes the ability to
handle these two ops separately. The question is whether we need to preserve
that ability (and what's the best way to achieve it).

There is also a record perf impact question still unanswered.

[robertphillips, 2014-03-18 14:59:38]

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

https://codereview.chromium.org/203203004/diff/1/src/core/SkPictureRecord.cpp...
src/core/SkPictureRecord.cpp:700: break;
Can we not check for the rotate special case here?

[reed1, 2014-03-18 15:13:16]

api lgtm

once this sort of thing lands, I can't wait to add/change the "helpers" we
expose...
- rotateDegrees, rotateRadians
...

[reed1, 2014-03-18 15:13:47]

next CL -- DEATH TO SETMATRIX

[reed1, 2014-03-18 15:19:43]

I had a different thought for SkCanvas.cpp -- what do you think of this pattern:

void SkCanvas::translate(x, y) {
    SkMatrix m;
    m.setTranslate(x, y);
    this->concat(m);
}

[f(malita), 2014-03-18 15:31:09]

On 2014/03/18 15:19:43, reed1 wrote:
> I had a different thought for SkCanvas.cpp -- what do you think of this
pattern:
> 
> void SkCanvas::translate(x, y) {
>     SkMatrix m;
>     m.setTranslate(x, y);
>     this->concat(m);
> }

Looks better, but I'm somewhat concerned we'd be missing some SkMatrix
optimizations by taking the generic preConcat path. Maybe nothing to worry
about, will update.

[reed1, 2014-03-18 15:32:38]

On 2014/03/18 15:31:09, Florin Malita wrote:
> On 2014/03/18 15:19:43, reed1 wrote:
> > I had a different thought for SkCanvas.cpp -- what do you think of this
> pattern:
> > 
> > void SkCanvas::translate(x, y) {
> >     SkMatrix m;
> >     m.setTranslate(x, y);
> >     this->concat(m);
> > }
> 
> Looks better, but I'm somewhat concerned we'd be missing some SkMatrix
> optimizations by taking the generic preConcat path. Maybe nothing to worry
> about, will update.

All true. When we can time any of this, we might try timing both patterns.

[f(malita), 2014-03-21 16:17:46]

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

https://codereview.chromium.org/203203004/diff/1/src/core/SkPictureRecord.cpp...
src/core/SkPictureRecord.cpp:700: break;
On 2014/03/18 14:59:38, robertphillips wrote:
> Can we not check for the rotate special case here?

We currently only track an 'affine' bit. We could add a 'rotate' (and 'skew')
initial bit as discussed and then figure the angle, but I'm not sure whether  an
acos is actually cheaper than shoving the whole matrix in. Maybe something to
try later?

[f(malita), 2014-03-21 16:21:47]

On 2014/03/21 16:17:46, Florin Malita wrote:
> https://codereview.chromium.org/203203004/diff/1/src/core/SkPictureRecord.cpp
> File src/core/SkPictureRecord.cpp (right):
> 
>
https://codereview.chromium.org/203203004/diff/1/src/core/SkPictureRecord.cpp...
> src/core/SkPictureRecord.cpp:700: break;
> On 2014/03/18 14:59:38, robertphillips wrote:
> > Can we not check for the rotate special case here?
> 
> We currently only track an 'affine' bit. We could add a 'rotate' (and 'skew')
> initial bit as discussed and then figure the angle, but I'm not sure whether 
an
> acos is actually cheaper than shoving the whole matrix in. Maybe something to
> try later?

Telemetry numbers are in (two runs):

https://storage.cloud.google.com/chromium-skia-gm/telemetry/tryserver-outputs...
https://storage.cloud.google.com/chromium-skia-gm/telemetry/tryserver-outputs...

I think we're within noise levels:

record_time: -1.668%, +0.446%
rasterize_time: -0.132%, +0.278%

Now on to try a couple of runs with Mike's suggested change.

[f(malita), 2014-03-24 20:05:03]

On 2014/03/21 16:21:47, Florin Malita wrote:
> On 2014/03/21 16:17:46, Florin Malita wrote:
> >
https://codereview.chromium.org/203203004/diff/1/src/core/SkPictureRecord.cpp
> > File src/core/SkPictureRecord.cpp (right):
> > 
> >
>
https://codereview.chromium.org/203203004/diff/1/src/core/SkPictureRecord.cpp...
> > src/core/SkPictureRecord.cpp:700: break;
> > On 2014/03/18 14:59:38, robertphillips wrote:
> > > Can we not check for the rotate special case here?
> > 
> > We currently only track an 'affine' bit. We could add a 'rotate' (and
'skew')
> > initial bit as discussed and then figure the angle, but I'm not sure whether

> an
> > acos is actually cheaper than shoving the whole matrix in. Maybe something
to
> > try later?
> 
> Telemetry numbers are in (two runs):
> 
>
https://storage.cloud.google.com/chromium-skia-gm/telemetry/tryserver-outputs...
>
https://storage.cloud.google.com/chromium-skia-gm/telemetry/tryserver-outputs...
> 
> I think we're within noise levels:
> 
> record_time: -1.668%, +0.446%
> rasterize_time: -0.132%, +0.278%
> 
> Now on to try a couple of runs with Mike's suggested change.

Cluster Telemetry numbers with the updated version still look ok, if noisy:

https://storage.cloud.google.com/chromium-skia-gm/telemetry/tryserver-outputs...
https://storage.cloud.google.com/chromium-skia-gm/telemetry/tryserver-outputs...

record_time: -0.100%, +2.645%
rasterize_time: -0.050%, +0.023%

To make sure, I also ran it locally for the top10 page set: no measurable
difference. So I think we should be good to land it.

[robertphillips, 2014-03-25 15:44:35]

lgtm with nits

https://codereview.chromium.org/203203004/diff/20001/src/core/SkPictureRecord...
File src/core/SkPictureRecord.cpp (right):

https://codereview.chromium.org/203203004/diff/20001/src/core/SkPictureRecord...
src/core/SkPictureRecord.cpp:694: switch (matrix.getType()) {
cases are supposed to be indented.

https://codereview.chromium.org/203203004/diff/20001/src/pipe/SkGPipeWrite.cpp
File src/pipe/SkGPipeWrite.cpp (right):

https://codereview.chromium.org/203203004/diff/20001/src/pipe/SkGPipeWrite.cp...
src/pipe/SkGPipeWrite.cpp:600: NOTIFY_SETUP(this);
case indent.

https://codereview.chromium.org/203203004/diff/20001/src/utils/SkDumpCanvas.cpp
File src/utils/SkDumpCanvas.cpp (right):

https://codereview.chromium.org/203203004/diff/20001/src/utils/SkDumpCanvas.c...
src/utils/SkDumpCanvas.cpp:228: switch (matrix.getType()) {
same here.

https://codereview.chromium.org/203203004/diff/20001/src/utils/SkLuaCanvas.cpp
File src/utils/SkLuaCanvas.cpp (right):

https://codereview.chromium.org/203203004/diff/20001/src/utils/SkLuaCanvas.cp...
src/utils/SkLuaCanvas.cpp:110: switch (matrix.getType()) {
same here

https://codereview.chromium.org/203203004/diff/20001/src/utils/SkLuaCanvas.cp...
src/utils/SkLuaCanvas.cpp:114: lua.pushScalar(matrix.getTranslateY(), "dy");
break goes before the } when you do this.

https://codereview.chromium.org/203203004/diff/20001/src/utils/debugger/SkDeb...
File src/utils/debugger/SkDebugCanvas.cpp (right):

https://codereview.chromium.org/203203004/diff/20001/src/utils/debugger/SkDeb...
src/utils/debugger/SkDebugCanvas.cpp:413: switch (matrix.getType()) {
indent

https://codereview.chromium.org/203203004/diff/20001/src/utils/debugger/SkDeb...
src/utils/debugger/SkDebugCanvas.cpp:422: default:
this->

[f(malita), 2014-03-25 15:58:07]

https://codereview.chromium.org/203203004/diff/20001/src/core/SkPictureRecord...
File src/core/SkPictureRecord.cpp (right):

https://codereview.chromium.org/203203004/diff/20001/src/core/SkPictureRecord...
src/core/SkPictureRecord.cpp:694: switch (matrix.getType()) {
On 2014/03/25 15:44:36, robertphillips wrote:
> cases are supposed to be indented.

Done.

https://codereview.chromium.org/203203004/diff/20001/src/pipe/SkGPipeWrite.cpp
File src/pipe/SkGPipeWrite.cpp (right):

https://codereview.chromium.org/203203004/diff/20001/src/pipe/SkGPipeWrite.cp...
src/pipe/SkGPipeWrite.cpp:600: NOTIFY_SETUP(this);
On 2014/03/25 15:44:36, robertphillips wrote:
> case indent.

Done.

https://codereview.chromium.org/203203004/diff/20001/src/utils/SkDumpCanvas.cpp
File src/utils/SkDumpCanvas.cpp (right):

https://codereview.chromium.org/203203004/diff/20001/src/utils/SkDumpCanvas.c...
src/utils/SkDumpCanvas.cpp:228: switch (matrix.getType()) {
On 2014/03/25 15:44:36, robertphillips wrote:
> same here.

Done.

https://codereview.chromium.org/203203004/diff/20001/src/utils/SkLuaCanvas.cpp
File src/utils/SkLuaCanvas.cpp (right):

https://codereview.chromium.org/203203004/diff/20001/src/utils/SkLuaCanvas.cp...
src/utils/SkLuaCanvas.cpp:110: switch (matrix.getType()) {
On 2014/03/25 15:44:36, robertphillips wrote:
> same here

Done.

https://codereview.chromium.org/203203004/diff/20001/src/utils/SkLuaCanvas.cp...
src/utils/SkLuaCanvas.cpp:114: lua.pushScalar(matrix.getTranslateY(), "dy");
On 2014/03/25 15:44:36, robertphillips wrote:
> break goes before the } when you do this.

Done.

https://codereview.chromium.org/203203004/diff/20001/src/utils/debugger/SkDeb...
File src/utils/debugger/SkDebugCanvas.cpp (right):

https://codereview.chromium.org/203203004/diff/20001/src/utils/debugger/SkDeb...
src/utils/debugger/SkDebugCanvas.cpp:413: switch (matrix.getType()) {
On 2014/03/25 15:44:36, robertphillips wrote:
> indent

Done.

https://codereview.chromium.org/203203004/diff/20001/src/utils/debugger/SkDeb...
src/utils/debugger/SkDebugCanvas.cpp:422: default:
On 2014/03/25 15:44:36, robertphillips wrote:
> this->

Done.

[f(malita), 2014-03-25 15:58:12]

The CQ bit was checked by fmalita@chromium.org

[commit-bot: I haz the power, 2014-03-25 15:58:22]

CQ is trying da patch. Follow status at
https://skia-tree-status.appspot.com/cq/fmalita@chromium.org/203203004/100001

[commit-bot: I haz the power, 2014-03-25 17:32:42]

Change committed as 13940