Replace SkPictureReplacementPlayback with GrRecordReplaceDraw

Replace SkPictureReplacementPlayback with GrRecordReplaceDraw

I think this is sufficiently specialized to keep it in Ganesh for the time being.

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

[robertphillips, 2014-09-03 15:07:57]

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

[bsalomon, 2014-09-03 15:43:50]

https://codereview.chromium.org/535953002/diff/40001/src/gpu/GrRecordReplaceD...
File src/gpu/GrRecordReplaceDraw.cpp (right):

https://codereview.chromium.org/535953002/diff/40001/src/gpu/GrRecordReplaceD...
src/gpu/GrRecordReplaceDraw.cpp:98: for (; (uintptr_t)ops[i] < ri->fStop; ++i)
while ((uintptr_t)ops[i] < ri->fStop) {
 ++i;
}

while ((uintptr_t)ops[i++] < ri->fStop) {
}

?

https://codereview.chromium.org/535953002/diff/40001/src/gpu/GrRecordReplaceD...
File src/gpu/GrRecordReplaceDraw.h (right):

https://codereview.chromium.org/535953002/diff/40001/src/gpu/GrRecordReplaceD...
src/gpu/GrRecordReplaceDraw.h:31: SkBitmap*       fBM;     // fBM is allocated
so ReplacementInfo can remain POD
Does it have to be an SkBitmap? Could it be an SkImage?

[mtklein, 2014-09-03 15:46:12]

https://codereview.chromium.org/535953002/diff/40001/src/gpu/GrRecordReplaceD...
File src/gpu/GrRecordReplaceDraw.cpp (right):

https://codereview.chromium.org/535953002/diff/40001/src/gpu/GrRecordReplaceD...
src/gpu/GrRecordReplaceDraw.cpp:59: SkRecords::DrawBitmapRectToRect
dbmr2r(const_cast<SkPaint*>(ri->fPaint),
Hrm.  This isn't going to work.  dbmr2r will destroy (but not free the memory)
of rf->fPaint when it goes out of scope.  Shame on your const_cast!

I think if we're going to do things this way, it'd have to look like this:

SkAutoTMalloc<SkPaint> buffer(1);
SkNEW_PLACEMENT_ARGS(buffer.get(), SkPaint, (*rf->fPaint));
SkRecords::DBRTR dmbr2r(buffer.get(), *ri->fBM, ...);
(*draw)(dbmr2r);


This all raises the question of whether it'd be better to modify the SkRecord
before playing it back rather than playing it back differently.  Can't we just
no-op away the old commands and insert a new draw bitmap?

https://codereview.chromium.org/535953002/diff/40001/src/gpu/GrRecordReplaceD...
File src/gpu/GrRecordReplaceDraw.h (right):

https://codereview.chromium.org/535953002/diff/40001/src/gpu/GrRecordReplaceD...
src/gpu/GrRecordReplaceDraw.h:31: SkBitmap*       fBM;     // fBM is allocated
so ReplacementInfo can remain POD
Is this an alternative to SkTArray<ReplacementInfo>, or something more
complicated?

https://codereview.chromium.org/535953002/diff/40001/src/gpu/GrRecordReplaceD...
src/gpu/GrRecordReplaceDraw.h:45: const ReplacementInfo* lookupByStart(size_t
start, int* lastLookedUp) const;
Not clear here that lastLookedUp is both input and output.

https://codereview.chromium.org/535953002/diff/40001/src/gpu/SkGpuDevice.cpp
File src/gpu/SkGpuDevice.cpp (right):

https://codereview.chromium.org/535953002/diff/40001/src/gpu/SkGpuDevice.cpp#...
src/gpu/SkGpuDevice.cpp:1867: SkRect clipBounds = { 0, 0, 0, 0 };
Seems fine, but if we're returning when getClipBounds() fails, we shouldn't need
to initialize clipBounds.  This just for sanity?

[mtklein, 2014-09-03 15:49:14]

https://codereview.chromium.org/535953002/diff/40001/src/gpu/GrRecordReplaceD...
File src/gpu/GrRecordReplaceDraw.cpp (right):

https://codereview.chromium.org/535953002/diff/40001/src/gpu/GrRecordReplaceD...
src/gpu/GrRecordReplaceDraw.cpp:64: (*draw)(dbmr2r);
Wait a sec, maybe that's still even too complicated.  Why don't we pass the
canvas in here instead of the SkRecords::Draw?  Then we can just replace all
this with:

  canvas->drawBitmapRectToRect(*ri->fBM, &src, dst, ri->fPaint);

[robertphillips, 2014-09-03 17:36:24]

https://codereview.chromium.org/535953002/diff/40001/src/gpu/GrRecordReplaceD...
File src/gpu/GrRecordReplaceDraw.cpp (right):

https://codereview.chromium.org/535953002/diff/40001/src/gpu/GrRecordReplaceD...
src/gpu/GrRecordReplaceDraw.cpp:64: (*draw)(dbmr2r);
On 2014/09/03 15:49:14, mtklein wrote:
> Wait a sec, maybe that's still even too complicated.  Why don't we pass the
> canvas in here instead of the SkRecords::Draw?  Then we can just replace all
> this with:
> 
>   canvas->drawBitmapRectToRect(*ri->fBM, &src, dst, ri->fPaint);

Done.

https://codereview.chromium.org/535953002/diff/40001/src/gpu/GrRecordReplaceD...
src/gpu/GrRecordReplaceDraw.cpp:98: for (; (uintptr_t)ops[i] < ri->fStop; ++i)
On 2014/09/03 15:43:50, bsalomon wrote:
> while ((uintptr_t)ops[i] < ri->fStop) {
>  ++i;
> }
> 
> while ((uintptr_t)ops[i++] < ri->fStop) {
> }
> 
> ?

Done.

https://codereview.chromium.org/535953002/diff/40001/src/gpu/GrRecordReplaceD...
File src/gpu/GrRecordReplaceDraw.h (right):

https://codereview.chromium.org/535953002/diff/40001/src/gpu/GrRecordReplaceD...
src/gpu/GrRecordReplaceDraw.h:31: SkBitmap*       fBM;     // fBM is allocated
so ReplacementInfo can remain POD
On 2014/09/03 15:43:50, bsalomon wrote:
> Does it have to be an SkBitmap? Could it be an SkImage?

Done.

https://codereview.chromium.org/535953002/diff/40001/src/gpu/GrRecordReplaceD...
src/gpu/GrRecordReplaceDraw.h:31: SkBitmap*       fBM;     // fBM is allocated
so ReplacementInfo can remain POD
On 2014/09/03 15:46:12, mtklein wrote:
> Is this an alternative to SkTArray<ReplacementInfo>, or something more
> complicated?

The former.

https://codereview.chromium.org/535953002/diff/40001/src/gpu/GrRecordReplaceD...
src/gpu/GrRecordReplaceDraw.h:45: const ReplacementInfo* lookupByStart(size_t
start, int* lastLookedUp) const;
I've added a comment. Other then that I'm open to suggestions.

https://codereview.chromium.org/535953002/diff/40001/src/gpu/SkGpuDevice.cpp
File src/gpu/SkGpuDevice.cpp (right):

https://codereview.chromium.org/535953002/diff/40001/src/gpu/SkGpuDevice.cpp#...
src/gpu/SkGpuDevice.cpp:1867: SkRect clipBounds = { 0, 0, 0, 0 };
On 2014/09/03 15:46:12, mtklein wrote:
> Seems fine, but if we're returning when getClipBounds() fails, we shouldn't
need
> to initialize clipBounds.  This just for sanity?

Done.

[bsalomon, 2014-09-03 17:45:53]

lgtm

[mtklein, 2014-09-03 17:47:56]

lgtm

https://codereview.chromium.org/535953002/diff/60001/src/gpu/GrRecordReplaceD...
File src/gpu/GrRecordReplaceDraw.cpp (right):

https://codereview.chromium.org/535953002/diff/60001/src/gpu/GrRecordReplaceD...
src/gpu/GrRecordReplaceDraw.cpp:97: SkASSERT((uintptr_t)ops[i] == ri->fStop);
Replace the loop and assert with "i = ri->fStop;" as below?

[robertphillips, 2014-09-03 17:56:52]

https://codereview.chromium.org/535953002/diff/60001/src/gpu/GrRecordReplaceD...
File src/gpu/GrRecordReplaceDraw.cpp (right):

https://codereview.chromium.org/535953002/diff/60001/src/gpu/GrRecordReplaceD...
src/gpu/GrRecordReplaceDraw.cpp:97: SkASSERT((uintptr_t)ops[i] == ri->fStop);
ri->fStop is an index into the list of all the operations. 'i' is an index into
the operations returned by the BBH. So we have to skip 'i' forward until the op
index it identifies matches fi->fStop.

[robertphillips, 2014-09-03 17:56:58]

The CQ bit was checked by robertphillips@google.com

[commit-bot: I haz the power, 2014-09-03 17:57:09]

CQ is trying da patch. Follow status at

https://skia-tree-status.appspot.com/cq/robertphillips@google.com/535953002/8...

[mtklein, 2014-09-03 17:59:26]

On 2014/09/03 17:56:52, robertphillips wrote:
>
https://codereview.chromium.org/535953002/diff/60001/src/gpu/GrRecordReplaceD...
> File src/gpu/GrRecordReplaceDraw.cpp (right):
> 
>
https://codereview.chromium.org/535953002/diff/60001/src/gpu/GrRecordReplaceD...
> src/gpu/GrRecordReplaceDraw.cpp:97: SkASSERT((uintptr_t)ops[i] == ri->fStop);
> ri->fStop is an index into the list of all the operations. 'i' is an index
into
> the operations returned by the BBH. So we have to skip 'i' forward until the
op
> index it identifies matches fi->fStop.

Oh, duh.

[commit-bot: I haz the power, 2014-09-03 18:04:38]

Committed patchset #5 (id:80001) as d982eb22d7bce655a609e9b481b3ddfeff361067