Bug 90841 – [chromium] Replace use of ManagedTexture with CCScopedTexture for impl thread and remove implTextureManager from LayerRendererChromium

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Dana Jansens]

[chromium] Replace use of ManagedTexture with CCScopedTexture for impl thread and remove implTextureManager from LayerRendererChromium

[Dana Jansens]

Created attachment 151371 [details]
Patch

[Adrienne Walker]

Comment on attachment 151371 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=151371&action=review

Can you manually test the HUD, since we have no automated tests for it?

> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:610
> +    CCScopedTexture* contentsTexture = m_renderPassTextures.get(quad->renderPassId());
> +    if (!contentsTexture || !contentsTexture->id())

Is this something we should assert about? Presumably this should have been allocated previously.

> Source/WebCore/platform/graphics/chromium/TrackingTextureAllocator.h:59
> -    explicit TrackingTextureAllocator(WebKit::WebGraphicsContext3D*);
> +    explicit TrackingTextureAllocator(WebKit::WebGraphicsContext3D*, int maxTextureSize);

s/explicit//

[Dana Jansens]

Comment on attachment 151371 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=151371&action=review

I did run with fps counter and it seems to be fine.

>> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:610
>> +    if (!contentsTexture || !contentsTexture->id())
> 
> Is this something we should assert about? Presumably this should have been allocated previously.

If a RenderPass is removed due to being empty, the DrawQuad is still kept (we don't modify the quadList). So we'll get here and there will be no texture (we did this to avoid allocating the texture).

>> Source/WebCore/platform/graphics/chromium/TrackingTextureAllocator.h:59
>> +    explicit TrackingTextureAllocator(WebKit::WebGraphicsContext3D*, int maxTextureSize);
> 
> s/explicit//

ah yes.

[Dana Jansens]

Comment on attachment 151371 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=151371&action=review

> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:315
> +IntSize LayerRendererChromium::renderPassTextureSize(const CCRenderPass* pass)

note to self: this can return const IntSize& after https://bugs.webkit.org/show_bug.cgi?id=90844 lands.

[Dana Jansens]

Created attachment 151380 [details]
Patch

[Dana Jansens]

Comment on attachment 151371 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=151371&action=review

>>> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:610
>>> +    if (!contentsTexture || !contentsTexture->id())
>> 
>> Is this something we should assert about? Presumably this should have been allocated previously.
> 
> If a RenderPass is removed due to being empty, the DrawQuad is still kept (we don't modify the quadList). So we'll get here and there will be no texture (we did this to avoid allocating the texture).

For more context, https://bugs.webkit.org/show_bug.cgi?id=90702 changes this from an assert to a return for the above reason.

[Dana Jansens]

Created attachment 151382 [details]
Patch

[Dana Jansens]

(In reply to comment #6)
> For more context, https://bugs.webkit.org/show_bug.cgi?id=90702 changes this from an assert to a return for the above reason.

Another reason to not assert. If a surface is larger than the max texture size, we are not able to allocate it, but the quad will still be there trying to draw from the non-existent texture. (Similar issue came up with another assert in https://bugs.webkit.org/show_bug.cgi?id=90848)

[Adrienne Walker]

Comment on attachment 151382 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=151382&action=review

R=me, assuming the rebase is trivial and you CQ+ it to get test coverage.

> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:1095
> +    ASSERT(hudTexture->id());

I think you should be robust to this.  What if you have the HUD open and have a stupidly large browser window, larger than the max texture size?

[Dana Jansens]

Comment on attachment 151382 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=151382&action=review

>> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:1095
>> +    ASSERT(hudTexture->id());
> 
> I think you should be robust to this.  What if you have the HUD open and have a stupidly large browser window, larger than the max texture size?

Yaaa, good point! Max texture size has come up in these changes, I hadn't thot of it so much before. Thanks. I'll double think my other asserts in here too.

[Dana Jansens]

*** Bug 90848 has been marked as a duplicate of this bug. ***

[Dana Jansens]

Created attachment 151512 [details]
Patch for landing

[Dana Jansens]

(In reply to comment #10)
> (From update of attachment 151382 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=151382&action=review
> 
> >> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:1095
> >> +    ASSERT(hudTexture->id());
> > 
> > I think you should be robust to this.  What if you have the HUD open and have a stupidly large browser window, larger than the max texture size?
> 
> Yaaa, good point! Max texture size has come up in these changes, I hadn't thot of it so much before. Thanks. I'll double think my other asserts in here too.

We verify the texture is allocated in CCHeadsUpDisplay now before calling this function. So I think this ASSERT is not out of place. However, the allocation in useRenderPass could have failed and triggered a bad assert.

Diff:

diff --git a/Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp b/Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp
index ba3c255..657365c 100644
--- a/Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp
+++ b/Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp
@@ -1378,14 +1378,16 @@ bool LayerRendererChromium::useRenderPass(const CCRenderPass* renderPass)
     CCScopedTexture* texture = m_renderPassTextures.get(renderPass->id());
     ASSERT(texture);
 
-    if (!texture->id())
-        texture->allocate(renderPassTextureSize(renderPass), renderPassTextureFormat(renderPass));
+    if (!texture->id() && !texture->allocate(renderPassTextureSize(renderPass), renderPassTextureFormat(renderPass)))
+        return false;
 
     return bindFramebufferToTexture(texture, renderPass->framebufferOutputRect());
 }
 
 bool LayerRendererChromium::useScopedTexture(const CCScopedTexture* texture, const IntRect& viewportRect)
 {
+    ASSERT(texture->id());
+
     m_currentRenderPass = 0;
     m_currentTexture = texture;

[Dana Jansens]

Created attachment 151522 [details]
Patch

[Adrienne Walker]

Sounds good.  :)

[Dana Jansens]

Cool, thanks.

[Dana Jansens]

Committed r122272: <http://trac.webkit.org/changeset/122272>