cc: Remove LayerTextureUpdater::Texture::updateRect() callback.

cc: Remove LayerTextureUpdater::Texture::updateRect() callback.

This is the first step towards cleaning up and simplifying the resource
update system. Eliminates the LayerTextureUpdater::Texture::updateRect()
callback by moving some logic to CCResourceProvider. Remaining logic is
now contained in LayerTextureUpdater::Texture::update(). The logic itself
is not changed, this is only re-factoring.

BUG=154472
TEST=manual


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=161752

[reveman, 2012-10-08 17:28:24]

This is a large change that pave the way for other resource update cleanups.
This doesn't change the behavior of the compositor in any way.

[nduca, 2012-10-08 17:35:01]

This is hard to evaluate without context. Can you give some context, e.g. "what
it looked like before, what it loosk like now, where you want it to be like in
the future"?

[piman, 2012-10-08 18:33:42]

https://chromiumcodereview.appspot.com/11074009/diff/1/cc/CCResourceProvider.h
File cc/CCResourceProvider.h (right):

https://chromiumcodereview.appspot.com/11074009/diff/1/cc/CCResourceProvider....
cc/CCResourceProvider.h:102: void acceleratedUpdate(ResourceId, SkPicture*,
const IntRect& pictureRect, const IntRect& sourceRect, const IntSize&
destOffset);
This feels a bit out of place. I'd rather have this sit in a separate
function/class, outside of CCResourceProvider if possible. I would like to keep
CCResourceProvider as simple as possible, and not know anything about painting,
or the magic shared contexts for example.

[reveman, 2012-10-08 19:45:01]

https://chromiumcodereview.appspot.com/11074009/diff/1/cc/CCResourceProvider.h
File cc/CCResourceProvider.h (right):

https://chromiumcodereview.appspot.com/11074009/diff/1/cc/CCResourceProvider....
cc/CCResourceProvider.h:102: void acceleratedUpdate(ResourceId, SkPicture*,
const IntRect& pictureRect, const IntRect& sourceRect, const IntSize&
destOffset);
On 2012/10/08 18:33:42, piman wrote:
> This feels a bit out of place. I'd rather have this sit in a separate
> function/class, outside of CCResourceProvider if possible. I would like to
keep
> CCResourceProvider as simple as possible, and not know anything about
painting,
> or the magic shared contexts for example.

Ok, maybe it belongs in CCTextureUpdateController. Whether we can use
accelerated painting or not depends on the resource type so we'll need some
minimal CCResourceProvider interface to handle accelerated painting properly.

[piman, 2012-10-08 19:49:30]

On 2012/10/08 19:45:01, David Reveman wrote:
> https://chromiumcodereview.appspot.com/11074009/diff/1/cc/CCResourceProvider.h
> File cc/CCResourceProvider.h (right):
> 
>
https://chromiumcodereview.appspot.com/11074009/diff/1/cc/CCResourceProvider....
> cc/CCResourceProvider.h:102: void acceleratedUpdate(ResourceId, SkPicture*,
> const IntRect& pictureRect, const IntRect& sourceRect, const IntSize&
> destOffset);
> On 2012/10/08 18:33:42, piman wrote:
> > This feels a bit out of place. I'd rather have this sit in a separate
> > function/class, outside of CCResourceProvider if possible. I would like to
> keep
> > CCResourceProvider as simple as possible, and not know anything about
> painting,
> > or the magic shared contexts for example.
> 
> Ok, maybe it belongs in CCTextureUpdateController. Whether we can use
> accelerated painting or not depends on the resource type so we'll need some
> minimal CCResourceProvider interface to handle accelerated painting properly.

Isn't resourceType() enough?
Also, it would be much better to take the problem the opposite way: if you want
to do accelerated painting, you need to create the resource as a GL resource,
not a software one.

[reveman, 2012-10-08 20:19:52]

On 2012/10/08 19:49:30, piman wrote:
> On 2012/10/08 19:45:01, David Reveman wrote:
> >
https://chromiumcodereview.appspot.com/11074009/diff/1/cc/CCResourceProvider.h
> > File cc/CCResourceProvider.h (right):
> > 
> >
>
https://chromiumcodereview.appspot.com/11074009/diff/1/cc/CCResourceProvider....
> > cc/CCResourceProvider.h:102: void acceleratedUpdate(ResourceId, SkPicture*,
> > const IntRect& pictureRect, const IntRect& sourceRect, const IntSize&
> > destOffset);
> > On 2012/10/08 18:33:42, piman wrote:
> > > This feels a bit out of place. I'd rather have this sit in a separate
> > > function/class, outside of CCResourceProvider if possible. I would like to
> > keep
> > > CCResourceProvider as simple as possible, and not know anything about
> > painting,
> > > or the magic shared contexts for example.
> > 
> > Ok, maybe it belongs in CCTextureUpdateController. Whether we can use
> > accelerated painting or not depends on the resource type so we'll need some
> > minimal CCResourceProvider interface to handle accelerated painting
properly.
> 
> Isn't resourceType() enough?
> Also, it would be much better to take the problem the opposite way: if you
want
> to do accelerated painting, you need to create the resource as a GL resource,
> not a software one.

Sounds good, let's make --enable-accelerated-painting force usage of GL
resources.

We generally prefer to keep the resource type transparent, right? I feel like I
wouldn't want to make use of resourceType() except in an assert statement but
maybe that's too strict?

[piman, 2012-10-08 20:26:05]

On Mon, Oct 8, 2012 at 1:19 PM, <reveman@chromium.org> wrote:

> On 2012/10/08 19:49:30, piman wrote:
>
>> On 2012/10/08 19:45:01, David Reveman wrote:
>> >
>>
> https://chromiumcodereview.**appspot.com/11074009/diff/1/**
>
cc/CCResourceProvider.h<https://chromiumcodereview.appspot.com/11074009/diff/1/cc/CCResourceProvider.h>
>
>> > File cc/CCResourceProvider.h (right):
>> >
>> >
>>
>
> https://chromiumcodereview.**appspot.com/11074009/diff/1/**
>
cc/CCResourceProvider.h#**newcode102<https://chromiumcodereview.appspot.com/11074009/diff/1/cc/CCResourceProvider.h#newcode102>
>
>> > cc/CCResourceProvider.h:102: void acceleratedUpdate(ResourceId,
>> SkPicture*,
>> > const IntRect& pictureRect, const IntRect& sourceRect, const IntSize&
>> > destOffset);
>> > On 2012/10/08 18:33:42, piman wrote:
>> > > This feels a bit out of place. I'd rather have this sit in a separate
>> > > function/class, outside of CCResourceProvider if possible. I would
>> like to
>> > keep
>> > > CCResourceProvider as simple as possible, and not know anything about
>> > painting,
>> > > or the magic shared contexts for example.
>> >
>> > Ok, maybe it belongs in CCTextureUpdateController. Whether we can use
>> > accelerated painting or not depends on the resource type so we'll need
>> some
>> > minimal CCResourceProvider interface to handle accelerated painting
>>
> properly.
>
>  Isn't resourceType() enough?
>> Also, it would be much better to take the problem the opposite way: if you
>>
> want
>
>> to do accelerated painting, you need to create the resource as a GL
>> resource,
>> not a software one.
>>
>
> Sounds good, let's make --enable-accelerated-painting force usage of GL
> resources.
>

SG


>
> We generally prefer to keep the resource type transparent, right? I feel
> like I
> wouldn't want to make use of resourceType() except in an assert statement
> but
> maybe that's too strict?
>

Agreed, I think that's a good thing to do.

Antoine


>
>
https://chromiumcodereview.**appspot.com/11074009/<https://chromiumcodereview...
>

[nduca, 2012-10-10 00:31:44]

Another option here folks is to r

[nduca, 2012-10-10 00:32:18]

Whoops, didn't mean to comment. Nothing to say here. :)

[enne (OOO), 2012-10-12 21:25:04]

lgtm.  I like this cleanup a lot.

https://codereview.chromium.org/11074009/diff/3002/cc/LayerTextureUpdater.h
File cc/LayerTextureUpdater.h (right):

https://codereview.chromium.org/11074009/diff/3002/cc/LayerTextureUpdater.h#n...
cc/LayerTextureUpdater.h:34: virtual void update(CCTextureUpdateQueue&, const
IntRect& sourceRect, const IntSize& destOffset, bool partialUpdate,
CCRenderingStats&) = 0;
This is a small nit, but could you just figure out using sourceRect size /
destOffset / m_texture size to know if this was a partial update implicitly?

[reveman, 2012-10-12 21:43:11]

https://codereview.chromium.org/11074009/diff/3002/cc/LayerTextureUpdater.h
File cc/LayerTextureUpdater.h (right):

https://codereview.chromium.org/11074009/diff/3002/cc/LayerTextureUpdater.h#n...
cc/LayerTextureUpdater.h:34: virtual void update(CCTextureUpdateQueue&, const
IntRect& sourceRect, const IntSize& destOffset, bool partialUpdate,
CCRenderingStats&) = 0;
On 2012/10/12 21:25:04, enne wrote:
> This is a small nit, but could you just figure out using sourceRect size /
> destOffset / m_texture size to know if this was a partial update implicitly?

Yes, that's my plan. The problem is that 'partialUpdate' currently means two
things. 1) we're updating part of the texture. 2) the update is not buffered. We
care about both these things on the impl side. The size of the update matters
for our cost estimate and if it's buffered affects our ability to do incremental
updates.

My plans is to use sourceRect, destOffset and m_texture to determine if it's a
full update and use a separate property (in_use_on_impl_) to determine if it's a
buffered update.

[commit-bot: I haz the power, 2012-10-12 21:45:24]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/reveman@chromium.org/11074009/3002

[commit-bot: I haz the power, 2012-10-12 21:45:35]

Failed to apply patch for cc/CCLayerTreeHostTest.cpp:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file cc/CCLayerTreeHostTest.cpp
  Hunk #1 FAILED at 2398.
  Hunk #2 FAILED at 2432.
  Hunk #3 FAILED at 2447.
  Hunk #4 FAILED at 2483.
  Hunk #5 FAILED at 2492.
  Hunk #6 FAILED at 2506.
  Hunk #7 FAILED at 2573.
  Hunk #8 FAILED at 2615.
  Hunk #9 FAILED at 2706.
  9 out of 9 hunks FAILED -- saving rejects to file
cc/CCLayerTreeHostTest.cpp.rej

Patch:       cc/CCLayerTreeHostTest.cpp
Index: cc/CCLayerTreeHostTest.cpp
diff --git a/cc/CCLayerTreeHostTest.cpp b/cc/CCLayerTreeHostTest.cpp
index
129fa613579f66d72857015f5427280eaca8333f..b0aaef12d207c27403be1435ef5760e89625dbfc
100644
--- a/cc/CCLayerTreeHostTest.cpp
+++ b/cc/CCLayerTreeHostTest.cpp
@@ -2398,29 +2398,6 @@ private:
 

SINGLE_AND_MULTI_THREAD_TEST_F(CCLayerTreeHostTestSurfaceNotAllocatedForLayersOutsideMemoryLimit)
 
-
-class EvictionTrackingTexture : public LayerTextureUpdater::Texture {
-public:
-    static PassOwnPtr<EvictionTrackingTexture>
create(scoped_ptr<CCPrioritizedTexture> texture) { return adoptPtr(new
EvictionTrackingTexture(texture.Pass())); }
-    virtual ~EvictionTrackingTexture() { }
-
-    virtual void updateRect(CCResourceProvider* resourceProvider, const
IntRect&, const IntSize&) OVERRIDE
-    {
-        ASSERT_TRUE(!texture()->haveBackingTexture() ||
resourceProvider->numResources() > 0);
-        texture()->acquireBackingTexture(resourceProvider);
-        m_updated = true;
-    }
-    void resetUpdated() { m_updated = false; }
-    bool updated() const { return m_updated; }
-
-private:
-    explicit EvictionTrackingTexture(scoped_ptr<CCPrioritizedTexture> texture)
-        : LayerTextureUpdater::Texture(texture.Pass())
-        , m_updated(false)
-    { }
-    bool m_updated;
-};
-
 class EvictionTestLayer : public LayerChromium {
 public:
     static scoped_refptr<EvictionTestLayer> create() { return
make_scoped_refptr(new EvictionTestLayer()); }
@@ -2432,12 +2409,7 @@ public:
     virtual void pushPropertiesTo(CCLayerImpl*) OVERRIDE;
     virtual void setTexturePriorities(const CCPriorityCalculator&) OVERRIDE;
 
-    void resetUpdated()
-    {
-        if (m_texture.get())
-            m_texture->resetUpdated();
-    }
-    bool updated() const { return m_texture.get() ? m_texture->updated() :
false; }
+    bool haveBackingTexture() const { return m_texture.get() ?
m_texture->haveBackingTexture() : false; }
 
 private:
     EvictionTestLayer() : LayerChromium() { }
@@ -2447,11 +2419,13 @@ private:
     {
         if (m_texture.get())
             return;
-        m_texture =
EvictionTrackingTexture::create(CCPrioritizedTexture::create(layerTreeHost()->contentsTextureManager()));
-        m_texture->texture()->setDimensions(IntSize(10, 10),
cc::GraphicsContext3D::RGBA);
+        m_texture =
CCPrioritizedTexture::create(layerTreeHost()->contentsTextureManager());
+        m_texture->setDimensions(IntSize(10, 10), cc::GraphicsContext3D::RGBA);
+        m_bitmap.setConfig(SkBitmap::kARGB_8888_Config, 10, 10);
     }
 
-    OwnPtr<EvictionTrackingTexture> m_texture;
+    scoped_ptr<CCPrioritizedTexture> m_texture;
+    SkBitmap m_bitmap;
 };
 
 class EvictionTestLayerImpl : public CCLayerImpl {
@@ -2483,7 +2457,7 @@ void EvictionTestLayer::setTexturePriorities(const
CCPriorityCalculator&)
     createTextureIfNeeded();
     if (!m_texture.get())
         return;
-   
m_texture->texture()->setRequestPriority(CCPriorityCalculator::uiPriority(true));
+    m_texture->setRequestPriority(CCPriorityCalculator::uiPriority(true));
 }
 
 void EvictionTestLayer::update(CCTextureUpdateQueue& queue, const
CCOcclusionTracker*, CCRenderingStats&)
@@ -2492,8 +2466,8 @@ void EvictionTestLayer::update(CCTextureUpdateQueue&
queue, const CCOcclusionTra
     if (!m_texture.get())
         return;
     IntRect fullRect(0, 0, 10, 10);
-    TextureUploader::Parameters parameters = { m_texture.get(), fullRect,
IntSize() };
-    queue.appendFullUpload(parameters);
+    TextureUploader::Parameters upload = { m_texture.get(), &m_bitmap, NULL, {
fullRect, fullRect, IntSize() } };
+    queue.appendFullUpload(upload);
 }
 
 PassOwnPtr<CCLayerImpl> EvictionTestLayer::createCCLayerImpl()
@@ -2506,7 +2480,7 @@ void EvictionTestLayer::pushPropertiesTo(CCLayerImpl*
layerImpl)
     LayerChromium::pushPropertiesTo(layerImpl);
 
     EvictionTestLayerImpl* testLayerImpl =
static_cast<EvictionTestLayerImpl*>(layerImpl);
-    testLayerImpl->setHasTexture(m_texture->texture()->haveBackingTexture());
+    testLayerImpl->setHasTexture(m_texture->haveBackingTexture());
 }
 
 class CCLayerTreeHostTestEvictTextures : public CCLayerTreeHostTest {
@@ -2573,23 +2547,23 @@ public:
     {
         switch (m_numCommits) {
         case 1:
-            EXPECT_TRUE(m_layer->updated());
+            EXPECT_TRUE(m_layer->haveBackingTexture());
             postEvictTextures();
             break;
         case 2:
-            EXPECT_TRUE(m_layer->updated());
+            EXPECT_TRUE(m_layer->haveBackingTexture());
             m_layerTreeHost->setNeedsCommit();
             break;
         case 3:
             break;
         case 4:
-            EXPECT_TRUE(m_layer->updated());
+            EXPECT_TRUE(m_layer->haveBackingTexture());
             m_layerTreeHost->setNeedsCommit();
             break;
         case 5:
             break;
         case 6:
-            EXPECT_TRUE(m_layer->updated());
+            EXPECT_TRUE(m_layer->haveBackingTexture());
             endTest();
             break;
         default:
@@ -2615,21 +2589,20 @@ public:
             break;
         case 4:
             // We couldn't check in didCommitAndDrawFrame on commit 3, so check
here.
-            EXPECT_FALSE(m_layer->updated());
+            EXPECT_FALSE(m_layer->haveBackingTexture());
             break;
         case 5:
             postEvictTextures();
             break;
         case 6:
             // We couldn't check in didCommitAndDrawFrame on commit 5, so check
here.
-            EXPECT_FALSE(m_layer->updated());
+            EXPECT_FALSE(m_layer->haveBackingTexture());
             postEvictTextures();
             break;
         default:
             ASSERT_NOT_REACHED();
             break;
         }
-        m_layer->resetUpdated();
     }
 
     virtual void afterTest() OVERRIDE
@@ -2706,7 +2679,7 @@ public:
         ++m_numCommits;
         switch (m_numCommits) {
         case 1:
-            EXPECT_TRUE(m_layer->updated());
+            EXPECT_TRUE(m_layer->haveBackingTexture());
             m_layerTreeHost->setVisible(false);
             postEvictTextures();
             m_layerTreeHost->loseContext(1);

[commit-bot: I haz the power, 2012-10-12 22:11:21]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/reveman@chromium.org/11074009/25001

[commit-bot: I haz the power, 2012-10-13 06:14:09]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/reveman@chromium.org/11074009/25003

[commit-bot: I haz the power, 2012-10-13 10:20:29]

Change committed as 161752