Add tests for SkCanvas::drawImage[Rect]()

Add tests for SkCanvas::drawImage[Rect]()

BUG=skia:2947

[Rémi Piotaix, 2014-09-29 22:26:05]

piotaixr@chromium.org changed reviewers:
+ bsalomon@chromium.org, junov@chromium.org, reed@google.com

[Rémi Piotaix, 2014-09-29 22:26:19]

PTAL

[reed1, 2014-09-30 13:44:55]

This test is already very big (and therefore a bit hard to read/parse).

Independent of unittesting, I think a GM is in order to exercise the
drawImage[Rect] apis, using multiple image types and surface backends.

https://codereview.chromium.org/614773004/diff/1/tests/CanvasTest.cpp
File tests/CanvasTest.cpp (right):

https://codereview.chromium.org/614773004/diff/1/tests/CanvasTest.cpp#newcode271
tests/CanvasTest.cpp:271: SkImage* kTestImage;
ugh -- can we find a way to not use these globals?

[Rémi Piotaix, 2014-09-30 15:44:50]

This test may be big but is a good way to test the consistency between SkCanvas
and its subclasses.

https://codereview.chromium.org/614773004/diff/1/tests/CanvasTest.cpp
File tests/CanvasTest.cpp (right):

https://codereview.chromium.org/614773004/diff/1/tests/CanvasTest.cpp#newcode271
tests/CanvasTest.cpp:271: SkImage* kTestImage;
On 2014/09/30 13:44:55, reed1 wrote:
> ugh -- can we find a way to not use these globals?

Seems that we can't.
The use of the same bitmap/image/surface is needed by this test to test
consistency between deterrent canvas types or sth. like that.

[reed1, 2014-09-30 17:09:31]

On 2014/09/30 15:44:50, Rémi Piotaix wrote:
> This test may be big but is a good way to test the consistency between
SkCanvas
> and its subclasses.

Is your change testing against different canvas / image subclasses?

> 
> https://codereview.chromium.org/614773004/diff/1/tests/CanvasTest.cpp
> File tests/CanvasTest.cpp (right):
> 
>
https://codereview.chromium.org/614773004/diff/1/tests/CanvasTest.cpp#newcode271
> tests/CanvasTest.cpp:271: SkImage* kTestImage;
> On 2014/09/30 13:44:55, reed1 wrote:
> > ugh -- can we find a way to not use these globals?
> 
> Seems that we can't.
> The use of the same bitmap/image/surface is needed by this test to test
> consistency between deterrent canvas types or sth. like that.

Didn't need globals before, is there really no way to order the code to allow
sharing w/o resorting to globals?

[Rémi Piotaix, 2014-09-30 17:48:56]

> Is your change testing against different canvas / image subclasses?

The test compares the result between the base implementation in SkCanvas and the
implementation in DeferredCanvas. It is also testing the deprecated path (urgh)
for SkPicture (maybe i should change that) and the PDF device.

It is not tested with the different kinds of SkImages.

> Didn't need globals before, is there really no way to order the code to allow
> sharing w/o resorting to globals?

I don't understand what you men by that. There are constants/globals from line
204 to line 271

[Justin Novosad, 2014-09-30 18:06:47]

https://codereview.chromium.org/614773004/diff/1/tests/CanvasTest.cpp
File tests/CanvasTest.cpp (right):

https://codereview.chromium.org/614773004/diff/1/tests/CanvasTest.cpp#newcode912
tests/CanvasTest.cpp:912: kTestImage = kTestSurface->newImageSnapshot();
I guess these globals could be packaged into a local struct that gets passed to
the test functions

[Rémi Piotaix, 2014-09-30 18:28:54]

> It is not tested with the different kinds of SkImages.

But it is done in gm/image.cpp ;-)

[Rémi Piotaix, 2014-10-02 18:45:24]

https://codereview.chromium.org/614773004/diff/1/tests/CanvasTest.cpp
File tests/CanvasTest.cpp (right):

https://codereview.chromium.org/614773004/diff/1/tests/CanvasTest.cpp#newcode912
tests/CanvasTest.cpp:912: kTestImage = kTestSurface->newImageSnapshot();
On 2014/09/30 18:06:46, junov wrote:
> I guess these globals could be packaged into a local struct that gets passed
to
> the test functions

Done here: https://codereview.chromium.org/617093004/

[Rémi Piotaix, 2014-10-03 20:18:08]

New patch :)

[reed1, 2014-10-06 12:46:09]

is the image that is being tested a single color? (from createSurface)? If so,
how does that test drawImageRect very well?

[Rémi Piotaix, 2014-10-06 15:04:47]

On 2014/10/06 12:46:09, reed1 wrote:
> is the image that is being tested a single color? (from createSurface)? If so,
> how does that test drawImageRect very well?

Yeah, true :S

I copied (more or less succesfully) copied the DrawBitmap[Rect] test steps.
Should we change them as well?

[Rémi Piotaix, 2014-10-08 17:16:37]

Ping?

[Rémi Piotaix, 2014-10-14 19:17:31]

On 2014/10/08 17:16:37, Rémi Piotaix wrote:
> Ping?

Re-ping?

[Justin Novosad, 2014-10-14 19:27:22]

On 2014/10/06 12:46:09, reed1 wrote:
> is the image that is being tested a single color? (from createSurface)? If so,
> how does that test drawImageRect very well?

We have GMs to verify correct raterization. This unit test is more about
verifying that the call goes through successfully, with a bunch of different
configurations.

[reed1, 2014-10-15 14:16:48]

On 2014/10/14 19:27:22, junov wrote:
> On 2014/10/06 12:46:09, reed1 wrote:
> > is the image that is being tested a single color? (from createSurface)? If
so,
> > how does that test drawImageRect very well?
> 
> We have GMs to verify correct raterization. This unit test is more about
> verifying that the call goes through successfully, with a bunch of different
> configurations.

I see. Actually, I'm not sure I see how this tests works in general. Since we
have GMs that exercise these draw calls, and the GMs run with all our backends
(raster, gpu, deferred, picture, pipe) what is this test covering that is not
already there?

[reed1, 2014-10-15 14:17:28]

lgtm for this change, but I would like to understand how this test works, and
its value.

[Justin Novosad, 2014-10-15 14:32:32]

On 2014/10/15 14:17:28, reed1 wrote:
> lgtm for this change, but I would like to understand how this test works, and
> its value.

The tests focus on validating SkCanvas state rather than pixels produced.  For
example when you run command X on an SkPictureRecord, and query it state
(Matrix, clip stack, layer count, etc.) It should be the same as if you ran
command X on a regular SkCanvas; and when you do the playback, the state of the
playback canvas should also be the same.  These tests helped identify and
correct side effects in many flavors of canvas.

[reed1, 2014-10-15 14:44:37]

On 2014/10/15 14:32:32, junov wrote:
> On 2014/10/15 14:17:28, reed1 wrote:
> > lgtm for this change, but I would like to understand how this test works,
and
> > its value.
> 
> The tests focus on validating SkCanvas state rather than pixels produced.  For
> example when you run command X on an SkPictureRecord, and query it state
> (Matrix, clip stack, layer count, etc.) It should be the same as if you ran
> command X on a regular SkCanvas; and when you do the playback, the state of
the
> playback canvas should also be the same.  These tests helped identify and
> correct side effects in many flavors of canvas.

That makes sense (I think) for state you can query (e.g. total-matrix). What
"state" are we checking when we make a draw-call? It seems to me that only state
is in its results.

[Justin Novosad, 2014-10-15 15:01:20]

On 2014/10/15 14:44:37, reed1 wrote:
> On 2014/10/15 14:32:32, junov wrote:
> > On 2014/10/15 14:17:28, reed1 wrote:
> > > lgtm for this change, but I would like to understand how this test works,
> and
> > > its value.
> > 
> > The tests focus on validating SkCanvas state rather than pixels produced. 
For
> > example when you run command X on an SkPictureRecord, and query it state
> > (Matrix, clip stack, layer count, etc.) It should be the same as if you ran
> > command X on a regular SkCanvas; and when you do the playback, the state of
> the
> > playback canvas should also be the same.  These tests helped identify and
> > correct side effects in many flavors of canvas.
> 
> That makes sense (I think) for state you can query (e.g. total-matrix). What
> "state" are we checking when we make a draw-call? It seems to me that only
state
> is in its results.

So this test is less interesting for draw calls, which are not expected to
affect state, but there have been cases where they did, as a side effect.  The
test is mostly interesting for state-changing calls.
The state we check is:

deviceSize,
saveCount,
isDrawingToLayer,
getClipBounds (return value and bounds)
getClipDeviceBounds (return value and bounds)
getDrawFilter
getTotalMatrix
On the clip: isEmpty(), isRect()

[reed1, 2014-10-15 15:03:08]

On 2014/10/15 15:01:20, junov wrote:
> On 2014/10/15 14:44:37, reed1 wrote:
> > On 2014/10/15 14:32:32, junov wrote:
> > > On 2014/10/15 14:17:28, reed1 wrote:
> > > > lgtm for this change, but I would like to understand how this test
works,
> > and
> > > > its value.
> > > 
> > > The tests focus on validating SkCanvas state rather than pixels produced. 
> For
> > > example when you run command X on an SkPictureRecord, and query it state
> > > (Matrix, clip stack, layer count, etc.) It should be the same as if you
ran
> > > command X on a regular SkCanvas; and when you do the playback, the state
of
> > the
> > > playback canvas should also be the same.  These tests helped identify and
> > > correct side effects in many flavors of canvas.
> > 
> > That makes sense (I think) for state you can query (e.g. total-matrix). What
> > "state" are we checking when we make a draw-call? It seems to me that only
> state
> > is in its results.
> 
> So this test is less interesting for draw calls, which are not expected to
> affect state, but there have been cases where they did, as a side effect.  The
> test is mostly interesting for state-changing calls.
> The state we check is:
> 
> deviceSize,
> saveCount,
> isDrawingToLayer,
> getClipBounds (return value and bounds)
> getClipDeviceBounds (return value and bounds)
> getDrawFilter
> getTotalMatrix
> On the clip: isEmpty(), isRect()

Agreed. Perhaps we should remove the tests that are not useful (i.e. drawing
calls), just to make the test itself more clear and readable.