Proposed SkCanvas API for preLoading textures to VRAM v2.0

Proposed SkCanvas API for preLoading textures to VRAM v2.0

This is an update to (Proposed SkCanvas API for preLoading textures to VRAM - https://codereview.chromium.org/192853002/). It takes into account in-person feedback on the initial proposal. The main feedback was to land this closer to where we will ultimately wind up with the reordered rendering capability (and don't have an SkCanvas entry point (yet)).

Committed: http://code.google.com/p/skia/source/detail?r=13810

Committed: http://code.google.com/p/skia/source/detail?r=13822

[bsalomon, 2014-03-12 15:34:34]

This seems like a big step in the right direction from the previous approach. It
looks like it'll extend well to other preprocessing. I'm wondering if the
optimize() step should be explicitly called by the client or whether we should
lazily do it when then accel data is not present the first time the picture is
rendered.

https://codereview.chromium.org/197123003/diff/1/include/core/SkPicture.h
File include/core/SkPicture.h (right):

https://codereview.chromium.org/197123003/diff/1/include/core/SkPicture.h#new...
include/core/SkPicture.h:62: void setAccelData(const SkAccelData* data) {
SkRefCnt_SafeAssign(fAccelData, data); }
If we ever have other backends that want to optimize then we may want to store
the accel in some hash (so that there can be multiple) and have the get() take
an id and produce a matching accel if one has been added with that id..

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

https://codereview.chromium.org/197123003/diff/1/src/gpu/SkGpuDevice.cpp#newc...
src/gpu/SkGpuDevice.cpp:2006: static const int32_t kGPUID = 0x01;
Maybe can grab this from an atomic inc at runtime (a la GrEffect class IDs).

[robertphillips, 2014-03-13 12:43:42]

I think we have to wait and see if it makes sense to always precompute the
acceleration data. I can imagine a situation in which Chromium will want
explicit control over what gets preprocessed and when.

https://codereview.chromium.org/197123003/diff/1/include/core/SkPicture.h
File include/core/SkPicture.h (right):

https://codereview.chromium.org/197123003/diff/1/include/core/SkPicture.h#new...
include/core/SkPicture.h:62: void setAccelData(const SkAccelData* data) {
SkRefCnt_SafeAssign(fAccelData, data); }
On 2014/03/12 15:34:34, bsalomon wrote:
> If we ever have other backends that want to optimize then we may want to store
> the accel in some hash (so that there can be multiple) and have the get() take
> an id and produce a matching accel if one has been added with that id..

Done. I have updated the interface to support this but would like to delay
(until Mike K is around) before tangling with SkTDynamicHash again
(https://codereview.chromium.org/197443003/).

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

https://codereview.chromium.org/197123003/diff/1/src/gpu/SkGpuDevice.cpp#newc...
src/gpu/SkGpuDevice.cpp:2006: static const int32_t kGPUID = 0x01;
On 2014/03/12 15:34:34, bsalomon wrote:
> Maybe can grab this from an atomic inc at runtime (a la GrEffect class IDs).

Done.

[bsalomon, 2014-03-13 15:06:37]

This seems like a good approach to me but I'd like to here from some other
peanuts.

On 2014/03/13 12:43:42, robertphillips wrote:
> I think we have to wait and see if it makes sense to always precompute the
> acceleration data. I can imagine a situation in which Chromium will want
> explicit control over what gets preprocessed and when.

I'm leaning more in this direction now myself, too.

[robertphillips, 2014-03-13 16:25:53]

Mike - could you chime in?

[reed1, 2014-03-13 16:28:10]

https://codereview.chromium.org/197123003/diff/100001/include/core/SkDevice.h
File include/core/SkDevice.h (right):

https://codereview.chromium.org/197123003/diff/100001/include/core/SkDevice.h...
include/core/SkDevice.h:193: virtual bool
EXPERIMENTAL_optimizedRender(SkPicture& picture);
why & instead of * ?

https://codereview.chromium.org/197123003/diff/100001/include/core/SkPicture.h
File include/core/SkPicture.h (right):

https://codereview.chromium.org/197123003/diff/100001/include/core/SkPicture....
include/core/SkPicture.h:40: class SkAccelData : public SkRefCnt {
nit: inner classes don't need Sk prefix.

[reed1, 2014-03-13 16:28:38]

PS - thank you for making the names scary and ugly!

[robertphillips, 2014-03-13 16:55:38]

https://codereview.chromium.org/197123003/diff/100001/include/core/SkDevice.h
File include/core/SkDevice.h (right):

https://codereview.chromium.org/197123003/diff/100001/include/core/SkDevice.h...
include/core/SkDevice.h:193: virtual bool
EXPERIMENTAL_optimizedRender(SkPicture& picture);
On 2014/03/13 16:28:10, reed1 wrote:
> why & instead of * ?

Done. I was mirroring SkCanvas::drawPicture.

https://codereview.chromium.org/197123003/diff/100001/include/core/SkPicture.h
File include/core/SkPicture.h (right):

https://codereview.chromium.org/197123003/diff/100001/include/core/SkPicture....
include/core/SkPicture.h:40: class SkAccelData : public SkRefCnt {
On 2014/03/13 16:28:10, reed1 wrote:
> nit: inner classes don't need Sk prefix.

Done.

[reed1, 2014-03-13 18:33:54]

https://codereview.chromium.org/197123003/diff/120001/include/core/SkDevice.h
File include/core/SkDevice.h (right):

https://codereview.chromium.org/197123003/diff/120001/include/core/SkDevice.h...
include/core/SkDevice.h:181: /**
Not sure I follow why we have two of these.
- does optimize() always create something, even if I call it twice?
- does optimizedRender() return true if optimize() was called? If so, is that
valuable to the client?

[robertphillips, 2014-03-13 19:58:28]

https://codereview.chromium.org/197123003/diff/120001/include/core/SkDevice.h
File include/core/SkDevice.h (right):

https://codereview.chromium.org/197123003/diff/120001/include/core/SkDevice.h...
include/core/SkDevice.h:181: /**
There are two possible landing points for this. In one, the client calls
optimize once (and optimization data is collected for the whole picture)  and
then calls optimizedRender several times (rendering portions of the picture). In
the other, the client calls optimize for each sub-portion and then only calls
optimizedRender when they want pixels for that sub-portion. In the former,
multiple calls to optimize() would return the same information. In the latter,
the different calls would return different data. 

optimizedRender returns true if there is nothing for SkCanvas::drawPicture to
do. Initially, when we are just preloading textures, we will still want
SkCanvas::drawPicture to loop over the operations and draw them. Later, when we
have enough information to just render the entire scene, we will want SkCanvas
to just return. 

I believe Chromium will, at least initially, want to separately control
information collection and rendering. If we land in the second world (where
optimizations are only determined for a sub-region), I suspect Chromium will
want even more control over when analysis is performed (and when they can purge
the collected information).

[robertphillips, 2014-03-14 13:37:36]

PTAL this patch:

renames SkBaseDevice::EXPERIMENTAL_optimizedRender to EXPERIMENTAL_drawPicture
makes SkBaseDevice EXPERIMENTAL_optimize and EXPERIMENTAL_drawPicture protected
adds an SkCanvas::EXPERIMENTAL_optimize entry point
replaces use of getDevice with getTopDevice in SkCanvas::drawPicture
adds more documentation about relationship between SkCanvas & SkDevice
drawPicture

[bsalomon, 2014-03-14 13:59:43]

lgtm

https://codereview.chromium.org/197123003/diff/140001/src/core/SkPicture.cpp
File src/core/SkPicture.cpp (right):

https://codereview.chromium.org/197123003/diff/140001/src/core/SkPicture.cpp#...
src/core/SkPicture.cpp:200: if (id >= 1 << (8 * sizeof(Domain))) {
Ben will be so unhappy with you for making these bold assumptions.

[reed1, 2014-03-14 14:13:33]

api lgtm

[robertphillips, 2014-03-14 16:59:55]

The CQ bit was checked by robertphillips@google.com

[commit-bot: I haz the power, 2014-03-14 17:00:00]

CQ is trying da patch. Follow status at
https://skia-tree-status.appspot.com/cq/robertphillips@google.com/197123003/1...

[commit-bot: I haz the power, 2014-03-14 17:00:06]

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

[commit-bot: I haz the power, 2014-03-14 17:00:07]

Presubmit check for 197123003-140001 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit Warnings **
These files should end in a newline character:
  src/core/SkDevice.cpp

[robertphillips, 2014-03-14 17:01:38]

The CQ bit was checked by robertphillips@google.com

[commit-bot: I haz the power, 2014-03-14 17:01:49]

CQ is trying da patch. Follow status at
https://skia-tree-status.appspot.com/cq/robertphillips@google.com/197123003/1...

[commit-bot: I haz the power, 2014-03-14 17:07:11]

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

[commit-bot: I haz the power, 2014-03-14 17:07:11]

Retried try job too often on Build-Mac10.8-Clang-x86-Release-Trybot for step(s)
BuildSkiaLib
http://108.170.219.164:10117/buildstatus?builder=Build-Mac10.8-Clang-x86-Rele...

[robertphillips, 2014-03-14 17:10:56]

The CQ bit was checked by robertphillips@google.com

[commit-bot: I haz the power, 2014-03-14 17:11:00]

CQ is trying da patch. Follow status at
https://skia-tree-status.appspot.com/cq/robertphillips@google.com/197123003/1...

[commit-bot: I haz the power, 2014-03-14 17:16:17]

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

[commit-bot: I haz the power, 2014-03-14 17:16:17]

Retried try job too often on Build-Mac10.8-Clang-x86-Release-Trybot for step(s)
BuildSkiaLib
http://108.170.219.164:10117/buildstatus?builder=Build-Mac10.8-Clang-x86-Rele...

[robertphillips, 2014-03-14 17:21:43]

The CQ bit was checked by robertphillips@google.com

[commit-bot: I haz the power, 2014-03-14 17:21:52]

CQ is trying da patch. Follow status at
https://skia-tree-status.appspot.com/cq/robertphillips@google.com/197123003/1...

[commit-bot: I haz the power, 2014-03-14 18:23:17]

Change committed as 13810

[robertphillips, 2014-03-16 19:30:34]

The CQ bit was checked by robertphillips@google.com

[commit-bot: I haz the power, 2014-03-16 19:30:36]

CQ is trying da patch. Follow status at
https://skia-tree-status.appspot.com/cq/robertphillips@google.com/197123003/2...

[commit-bot: I haz the power, 2014-03-16 19:46:41]

Change committed as 13822