SkMatrix::preScale() is too conservative

SkMatrix::preScale() is too conservative

SkMatrix::preScale() always sets the kScale bit, which means something
like

  m = SkMatrix::MakeScale(2, 2);
  m.preScale(0.5, 0.5);

leaves m.getType() == kScale_Mask, and can throw off the bitmap proc
heuristics.

We could detect the inverse case and clear the scale bit instead.

R=reed@google.com,caryclark@google.com
GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&issue=1531323002

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

[f(malita), 2015-12-17 17:56:57]

Description was changed from

==========
Simplify the matrix type when applying an inverse scale using preScale()

SkMatrix::preScale() always sets the kScale bit, which means something
like

  m = SkMatrix::MakeScale(2, 2);
  m.preScale(0.5, 0.5);

leaves m.getType() == kScale_Mask, and can throw off some bitmap proc
heuristics.

We could detect the inverse case and clear the scale bit instead.

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

to

==========
Simplify the matrix type when applying an inverse scale using preScale()

SkMatrix::preScale() always sets the kScale bit, which means something
like

  m = SkMatrix::MakeScale(2, 2);
  m.preScale(0.5, 0.5);

leaves m.getType() == kScale_Mask, and can throw off some bitmap proc
heuristics.

We could detect the inverse case and clear the scale bit instead.

R=reed@google.com,caryclark@google.com
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========

[f(malita), 2015-12-17 18:00:54]

Description was changed from

==========
Simplify the matrix type when applying an inverse scale using preScale()

SkMatrix::preScale() always sets the kScale bit, which means something
like

  m = SkMatrix::MakeScale(2, 2);
  m.preScale(0.5, 0.5);

leaves m.getType() == kScale_Mask, and can throw off some bitmap proc
heuristics.

We could detect the inverse case and clear the scale bit instead.

R=reed@google.com,caryclark@google.com
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========

to

==========
SkMatrix::preScale() is too conservative

SkMatrix::preScale() always sets the kScale bit, which means something
like

  m = SkMatrix::MakeScale(2, 2);
  m.preScale(0.5, 0.5);

leaves m.getType() == kScale_Mask, and can throw off the bitmap proc
heuristics.

We could detect the inverse case and clear the scale bit instead.

R=reed@google.com,caryclark@google.com
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========

[f(malita), 2015-12-17 18:08:01]

I ran into this while trying to figure why picture shaders are always
bilerp-filtered even though their tiles are rasterized at the correct rez/CTM
most of the time.

Alternatively, I could just update the picture shader impl to avoid preScale()
(use setScale() or something), but it seems odd to have such a landmine - and
other code paths may benefit too.

[caryclark, 2015-12-17 18:14:40]

lgtm

In general, I wonder if matrix should have a dirty bit so it can lazily evaluate
the matrix order when it is read... May not be worth the complexity

[f(malita), 2015-12-17 18:19:32]

On 2015/12/17 18:14:40, caryclark wrote:
> lgtm
> 
> In general, I wonder if matrix should have a dirty bit so it can lazily
evaluate
> the matrix order when it is read... May not be worth the complexity

I think it does (kUnknown), so we could also just setTypeMask(kUnknown) and
leave the persp/skew heuristics out.

But my understanding is this preScale version attempts to avoid a full mask
recalc.

[reed1, 2015-12-17 18:23:37]

can't we always clear the scale bit if both scales are 1?

[f(malita), 2015-12-17 18:26:05]

On 2015/12/17 18:23:37, reed1 wrote:
> can't we always clear the scale bit if both scales are 1?

We could, but that's inconsistent with how we compute the mask when a persp/skew
is present.

Looking at computeTypeMask, it seems we always set kScale when these are
present:
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/skia/s...

[f(malita), 2015-12-17 18:30:07]

On 2015/12/17 18:26:05, f(malita) wrote:
> On 2015/12/17 18:23:37, reed1 wrote:
> > can't we always clear the scale bit if both scales are 1?
> 
> We could, but that's inconsistent with how we compute the mask when a
persp/skew
> is present.
> 
> Looking at computeTypeMask, it seems we always set kScale when these are
> present:
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/skia/s...

OTOH duplicating that logic here is also fragile, so maybe I should take Cary's
hint and just setMaskType(kUnknown) in this case?

We'd be triggering a full mask recalc, but only when inverse-scaling.

[reed1, 2015-12-17 19:07:05]

lgtm

[f(malita), 2015-12-17 20:53:38]

The CQ bit was checked by fmalita@chromium.org

[f(malita), 2015-12-17 20:53:39]

The patchset sent to the CQ was uploaded after l-g-t-m from caryclark@google.com
Link to the patchset: https://codereview.chromium.org/1531323002/#ps40001
(title: "comment + guard")

[commit-bot: I haz the power, 2015-12-17 20:53:53]

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

[commit-bot: I haz the power, 2015-12-17 22:12:32]

Description was changed from

==========
SkMatrix::preScale() is too conservative

SkMatrix::preScale() always sets the kScale bit, which means something
like

  m = SkMatrix::MakeScale(2, 2);
  m.preScale(0.5, 0.5);

leaves m.getType() == kScale_Mask, and can throw off the bitmap proc
heuristics.

We could detect the inverse case and clear the scale bit instead.

R=reed@google.com,caryclark@google.com
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========

to

==========
SkMatrix::preScale() is too conservative

SkMatrix::preScale() always sets the kScale bit, which means something
like

  m = SkMatrix::MakeScale(2, 2);
  m.preScale(0.5, 0.5);

leaves m.getType() == kScale_Mask, and can throw off the bitmap proc
heuristics.

We could detect the inverse case and clear the scale bit instead.

R=reed@google.com,caryclark@google.com
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...

Committed:
https://skia.googlesource.com/skia/+/a379da41b7b0408c834db967c0034fd7f611aa83
==========

[commit-bot: I haz the power, 2015-12-17 22:12:33]

Committed patchset #3 (id:40001) as
https://skia.googlesource.com/skia/+/a379da41b7b0408c834db967c0034fd7f611aa83