Implement multi-threaded picture playback via cloning.

Implement multi-threaded picture playback via cloning.

The CL adds SkPicture.clone() which produces a thread-safe copy by
creating a shallow copy of the thread-safe data within the picture and
a deep copy of the data that is not (e.g. SkPaint).  This implementation
re-flattens the paints when cloning instead of retaining the flattened
paints from the recording process.

Changes were also needed to various classes to ensure thread safety

Committed: https://code.google.com/p/skia/source/detail?r=5335

[Leon, 2012-08-20 19:25:57]

http://codereview.appspot.com/6459105/diff/1/src/core/SkPicturePlayback.h
File src/core/SkPicturePlayback.h (right):

http://codereview.appspot.com/6459105/diff/1/src/core/SkPicturePlayback.h#new...
src/core/SkPicturePlayback.h:170: SkRefPtr<SkBitmapHeap> fBitmapHeap;
SkRefPtr is discouraged. I think Ben has been trying to remove them all.

[Leon, 2012-08-28 21:29:46]

Looks good overall, with some nits.

http://codereview.appspot.com/6459105/diff/4001/include/core/SkRefCnt.h
File include/core/SkRefCnt.h (right):

http://codereview.appspot.com/6459105/diff/4001/include/core/SkRefCnt.h#newco...
include/core/SkRefCnt.h:157: // TODO why can't we have this?
What happens when you try it?

http://codereview.appspot.com/6459105/diff/4001/src/core/SkPicturePlayback.cpp
File src/core/SkPicturePlayback.cpp (right):

http://codereview.appspot.com/6459105/diff/4001/src/core/SkPicturePlayback.cp...
src/core/SkPicturePlayback.cpp:152: if (copyInfo == NULL || *copyInfo == NULL) {
NULL == copyInfo ||  NULL == *copyInfo

http://codereview.appspot.com/6459105/diff/4001/src/core/SkPicturePlayback.cp...
src/core/SkPicturePlayback.cpp:155: SkDEBUGCODE(int heapSize = fBitmapHeap.get()
? fBitmapHeap->count() : 0;)
int heapSize = SafeCount(fBitmapHeap.get())

http://codereview.appspot.com/6459105/diff/4001/src/core/SkPicturePlayback.cp...
src/core/SkPicturePlayback.cpp:161: SkASSERT(!fBitmapHeap.get() ||
fBitmapHeap->count() == heapSize);
For the first case, we ought to also assert that 0 == heapSize. Would be even
easier with SafeCount:

SkASSERT(SafeCount(fBitmapHeap.get()) == heapSize)

http://codereview.appspot.com/6459105/diff/4001/src/core/SkPicturePlayback.cp...
src/core/SkPicturePlayback.cpp:163: if (copyInfo != NULL && *copyInfo == NULL) {
If copyInfo is non-NULL, *copyInfo is guaranteed to be NULL, or execution would
not have entered this block.

http://codereview.appspot.com/6459105/diff/4001/src/core/SkPicturePlayback.cp...
src/core/SkPicturePlayback.cpp:173: localInfo->controller.setupPlaybacks(); //
needed to create typeface playback
Can't this go inside the if (copyInfo == NULL || *copyInfo == NULL) block?

[DerekS, 2012-08-29 14:38:32]

https://codereview.appspot.com/6459105/diff/4001/include/core/SkRefCnt.h
File include/core/SkRefCnt.h (right):

https://codereview.appspot.com/6459105/diff/4001/include/core/SkRefCnt.h#newc...
include/core/SkRefCnt.h:157: // TODO why can't we have this?
On 2012/08/28 21:29:46, scroggo-work wrote:
> What happens when you try it?

It is technically not a problem, but I was informed second-hand that Mike
wouldn't like it. So I'm going to clear it with him before I submit it.

https://codereview.appspot.com/6459105/diff/4001/src/core/SkPicturePlayback.cpp
File src/core/SkPicturePlayback.cpp (right):

https://codereview.appspot.com/6459105/diff/4001/src/core/SkPicturePlayback.c...
src/core/SkPicturePlayback.cpp:152: if (copyInfo == NULL || *copyInfo == NULL) {
On 2012/08/28 21:29:46, scroggo-work wrote:
> NULL == copyInfo ||  NULL == *copyInfo

Done.

https://codereview.appspot.com/6459105/diff/4001/src/core/SkPicturePlayback.c...
src/core/SkPicturePlayback.cpp:155: SkDEBUGCODE(int heapSize = fBitmapHeap.get()
? fBitmapHeap->count() : 0;)
On 2012/08/28 21:29:46, scroggo-work wrote:
> int heapSize = SafeCount(fBitmapHeap.get())

Done.

https://codereview.appspot.com/6459105/diff/4001/src/core/SkPicturePlayback.c...
src/core/SkPicturePlayback.cpp:161: SkASSERT(!fBitmapHeap.get() ||
fBitmapHeap->count() == heapSize);
On 2012/08/28 21:29:46, scroggo-work wrote:
> For the first case, we ought to also assert that 0 == heapSize. Would be even
> easier with SafeCount:
> 
> SkASSERT(SafeCount(fBitmapHeap.get()) == heapSize)

Done.

https://codereview.appspot.com/6459105/diff/4001/src/core/SkPicturePlayback.c...
src/core/SkPicturePlayback.cpp:163: if (copyInfo != NULL && *copyInfo == NULL) {
On 2012/08/28 21:29:46, scroggo-work wrote:
> If copyInfo is non-NULL, *copyInfo is guaranteed to be NULL, or execution
would
> not have entered this block.

Done.

https://codereview.appspot.com/6459105/diff/4001/src/core/SkPicturePlayback.c...
src/core/SkPicturePlayback.cpp:173: localInfo->controller.setupPlaybacks(); //
needed to create typeface playback
On 2012/08/28 21:29:46, scroggo-work wrote:
> Can't this go inside the if (copyInfo == NULL || *copyInfo == NULL) block?

Done.

[Leon, 2012-08-29 15:59:10]

https://codereview.appspot.com/6459105/diff/19001/include/core/SkPicture.h
File include/core/SkPicture.h (right):

https://codereview.appspot.com/6459105/diff/19001/include/core/SkPicture.h#ne...
include/core/SkPicture.h:55: SkPicture* clone();
const

https://codereview.appspot.com/6459105/diff/19001/include/core/SkPicture.h#ne...
include/core/SkPicture.h:58: * Creates multiple thread-safe clones of this
picture that is ready for
are*

https://codereview.appspot.com/6459105/diff/19001/include/core/SkPicture.h#ne...
include/core/SkPicture.h:62: void clone(SkPicture* pictures, int count);
const

https://codereview.appspot.com/6459105/diff/19001/src/core/SkPicture.cpp
File src/core/SkPicture.cpp (right):

https://codereview.appspot.com/6459105/diff/19001/src/core/SkPicture.cpp#newc...
src/core/SkPicture.cpp:148: SkPictCopyInfo* copyInfo = NULL;
What if we put an SkPictCopyInfo on the stack? Then it doesn't need to be ref
counted, we won't start off with a pointer to NULL that always gets set to a new
object (at least in the case where fPlayback is non-NULL), and the constructor
of an SkPicturePlayback from another SkPicturePlayback is simpler. We'd have to
add a field to tell that it has been set up (so that we only do the dirty work
once), and we wouldn't be able to initialize the SkTArray with the right
reserve.

https://codereview.appspot.com/6459105/diff/19001/src/core/SkPicturePlayback.cpp
File src/core/SkPicturePlayback.cpp (right):

https://codereview.appspot.com/6459105/diff/19001/src/core/SkPicturePlayback....
src/core/SkPicturePlayback.cpp:151: if (NULL == copyInfo || NULL == *copyInfo) {
Is there a valid reason for passing NULL as copyInfo (besides when deepCopy is
false)? Since the header is private, we are the only caller of this constructor,
and we only call it with a non-NULL (pointer to) copyInfo (or pass false to
deepCopy).

https://codereview.appspot.com/6459105/diff/19001/src/core/SkPicturePlayback....
src/core/SkPicturePlayback.cpp:153: * the deep copy of it's internal structures
as needed. The holdup to doing that is at
its*

https://codereview.appspot.com/6459105/diff/19001/src/core/SkPicturePlayback....
src/core/SkPicturePlayback.cpp:158: localInfo = new
SkPictCopyInfo(src.fPaints->count());
SkNEW_ARGS

https://codereview.appspot.com/6459105/diff/19001/src/core/SkPicturePlayback.h
File src/core/SkPicturePlayback.h (right):

https://codereview.appspot.com/6459105/diff/19001/src/core/SkPicturePlayback....
src/core/SkPicturePlayback.h:46: * Container for data that is needed to deep
copy a SkPicture. The container
This comment makes me wonder if having a parameter for deepCopy and a parameter
for copyInfo is redundant. Can we assume that if a copyInfo is provided then a
deepCopy is wanted? Or is it strange to say that providing a copyInfo means
deepCopy? Maybe it would be clearer if we called this class SkPictDeepCopyInfo?

[DerekS, 2012-08-29 18:16:03]

https://codereview.appspot.com/6459105/diff/19001/include/core/SkPicture.h
File include/core/SkPicture.h (right):

https://codereview.appspot.com/6459105/diff/19001/include/core/SkPicture.h#ne...
include/core/SkPicture.h:55: SkPicture* clone();
On 2012/08/29 15:59:10, scroggo-work wrote:
> const

Done.

https://codereview.appspot.com/6459105/diff/19001/include/core/SkPicture.h#ne...
include/core/SkPicture.h:58: * Creates multiple thread-safe clones of this
picture that is ready for
On 2012/08/29 15:59:10, scroggo-work wrote:
> are*

Done.

https://codereview.appspot.com/6459105/diff/19001/include/core/SkPicture.h#ne...
include/core/SkPicture.h:62: void clone(SkPicture* pictures, int count);
On 2012/08/29 15:59:10, scroggo-work wrote:
> const

Done.

https://codereview.appspot.com/6459105/diff/19001/src/core/SkPicture.cpp
File src/core/SkPicture.cpp (right):

https://codereview.appspot.com/6459105/diff/19001/src/core/SkPicture.cpp#newc...
src/core/SkPicture.cpp:148: SkPictCopyInfo* copyInfo = NULL;
On 2012/08/29 15:59:10, scroggo-work wrote:
> What if we put an SkPictCopyInfo on the stack? Then it doesn't need to be ref
> counted, we won't start off with a pointer to NULL that always gets set to a
new
> object (at least in the case where fPlayback is non-NULL), and the constructor
> of an SkPicturePlayback from another SkPicturePlayback is simpler. We'd have
to
> add a field to tell that it has been set up (so that we only do the dirty work
> once), and we wouldn't be able to initialize the SkTArray with the right
> reserve.

Done.

https://codereview.appspot.com/6459105/diff/19001/src/core/SkPicturePlayback.cpp
File src/core/SkPicturePlayback.cpp (right):

https://codereview.appspot.com/6459105/diff/19001/src/core/SkPicturePlayback....
src/core/SkPicturePlayback.cpp:153: * the deep copy of it's internal structures
as needed. The holdup to doing that is at
On 2012/08/29 15:59:10, scroggo-work wrote:
> its*

Done.

https://codereview.appspot.com/6459105/diff/19001/src/core/SkPicturePlayback....
src/core/SkPicturePlayback.cpp:158: localInfo = new
SkPictCopyInfo(src.fPaints->count());
On 2012/08/29 15:59:10, scroggo-work wrote:
> SkNEW_ARGS

Done.

[Leon, 2012-08-29 18:32:14]

lgtm

https://codereview.appspot.com/6459105/diff/11005/src/core/SkPicturePlayback.h
File src/core/SkPicturePlayback.h (right):

https://codereview.appspot.com/6459105/diff/11005/src/core/SkPicturePlayback....
src/core/SkPicturePlayback.h:49: class SkPictCopyInfo {
Minor nit: could be a struct

[TomH, 2014-02-14 12:32:12]

https://codereview.appspot.com/6459105/diff/8016/src/core/SkPicture.cpp
File src/core/SkPicture.cpp (right):

https://codereview.appspot.com/6459105/diff/8016/src/core/SkPicture.cpp#newco...
src/core/SkPicture.cpp:159: it (since it is destructive, and we don't want to
change src).
Was there a use case or explicit design behind this choice? We *could* either
force endRecording() or return an error if we're trying to clone() something
that's still recording; that'd let us make some performance improvements.

[Leon, 2014-02-14 14:45:21]

https://codereview.appspot.com/6459105/diff/8016/src/core/SkPicture.cpp
File src/core/SkPicture.cpp (right):

https://codereview.appspot.com/6459105/diff/8016/src/core/SkPicture.cpp#newco...
src/core/SkPicture.cpp:159: it (since it is destructive, and we don't want to
change src).
On 2014/02/14 12:32:12, TomH wrote:
> Was there a use case or explicit design behind this choice? We *could* either
> force endRecording() or return an error if we're trying to clone() something
> that's still recording; that'd let us make some performance improvements.

I don't recall whether there was a use case, but it mimics the copy constructor.
I think the idea was that it would just work, and you wouldn't get any surprise
behavior (you could no longer record to your original picture, for example).

We should check to see if anyone takes advantage of the current implementation.
Fortunately, Android does not expose clone, but they do expose the copy
constructor (through Java), and I can see an argument for keeping the two
consistent.

That said, if we can get a performance improvement by removing a feature that is
not used (or switching the clients to use it differently), we should take it.
How do you imagine this will yield a performance improvement? Would it be enough
to tell the client to call endRecording() first to get the speedup?

[TomH, 2014-02-14 14:56:30]

Oops, forgot to +Ian last time.

IIUC, when we transition from SkPictureRecord -> SkPicturePlayback, some of the
data structures get translated, but some just get copied across, and those
copies are expensive enough to show up in the profiles. We'd like to just
transfer ownership of the data structures where possible (to avoid the copies),
but the pieces of the API that copy-without-doing-an-endRecording() make that a
lot more complicated.

On reflection I'm not sure they *prevent* Ian's intended optimization, just make
it uglier, but he's got a finely developed aesthetic sense so we figured we'd
put the API to the question.

[mtklein, 2014-02-14 14:58:32]

On 2014/02/14 14:56:30, TomH wrote:
> Oops, forgot to +Ian last time.
> 
> IIUC, when we transition from SkPictureRecord -> SkPicturePlayback, some of
the
> data structures get translated, but some just get copied across, and those
> copies are expensive enough to show up in the profiles. We'd like to just
> transfer ownership of the data structures where possible (to avoid the
copies),
> but the pieces of the API that copy-without-doing-an-endRecording() make that
a
> lot more complicated.
> 
> On reflection I'm not sure they *prevent* Ian's intended optimization, just
make
> it uglier, but he's got a finely developed aesthetic sense so we figured we'd
> put the API to the question.

Specifically, we'd like to move the ops stream instead of copying it.