Add a means of extracting active operations from SkPicture

Add a means of extracting active operations from SkPicture

For the "pull forward" task I will be comparing the two cases:   
  analyze the whole skp and use the BBH information
  analyze only the active portion of the skp

In the first case we need a way to get the BBH information out of the picture in order to extract the relevant portions of the whole-skp analysis. This adds caching of the active ops so that work isn't duplicated between when the optimization path queries for that information and when the usual draw path queries for it.

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

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

[robertphillips, 2014-03-13 12:45:19]

ping

[bsalomon, 2014-03-13 17:59:33]

It might help me a bit to know how this will be called.

https://codereview.chromium.org/195793010/diff/70001/src/core/SkPicturePlayba...
File src/core/SkPicturePlayback.cpp (right):

https://codereview.chromium.org/195793010/diff/70001/src/core/SkPicturePlayba...
src/core/SkPicturePlayback.cpp:830: SkTQSort<SkPictureStateTree::Draw>(
Is this sorting by offset?

https://codereview.chromium.org/195793010/diff/70001/src/core/SkPicturePlayba...
File src/core/SkPicturePlayback.h (right):

https://codereview.chromium.org/195793010/diff/70001/src/core/SkPicturePlayba...
src/core/SkPicturePlayback.h:88: const SkTDArray<void*>* getActiveOps(const
SkIRect& queryRect);
The signature seems a little dangerous to me.

[robertphillips, 2014-03-14 17:59:34]

Making up entry points, this will ultimately used something like:


bool SkGpuDevice::drawPicture(cosnt SkPicture& picture) {
    SkPicture::AccelData::Key key = ComputeAccelDataKey();

    const SkPicture::AccelData* data = picture.EXPERIMENTAL_getAccelData(key);
    if (NULL == data) {
        return false;
    }

    const GPUAccelData *gpuData = static_cast<const GPUAccelData*>(data);

    const SkPicture::OperationList& opList = picture.getActiveOps(??);
    if (!opList.validList()) {
        // no optimization data present - preload _all_ the bitmaps
        for (int i = 0; i < gpuData->totNumBitmaps(); ++i) {
            this->upload(gpuData->getBitmap(i));
        }
        return false;
    }

    gpuData->clearBitmapInfo();
    for (int i = 0; i < opList.numOps(); ++i) {
        gpuData->addOp(opList.offset(i));
    }

    for (int i = 0; i < gpuData->numBitmapsToUpload(); ++i) {
        this->upload(gpuData->getBitmap(i));
    }

    return false;
}

https://codereview.chromium.org/195793010/diff/70001/src/core/SkPicturePlayba...
File src/core/SkPicturePlayback.cpp (right):

https://codereview.chromium.org/195793010/diff/70001/src/core/SkPicturePlayba...
src/core/SkPicturePlayback.cpp:830: SkTQSort<SkPictureStateTree::Draw>(
Yep - that is pre-existing behavior.

https://codereview.chromium.org/195793010/diff/70001/src/core/SkPicturePlayba...
File src/core/SkPicturePlayback.h (right):

https://codereview.chromium.org/195793010/diff/70001/src/core/SkPicturePlayba...
src/core/SkPicturePlayback.h:88: const SkTDArray<void*>* getActiveOps(const
SkIRect& queryRect);
On 2014/03/13 17:59:34, bsalomon wrote:
> The signature seems a little dangerous to me.

Done. I think I have addressed your concerns here. PTAL.

[reed1, 2014-03-14 18:28:32]

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

https://codereview.chromium.org/195793010/diff/110002/include/core/SkPicture....
include/core/SkPicture.h:153: class OperationList : public SkNoncopyable {
does this class need to be public? Can it instead be private but declare our
impl as a friend?

https://codereview.chromium.org/195793010/diff/110002/src/core/SkPicturePlayb...
File src/core/SkPicturePlayback.h (right):

https://codereview.chromium.org/195793010/diff/110002/src/core/SkPicturePlayb...
src/core/SkPicturePlayback.h:114: void preLoadBitmaps(const SkTDArray<void*>*
results);
maybe unavoidable, but this method is not const, and our hope for the future is
that "playback" objects can be immutable...

[robertphillips, 2014-03-14 18:36:07]

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

https://codereview.chromium.org/195793010/diff/110002/include/core/SkPicture....
include/core/SkPicture.h:153: class OperationList : public SkNoncopyable {
SkPicturePlayback provides the impl and is already a friend. SkGpuDevice needs
to access the information and is pretty far away.

https://codereview.chromium.org/195793010/diff/110002/src/core/SkPicturePlayb...
File src/core/SkPicturePlayback.h (right):

https://codereview.chromium.org/195793010/diff/110002/src/core/SkPicturePlayb...
src/core/SkPicturePlayback.h:114: void preLoadBitmaps(const SkTDArray<void*>*
results);
This entry point will be going away. Everything is currently "does" will be
subsumed by SkGpuDevice::drawPicture.

[robertphillips, 2014-03-14 19:23:23]

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

https://codereview.chromium.org/195793010/diff/110002/include/core/SkPicture....
include/core/SkPicture.h:153: class OperationList : public SkNoncopyable {
On 2014/03/14 18:28:32, reed1 wrote:
> does this class need to be public? Can it instead be private but declare our
> impl as a friend?

Done.

[robertphillips, 2014-03-17 14:16:51]

ping

[reed1, 2014-03-17 14:19:21]

lgtm

[bsalomon, 2014-03-17 14:29:36]

On 2014/03/17 14:19:21, reed1 wrote:
> lgtm

lgtm

[robertphillips, 2014-03-17 14:34:25]

The CQ bit was checked by robertphillips@google.com

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

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

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

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

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

Failed to apply patch for src/core/SkPicturePlayback.cpp:
While running patch -p0 --forward --force --no-backup-if-mismatch;
  patching file src/core/SkPicturePlayback.cpp
  Hunk #1 FAILED at 69.
  Hunk #2 succeeded at 261 (offset -6 lines).
  Hunk #3 succeeded at 272 (offset -6 lines).
  Hunk #4 succeeded at 757 (offset -6 lines).
  Hunk #5 succeeded at 774 (offset -6 lines).
  Hunk #6 succeeded at 803 (offset -6 lines).
  Hunk #7 succeeded at 858 (offset -6 lines).
  1 out of 7 hunks FAILED -- saving rejects to file
src/core/SkPicturePlayback.cpp.rej

Patch:       src/core/SkPicturePlayback.cpp
Index: src/core/SkPicturePlayback.cpp
===================================================================
--- src/core/SkPicturePlayback.cpp	(revision 13766)
+++ src/core/SkPicturePlayback.cpp	(working copy)
@@ -69,7 +69,7 @@
 
     record.validate(record.writeStream().bytesWritten(), 0);
     const SkWriter32& writer = record.writeStream();
-    init();
+    this->init();
     if (writer.bytesWritten() == 0) {
         fOpData = SkData::NewEmpty();
         return;
@@ -267,6 +267,7 @@
     fFactoryPlayback = NULL;
     fBoundingHierarchy = NULL;
     fStateTree = NULL;
+    fCachedActiveOps = NULL;
 }
 
 SkPicturePlayback::~SkPicturePlayback() {
@@ -277,6 +278,8 @@
     SkSafeUnref(fBoundingHierarchy);
     SkSafeUnref(fStateTree);
 
+    SkDELETE(fCachedActiveOps);
+
     for (int i = 0; i < fPictureCount; i++) {
         fPictureRefs[i]->unref();
     }
@@ -760,11 +763,16 @@
 // The activeOps parameter is actually "const
SkTDArray<SkPictureStateTree::Draw*>&".
 // It represents the operations about to be drawn, as generated by some spatial
 // subdivision helper class. It should already be in 'fOffset' sorted order.
-void SkPicturePlayback::preLoadBitmaps(const SkTDArray<void*>& activeOps) {
-    if (0 == activeOps.count() || NULL == fBitmapUseOffsets) {
+void SkPicturePlayback::preLoadBitmaps(const SkTDArray<void*>* activeOps) {
+    if ((NULL != activeOps && 0 == activeOps->count()) || NULL ==
fBitmapUseOffsets) {
         return;
     }
 
+    if (NULL == activeOps) {
+        // going to need everything
+        return;
+    }
+
     SkTDArray<int> active;
 
     SkAutoTDeleteArray<bool> needToCheck(new
bool[fBitmapUseOffsets->numIDs()]);
@@ -772,10 +780,10 @@
         needToCheck.get()[i] = true;
     }
 
-    uint32_t max =
((SkPictureStateTree::Draw*)activeOps[activeOps.count()-1])->fOffset;
+    uint32_t max =
((SkPictureStateTree::Draw*)(*activeOps)[(*activeOps).count()-1])->fOffset;
 
-    for (int i = 0; i < activeOps.count(); ++i) {
-        SkPictureStateTree::Draw* draw = (SkPictureStateTree::Draw*)
activeOps[i];
+    for (int i = 0; i < activeOps->count(); ++i) {
+        SkPictureStateTree::Draw* draw = (SkPictureStateTree::Draw*)
(*activeOps)[i];
 
         for (int j = 0; j < fBitmapUseOffsets->numIDs(); ++j) {
             if (!needToCheck.get()[j]) {
@@ -801,6 +809,41 @@
     }
 }
 
+uint32_t SkPicturePlayback::CachedOperationList::offset(int index) const {
+    SkASSERT(index < fOps.count());
+    return ((SkPictureStateTree::Draw*)fOps[index])->fOffset;
+}
+
+const SkMatrix& SkPicturePlayback::CachedOperationList::matrix(int index) const
{
+    SkASSERT(index < fOps.count());
+    return *((SkPictureStateTree::Draw*)fOps[index])->fMatrix;
+}
+
+const SkPicture::OperationList& SkPicturePlayback::getActiveOps(const SkIRect&
query) {
+    if (NULL == fStateTree || NULL == fBoundingHierarchy) {
+        return SkPicture::OperationList::InvalidList();
+    }
+
+    if (NULL == fCachedActiveOps) {
+        fCachedActiveOps = SkNEW(CachedOperationList);
+    }
+
+    if (query == fCachedActiveOps->fCacheQueryRect) {
+        return *fCachedActiveOps;
+    }
+
+    fCachedActiveOps->fOps.rewind();
+
+    fBoundingHierarchy->search(query, &(fCachedActiveOps->fOps));
+
+    SkTQSort<SkPictureStateTree::Draw>(
+       
reinterpret_cast<SkPictureStateTree::Draw**>(fCachedActiveOps->fOps.begin()),
+       
reinterpret_cast<SkPictureStateTree::Draw**>(fCachedActiveOps->fOps.end()-1));
+
+    fCachedActiveOps->fCacheQueryRect = query;
+    return *fCachedActiveOps;
+}
+
 void SkPicturePlayback::draw(SkCanvas& canvas, SkDrawPictureCallback* callback)
{
 #ifdef ENABLE_TIME_DRAW
     SkAutoTime  at("SkPicture::draw", 50);
@@ -821,26 +864,29 @@
 
     SkReader32 reader(fOpData->bytes(), fOpData->size());
     TextContainer text;
-    SkTDArray<void*> activeOps;
+    const SkTDArray<void*>* activeOps = NULL;
 
     if (NULL != fStateTree && NULL != fBoundingHierarchy) {
         SkRect clipBounds;
         if (canvas.getClipBounds(&clipBounds)) {
             SkIRect query;
             clipBounds.roundOut(&query);
-            fBoundingHierarchy->search(query, &activeOps);
-            if (activeOps.count() == 0) {
-                return;
+
+            const SkPicture::OperationList& activeOpsList =
this->getActiveOps(query);
+            if (activeOpsList.valid()) {
+                if (0 == activeOpsList.numOps()) {
+                    return;     // nothing to draw
+                }
+                
+                // Since the opList is valid we know it is our derived class
+                activeOps = &((const CachedOperationList&)activeOpsList).fOps;
             }
-            SkTQSort<SkPictureStateTree::Draw>(
-               
reinterpret_cast<SkPictureStateTree::Draw**>(activeOps.begin()),
-               
reinterpret_cast<SkPictureStateTree::Draw**>(activeOps.end()-1));
         }
     }
 
-    SkPictureStateTree::Iterator it = (NULL == fStateTree) ?
+    SkPictureStateTree::Iterator it = (NULL == activeOps) ?
         SkPictureStateTree::Iterator() :
-        fStateTree->getIterator(activeOps, &canvas);
+        fStateTree->getIterator(*activeOps, &canvas);
 
     if (it.isValid()) {
         uint32_t skipTo = it.draw();

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

committed as r13831

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

This was reverted in r13832 due to Mac compilation issues.

[robertphillips, 2014-03-17 17:36:26]

The CQ bit was checked by robertphillips@google.com

[commit-bot: I haz the power, 2014-03-17 17:36:35]

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

[commit-bot: I haz the power, 2014-03-17 18:12:01]

Change committed as 13836

[robertphillips, 2014-03-18 16:23:46]

This was reverted (again) in r13850 (Revert of r13836 due to Chromium
cc_unittests failures - https://codereview.chromium.org/203333005/)

[robertphillips, 2014-03-18 16:40:04]

The CQ bit was checked by robertphillips@google.com

[commit-bot: I haz the power, 2014-03-18 16:40:15]

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

[commit-bot: I haz the power, 2014-03-18 17:45:55]

Change committed as 13853

[tomhudson, 2014-03-19 10:22:01]

So what are "active" operations?

[robertphillips, 2014-03-19 11:57:11]

The operations needed to render a given rectangle. Basically this gives
SkGpuDevice access to the BBH results so the required textures can be preloaded
into VRAM.