Summary: | [chromium] Replace use of ManagedTexture with CCScopedTexture for impl thread and remove implTextureManager from LayerRendererChromium | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Dana Jansens <danakj> | ||||||||||||
Component: | New Bugs | Assignee: | Dana Jansens <danakj> | ||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||
Severity: | Normal | CC: | aelias, cc-bugs, enne, hayato, jamesr, piman, webkit.review.bot, wjmaclean, zlieber | ||||||||||||
Priority: | P2 | ||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||
Hardware: | Unspecified | ||||||||||||||
OS: | Unspecified | ||||||||||||||
Bug Depends on: | 89485, 90702 | ||||||||||||||
Bug Blocks: | |||||||||||||||
Attachments: |
|
Description
Dana Jansens
2012-07-09 17:43:00 PDT
Created attachment 151371 [details]
Patch
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// 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. 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. Created attachment 151380 [details]
Patch
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. Created attachment 151382 [details]
Patch
(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) 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? 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. *** Bug 90848 has been marked as a duplicate of this bug. *** Created attachment 151512 [details]
Patch for landing
(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; Created attachment 151522 [details]
Patch
Sounds good. :) Cool, thanks. Committed r122272: <http://trac.webkit.org/changeset/122272> |