Adding a PictureResolution option to SkPictureImageFilter

Adding a PictureResolution option to SkPictureImageFilter

This change adds an option to SkPictureImageFilter to make it
rasterize SkPicture in a resolution that matches the local coordinate
space (equivalent to the record-time device space).

BUG=skia:3176
R=reed@google.com, senorblanco@chromium.org

Committed: https://skia.googlesource.com/skia/+/5234075b1c6bcada4ad17ed5a83bfcb53df66b7f

[Justin Novosad, 2014-11-27 16:45:26]

junov@chromium.org changed reviewers:
+ reed@chromium.org, senorblanco@chromium.org

[Justin Novosad, 2014-11-27 16:45:26]

PTAL

[reed2, 2014-11-28 15:33:53]

https://codereview.chromium.org/753073010/diff/1/include/effects/SkPictureIma...
File include/effects/SkPictureImageFilter.h (right):

https://codereview.chromium.org/753073010/diff/1/include/effects/SkPictureIma...
include/effects/SkPictureImageFilter.h:35: static SkPictureImageFilter*
Create(const SkPicture* picture, int32_t uniqueID = 0,
api nit: can we not have so many default arguments?

same comment for the next factory.

[reed2, 2014-11-28 15:38:23]

Can we get away with just another factory that explicitly is for
local-space-filtering? Is it important to have the full flexibility for
filter-level as well here?

would this be enough?

CreateForLocalSpace(...);   // filters w/ bilerp

[Justin Novosad, 2014-11-28 16:41:08]

On 2014/11/28 15:38:23, reed2 wrote:
> Can we get away with just another factory that explicitly is for
> local-space-filtering? Is it important to have the full flexibility for
> filter-level as well here?
> 
> would this be enough?
> 
> CreateForLocalSpace(...);   // filters w/ bilerp

Technically, yes that would be enough. However, the fact that we only ever want
to use this with kLow_FilterLevel is because that is what Chrome requires today.
I was trying to avoid hard-coding Chrome-specific choices.

Anyways, I can roll that bit back.  No problem.

[Justin Novosad, 2014-11-28 18:51:14]

New patch

[reed1, 2014-12-01 14:05:20]

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

[reed1, 2014-12-01 14:05:20]

thank you, I find the new APIs much clearer/simpler. api lgtm

[Stephen White, 2014-12-01 16:02:12]

Code looks fine; just some naming nits (although I don't have any better
suggestions).

https://codereview.chromium.org/753073010/diff/20001/include/effects/SkPictur...
File include/effects/SkPictureImageFilter.h (right):

https://codereview.chromium.org/753073010/diff/20001/include/effects/SkPictur...
include/effects/SkPictureImageFilter.h:52: kLocalSpace_PictureResolution
Is "local space" the usual Skia nomenclature for non-device space?

https://codereview.chromium.org/753073010/diff/20001/include/effects/SkPictur...
include/effects/SkPictureImageFilter.h:75: void
drawPictureAtNativeResolution(Proxy*, SkBaseDevice*, const SkIRect&
deviceBounds,
We have native, device, local and original. Is there a better/simpler naming we
could use?

https://codereview.chromium.org/753073010/diff/20001/src/effects/SkPictureIma...
File src/effects/SkPictureImageFilter.cpp (right):

https://codereview.chromium.org/753073010/diff/20001/src/effects/SkPictureIma...
src/effects/SkPictureImageFilter.cpp:174: //canvas.drawPicture(fPicture);
Commented-out code; please remove.

[Justin Novosad, 2014-12-02 16:33:37]

https://codereview.chromium.org/753073010/diff/20001/include/effects/SkPictur...
File include/effects/SkPictureImageFilter.h (right):

https://codereview.chromium.org/753073010/diff/20001/include/effects/SkPictur...
include/effects/SkPictureImageFilter.h:52: kLocalSpace_PictureResolution
On 2014/12/01 16:02:12, Stephen White wrote:
> Is "local space" the usual Skia nomenclature for non-device space?

It is standard nomenclature in computer graphics.
Local space = the space that is current. In the scene graph metaphor, it is the
space of the current node during a traversal, as opposed to "World Space" which
is the space of the root node (post-transform). 

BTW, in the case of SkCanvas (and many other 2D APIs), there is no distinciton
between world transforms and projections (a.k.a camera tranforms). Therefore
there is no practical distinction between "world space" and "device space" in
skia, at least as far as the API is concerned.

https://codereview.chromium.org/753073010/diff/20001/include/effects/SkPictur...
include/effects/SkPictureImageFilter.h:75: void
drawPictureAtNativeResolution(Proxy*, SkBaseDevice*, const SkIRect&
deviceBounds,
On 2014/12/01 16:02:12, Stephen White wrote:
> We have native, device, local and original. Is there a better/simpler naming
we
> could use?

Acknowledged.

[Justin Novosad, 2014-12-02 16:50:12]

New patch. senorblanco?

[Stephen White, 2014-12-02 17:56:12]

LGTM

[Justin Novosad, 2014-12-02 18:54:04]

The CQ bit was checked by junov@chromium.org

[commit-bot: I haz the power, 2014-12-02 18:54:45]

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

[commit-bot: I haz the power, 2014-12-02 18:54:47]

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

[commit-bot: I haz the power, 2014-12-02 18:54:48]

Failed to apply patch for expectations/gm/ignored-tests.txt:
While running git apply --index -3 -p1;
  error: patch failed: expectations/gm/ignored-tests.txt:57
  Falling back to three-way merge...
  Applied patch to 'expectations/gm/ignored-tests.txt' with conflicts.
  U expectations/gm/ignored-tests.txt

Patch:       expectations/gm/ignored-tests.txt
Index: expectations/gm/ignored-tests.txt
diff --git a/expectations/gm/ignored-tests.txt
b/expectations/gm/ignored-tests.txt
index
4b7e75001e5e6d2e461f42980586a7a7badda389..10d19427fb7e38c38b0608a0d269e3e3a9e7be36
100644
--- a/expectations/gm/ignored-tests.txt
+++ b/expectations/gm/ignored-tests.txt
@@ -57,5 +57,5 @@ dashcubics
 #mtklein
 verylargebitmap
 
-
-
+#junov skbug.com/3176
+pictureimagefilter

[Justin Novosad, 2014-12-02 19:25:05]

The CQ bit was checked by junov@chromium.org

[commit-bot: I haz the power, 2014-12-02 19:26:05]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/753073010/60001

[commit-bot: I haz the power, 2014-12-02 19:26:09]

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

[commit-bot: I haz the power, 2014-12-02 19:26:10]

Failed to apply patch for expectations/gm/ignored-tests.txt:
While running git apply --index -3 -p1;
  error: patch failed: expectations/gm/ignored-tests.txt:61
  Falling back to three-way merge...
  Applied patch to 'expectations/gm/ignored-tests.txt' with conflicts.
  U expectations/gm/ignored-tests.txt

Patch:       expectations/gm/ignored-tests.txt
Index: expectations/gm/ignored-tests.txt
diff --git a/expectations/gm/ignored-tests.txt
b/expectations/gm/ignored-tests.txt
index
66ddaa6b06a3e0dbff22364c37a3525fa50f1e1a..ea743af40d36f0ef27cd9fbed194fd691ea57233
100644
--- a/expectations/gm/ignored-tests.txt
+++ b/expectations/gm/ignored-tests.txt
@@ -61,3 +61,5 @@ verylargebitmap
 multipicturedraw_sierpinski_simple
 multipicturedraw_sierpinski_tiled
 
+#junov skbug.com/3176
+pictureimagefilter

[Justin Novosad, 2014-12-02 19:41:46]

The CQ bit was checked by junov@chromium.org

[commit-bot: I haz the power, 2014-12-02 19:42:15]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/753073010/80001

[commit-bot: I haz the power, 2014-12-02 19:45:46]

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

[commit-bot: I haz the power, 2014-12-02 19:45:47]

Try jobs failed on following builders:
  Build-Win-VS2013-x86-Debug-Trybot on client.skia.compile
(http://build.chromium.org/p/client.skia.compile/builders/Build-Win-VS2013-x86...)

[Justin Novosad, 2014-12-02 19:50:47]

On 2014/12/02 19:45:47, I haz the power (commit-bot) wrote:
> Try jobs failed on following builders:
>   Build-Win-VS2013-x86-Debug-Trybot on client.skia.compile
>
(http://build.chromium.org/p/client.skia.compile/builders/Build-Win-VS2013-x86...)

The windows bot is choking on a merge conflict that I already resolved. Not sure
what this is about.

[Justin Novosad, 2014-12-02 19:51:01]

Committed patchset #5 (id:80001) manually as
5234075b1c6bcada4ad17ed5a83bfcb53df66b7f (presubmit successful).