Override SkCanvas::drawImage() in SkRecorder.

Override SkCanvas::drawImage() in SkRecorder.

BUG=skia:2947
Committed: https://skia.googlesource.com/skia/+/65151754b9fdb6a968d7307764c20655d1b680a0

[Rémi Piotaix, 2014-09-26 21:29:14]

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

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

PTAL

[reed1, 2014-09-26 21:32:25]

reed@google.com changed reviewers:
+ mtklein@google.com, robertphillips@google.com

[reed1, 2014-09-26 21:32:25]

adding the picture experts

[reed1, 2014-09-26 21:33:05]

how much more to update other SkCanvas subclasses? (Pipe, Deferred, NWay, etc.)?

[Rémi Piotaix, 2014-09-26 21:50:07]

On 2014/09/26 21:33:05, reed1 wrote:
> how much more to update other SkCanvas subclasses? (Pipe, Deferred, NWay,
etc.)?

NVay is here: https://codereview.chromium.org/600643002/

Gpipe and DeferredCanvas are the next ones.

[mtklein, 2014-09-27 15:41:22]

The thrust of the new code looks good to me.  I have a few requests on the
tests.

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

https://codereview.chromium.org/610003002/diff/20001/src/core/SkRecordDraw.cp...
src/core/SkRecordDraw.cpp:346: return
this->adjustAndMap(SkRect::MakeXYWH(op.left, op.top, image->width(),
image->height()),
May be over 100 cols here?

https://codereview.chromium.org/610003002/diff/20001/tests/RecordDrawTest.cpp
File tests/RecordDrawTest.cpp (right):

https://codereview.chromium.org/610003002/diff/20001/tests/RecordDrawTest.cpp...
tests/RecordDrawTest.cpp:257: DEF_TEST(RecordDraw_drawImage, r){
I feel like this test may be taking on a bit too much at once.  I see at least 3
parts I'd prefer were tested separately:
  1) when we call SkRecorder::drawImage()/drawImageRect() we end up with the
correct DrawImage/DrawImageRect in the SkRecord (this can go in
tests/RecorderTest.cpp, and maybe is covered enough by the tests you've added
already)
  2) when we call SkRecordDraw on an SkRecord with DrawImage/DrawImageRect, it
calls SkCanvas::drawImage()/drawImageRect() correctly (this could be here)
  3) when we call SkCanvas::drawImage()/drawImageRect(), they proxy down to
drawBitmap()/drawBitmapRectToRect() correctly (this could go in CanvasTest, and
doesn't really need to be part of this CL).

I'm not opposed to having these sort of e2e integration tests where they matter
(particularly, when they recreate a scenario that tests a regression), but the
space of e2e tests like this is very highly combinatoric, and can be hard to
manage over time if we write lots of them.  While it's all new and we still have
no indication of a need to test these pieces in this particular combination, I'd
rather we split up the tests into as focused units as possible.

https://codereview.chromium.org/610003002/diff/20001/tests/RecordReplaceDrawT...
File tests/RecordReplaceDrawTest.cpp (right):

https://codereview.chromium.org/610003002/diff/20001/tests/RecordReplaceDrawT...
tests/RecordReplaceDrawTest.cpp:119: assert_type<SkRecords::DrawImageRect>(r,
rerecord, 3);
Neat!

https://codereview.chromium.org/610003002/diff/20001/tests/RecorderTest.cpp
File tests/RecorderTest.cpp (right):

https://codereview.chromium.org/610003002/diff/20001/tests/RecorderTest.cpp#n...
tests/RecorderTest.cpp:159: int beginImageRefCount = image->getRefCnt();
We generally try to avoid calling getRefCnt().  Here in this test it's safe, but
it's a bad habit to get into (because unless it returns 1, it wasn't thread-safe
to call).

I'd prefer it if you can write your test using unique(), if only to use the less
powerful tool when possible:

Maybe like this?

<setup>
REPORTER_ASSERT(r, image->unique());
{
    SkRecord record;
    SkRecorder recorder(&record, ...); 
    recorder.drawImage(image, ...);
    REPORTER_ASSERT(r, !image->unique());  
    Tally tally;
    tally.apply(record);
    REPORTER_ASSERT(r, 1 == tally.count<SkRecords::DrawImage>());
}
REPORTER_ASSERT(r, image->unique());
{
    SkRecord record;
    SkRecorder recorder(&record, ...);
    recorder.drawImageRect(image, ...);
    REPORTER_ASSERT(r, !image->unique());
    Tally tally;
    tally.apply(record);
    REPORTER_ASSERT(r, 1 == tally.count<SkRecords::DrawImageRect>());
}
REPORTER_ASSERT(r, image->unique());

[robertphillips, 2014-09-29 17:07:56]

https://codereview.chromium.org/610003002/diff/20001/src/image/SkImagePriv.h
File src/image/SkImagePriv.h (right):

https://codereview.chromium.org/610003002/diff/20001/src/image/SkImagePriv.h#...
src/image/SkImagePriv.h:35: // in which case the surface may need to perform a
copy-on-write.
Can this return a "cosnt SkPixelRef*" ?

https://codereview.chromium.org/610003002/diff/20001/tests/CanvasTest.cpp
File tests/CanvasTest.cpp (right):

https://codereview.chromium.org/610003002/diff/20001/tests/CanvasTest.cpp#new...
tests/CanvasTest.cpp:154: 
create_bitmap ?

https://codereview.chromium.org/610003002/diff/20001/tests/CanvasTest.cpp#new...
tests/CanvasTest.cpp:159: 
create_surface ?

https://codereview.chromium.org/610003002/diff/20001/tests/RecordDrawTest.cpp
File tests/RecordDrawTest.cpp (right):

https://codereview.chromium.org/610003002/diff/20001/tests/RecordDrawTest.cpp...
tests/RecordDrawTest.cpp:260: SkCanvasMock(int width, int height) :
INHERITED(width, height) {
this->

https://codereview.chromium.org/610003002/diff/20001/tests/RecordDrawTest.cpp...
tests/RecordDrawTest.cpp:266: 
fDrawImageCalled

https://codereview.chromium.org/610003002/diff/20001/tests/RecordDrawTest.cpp...
tests/RecordDrawTest.cpp:297: const SkImage* fDrawingImage;
No '_'s in the names.

[Rémi Piotaix, 2014-09-29 17:57:55]

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

https://codereview.chromium.org/610003002/diff/20001/src/core/SkRecordDraw.cp...
src/core/SkRecordDraw.cpp:346: return
this->adjustAndMap(SkRect::MakeXYWH(op.left, op.top, image->width(),
image->height()),
On 2014/09/27 15:41:22, mtklein wrote:
> May be over 100 cols here?

Yes, 101 :-@

Done.

https://codereview.chromium.org/610003002/diff/20001/src/image/SkImagePriv.h
File src/image/SkImagePriv.h (right):

https://codereview.chromium.org/610003002/diff/20001/src/image/SkImagePriv.h#...
src/image/SkImagePriv.h:35: // in which case the surface may need to perform a
copy-on-write.
On 2014/09/29 17:07:55, robertphillips wrote:
> Can this return a "cosnt SkPixelRef*" ?

Done.

https://codereview.chromium.org/610003002/diff/20001/tests/CanvasTest.cpp
File tests/CanvasTest.cpp (right):

https://codereview.chromium.org/610003002/diff/20001/tests/CanvasTest.cpp#new...
tests/CanvasTest.cpp:154: 
On 2014/09/29 17:07:55, robertphillips wrote:
> create_bitmap ?

Done.

https://codereview.chromium.org/610003002/diff/20001/tests/CanvasTest.cpp#new...
tests/CanvasTest.cpp:159: 
On 2014/09/29 17:07:55, robertphillips wrote:
> create_surface ?

Done.

https://codereview.chromium.org/610003002/diff/20001/tests/RecordDrawTest.cpp
File tests/RecordDrawTest.cpp (right):

https://codereview.chromium.org/610003002/diff/20001/tests/RecordDrawTest.cpp...
tests/RecordDrawTest.cpp:257: DEF_TEST(RecordDraw_drawImage, r){
> 1) when we call SkRecorder::drawImage()/drawImageRect() we end up with
> the correct DrawImage/DrawImageRect in the SkRecord (this can go in
> tests/RecorderTest.cpp, and maybe is covered enough by the tests you've
> added already)

Yes, already done in RecorderTest.cpp, but this is not tested in this particular
test.

> 2) when we call SkRecordDraw on an SkRecord with DrawImage/DrawImageRect, it
> calls SkCanvas::drawImage()/drawImageRect() correctly (this could be here)

Ok, I just have to remove the part where I check the calls to drawBitmap[Rect].

> 3) when we call SkCanvas::drawImage()/drawImageRect(), they proxy down to
> drawBitmap()/drawBitmapRectToRect() correctly (this could go in CanvasTest,
> and doesn't really need to be part of this CL).

Actually, I don't even know why I did that because it will not be always the
case that a SkImage is backed by a bitmap...



Done.

https://codereview.chromium.org/610003002/diff/20001/tests/RecordDrawTest.cpp...
tests/RecordDrawTest.cpp:260: SkCanvasMock(int width, int height) :
INHERITED(width, height) {
On 2014/09/29 17:07:56, robertphillips wrote:
> this->

Done.

https://codereview.chromium.org/610003002/diff/20001/tests/RecordDrawTest.cpp...
tests/RecordDrawTest.cpp:266: 
On 2014/09/29 17:07:56, robertphillips wrote:
> fDrawImageCalled

Done.

https://codereview.chromium.org/610003002/diff/20001/tests/RecordDrawTest.cpp...
tests/RecordDrawTest.cpp:297: const SkImage* fDrawingImage;
On 2014/09/29 17:07:56, robertphillips wrote:
> No '_'s in the names.

Done.

https://codereview.chromium.org/610003002/diff/20001/tests/RecordReplaceDrawT...
File tests/RecordReplaceDrawTest.cpp (right):

https://codereview.chromium.org/610003002/diff/20001/tests/RecordReplaceDrawT...
tests/RecordReplaceDrawTest.cpp:119: assert_type<SkRecords::DrawImageRect>(r,
rerecord, 3);
On 2014/09/27 15:41:22, mtklein wrote:
> Neat!

Acknowledged.

https://codereview.chromium.org/610003002/diff/20001/tests/RecorderTest.cpp
File tests/RecorderTest.cpp (right):

https://codereview.chromium.org/610003002/diff/20001/tests/RecorderTest.cpp#n...
tests/RecorderTest.cpp:159: int beginImageRefCount = image->getRefCnt();
On 2014/09/27 15:41:22, mtklein wrote:
> We generally try to avoid calling getRefCnt().  Here in this test it's safe,
but
> it's a bad habit to get into (because unless it returns 1, it wasn't
thread-safe
> to call).
> 
> I'd prefer it if you can write your test using unique(), if only to use the
less
> powerful tool when possible:
> 
> Maybe like this?
> 
> <setup>
> REPORTER_ASSERT(r, image->unique());
> {
>     SkRecord record;
>     SkRecorder recorder(&record, ...); 
>     recorder.drawImage(image, ...);
>     REPORTER_ASSERT(r, !image->unique());  
>     Tally tally;
>     tally.apply(record);
>     REPORTER_ASSERT(r, 1 == tally.count<SkRecords::DrawImage>());
> }
> REPORTER_ASSERT(r, image->unique());
> {
>     SkRecord record;
>     SkRecorder recorder(&record, ...);
>     recorder.drawImageRect(image, ...);
>     REPORTER_ASSERT(r, !image->unique());
>     Tally tally;
>     tally.apply(record);
>     REPORTER_ASSERT(r, 1 == tally.count<SkRecords::DrawImageRect>());
> }
> REPORTER_ASSERT(r, image->unique());

Done.

[reed1, 2014-09-29 18:08:26]

https://codereview.chromium.org/610003002/diff/20001/tests/RecordDrawTest.cpp
File tests/RecordDrawTest.cpp (right):

https://codereview.chromium.org/610003002/diff/20001/tests/RecordDrawTest.cpp...
tests/RecordDrawTest.cpp:305: SkAutoTUnref<SkSurface>
surface(SkSurface::NewRasterPMColor(10, 10));
1. Can we using an outer-loop or something to vary how the surface is being
created? (e.g. gpu, imagegenerators)
2. Can we perform these asserts using SkPicture public APIs, or do we really
need to use SkRecord directly?

[Rémi Piotaix, 2014-09-29 18:40:21]

https://codereview.chromium.org/610003002/diff/20001/tests/RecordDrawTest.cpp
File tests/RecordDrawTest.cpp (right):

https://codereview.chromium.org/610003002/diff/20001/tests/RecordDrawTest.cpp...
tests/RecordDrawTest.cpp:305: SkAutoTUnref<SkSurface>
surface(SkSurface::NewRasterPMColor(10, 10));
On 2014/09/29 18:08:26, reed1 wrote:
> 1. Can we using an outer-loop or something to vary how the surface is being
> created? (e.g. gpu, imagegenerators)

Yeah, sure.

> 2. Can we perform these asserts using SkPicture public APIs, or do we really
> need to use SkRecord directly?
If we want a test using SkPicture public API, it is more an integration test
than a unit test. Because i added an opcode in SkRecord, i think it is
meaningful to have this test using directly SkRecord.

If we use the SkPicture API for a test, shouldn't we modify the deprecated path
as well? (the test will break otherwise)

[mtklein, 2014-09-30 14:42:21]

Except for a few nits, this LGTM.  Please let other folks review to their
satisfaction too before landing this.

For what it's worth, even if we were to add higher-level SkPicture or SkCanvas
public API based tests, I do like the existence and scope of unit tests for the
various parts of SkRecord just as they're written here.

https://codereview.chromium.org/610003002/diff/60001/tests/RecordDrawTest.cpp
File tests/RecordDrawTest.cpp (right):

https://codereview.chromium.org/610003002/diff/60001/tests/RecordDrawTest.cpp...
tests/RecordDrawTest.cpp:265: const SkPaint* paint = NULL) SK_OVERRIDE {
some funky indents on these methods

https://codereview.chromium.org/610003002/diff/60001/tests/RecordDrawTest.cpp...
tests/RecordDrawTest.cpp:268: fDrawingImage = image;
fDrawingImage seems unused?

https://codereview.chromium.org/610003002/diff/60001/tests/RecorderTest.cpp
File tests/RecorderTest.cpp (right):

https://codereview.chromium.org/610003002/diff/60001/tests/RecorderTest.cpp#n...
tests/RecorderTest.cpp:176: // The SkImage should be unique
This comment and its friend below say exactly what the next lines of code say. 
You might consider deleting them.

[reed1, 2014-09-30 15:43:07]

lgtm

[Rémi Piotaix, 2014-09-30 15:48:10]

https://codereview.chromium.org/610003002/diff/60001/tests/RecordDrawTest.cpp
File tests/RecordDrawTest.cpp (right):

https://codereview.chromium.org/610003002/diff/60001/tests/RecordDrawTest.cpp...
tests/RecordDrawTest.cpp:265: const SkPaint* paint = NULL) SK_OVERRIDE {
On 2014/09/30 14:42:21, mtklein wrote:
> some funky indents on these methods

Done.

https://codereview.chromium.org/610003002/diff/60001/tests/RecordDrawTest.cpp...
tests/RecordDrawTest.cpp:268: fDrawingImage = image;
On 2014/09/30 14:42:21, mtklein wrote:
> fDrawingImage seems unused?

Done.

https://codereview.chromium.org/610003002/diff/60001/tests/RecorderTest.cpp
File tests/RecorderTest.cpp (right):

https://codereview.chromium.org/610003002/diff/60001/tests/RecorderTest.cpp#n...
tests/RecorderTest.cpp:176: // The SkImage should be unique
On 2014/09/30 14:42:21, mtklein wrote:
> This comment and its friend below say exactly what the next lines of code say.

> You might consider deleting them.

Done.

[robertphillips, 2014-09-30 16:10:49]

lgtm

[Rémi Piotaix, 2014-10-16 18:50:59]

The CQ bit was checked by piotaixr@chromium.org

[commit-bot: I haz the power, 2014-10-16 18:51:26]

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

[commit-bot: I haz the power, 2014-10-16 18:58:48]

Committed patchset #6 (id:100001) as 65151754b9fdb6a968d7307764c20655d1b680a0

[tfarina, 2014-10-24 00:52:15]

https://codereview.chromium.org/610003002/diff/100001/tests/RecordDrawTest.cpp
File tests/RecordDrawTest.cpp (right):

https://codereview.chromium.org/610003002/diff/100001/tests/RecordDrawTest.cp...
tests/RecordDrawTest.cpp:284: typedef SkCanvas INHERITED;
Filed https://code.google.com/p/skia/issues/detail?id=3059 for this.