[Webkit-unassigned] [Bug 88482] [Chromium] Compositor should avoid drawing quads when cached textures are available and contents unchanged

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=88482





--- Comment #38 from Adrienne Walker <enne at google.com>  2012-06-14 18:47:48 PST ---
(From update of attachment 147685)
View in context: https://bugs.webkit.org/attachment.cgi?id=147685&action=review

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:267
> -bool CCLayerTreeHostImpl::calculateRenderPasses(CCRenderPassList& passes, CCLayerList& renderSurfaceLayerList, CCLayerList& willDrawLayers)
> +bool CCLayerTreeHostImpl::calculateRenderPasses(CCRenderPassList& passes, CCLayerList& renderSurfaceLayerList, CCLayerList& willDrawLayers, CCRenderPassList& skippedPasses)

Can this function just take FrameData& instead of all of parameters separately?

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:386
> +static ssize_t indexOfRenderPass(const CCRenderPassList& passes, const CCRenderPass* pass, size_t searchUntil)

Did you mean to use searchUntil?

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:397
> +    ssize_t removeIndex = indexOfRenderPass(passes, firstToRemove, bottomPass);

Maybe just use passes.find(firstToRemove) rather than roll your own?

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:445
> +            // Reserve the texture immediately. We do not need to pass in layer renderer
> +            // since the texture already exists, just needs to be reserved.
> +            if (!targetSurface->prepareContentsTexture(0))

If you need a static function for testing, then I'd rather you passed in the layer renderer to the function and forwarded that along to prepareContentsTexture, rather than making CCRenderSurface::prepareContentsTexture robust to a null LRC.

> Source/WebCore/platform/graphics/chromium/cc/CCRenderPass.h:61
> +    void appendQuad(PassOwnPtr<CCDrawQuad>);

I know you're adding this for testing, but can you not add this as a public function? The reason the other appendQuads functions don't take quads is so that the render pass can own the lifetime of the shared quad state.  With this function, it's not clear that the caller has to manage the lifetime of the shared quad state on the quad.

Maybe change the private modifier below to protected and make a CCTestRenderPass that exposes what you need?

> Source/WebCore/platform/graphics/chromium/cc/CCRenderPassDrawQuad.cpp:31
> +#include "cc/CCRenderPass.h"
> +

Not needed anymore?

> Source/WebCore/platform/graphics/chromium/cc/CCRenderPassDrawQuad.h:30
> +#include "cc/CCRenderSurface.h"

Not needed anymore?

> Source/WebCore/platform/graphics/chromium/cc/CCRenderSurface.h:184
> +    // FIXME: When the relationships between CCDamageTracker/CCRenderPass/CCRenderSurface/CCRenderPasDrawQuad
> +    // are fixed, this flag may go away. Right now needed to transfer the information from CCDamageTracker
> +    // to CCRenderPassDrawQuad without creating new class dependencies.
> +    bool m_contentsChanged;
> +

Unused?

> Source/WebKit/chromium/tests/CCLayerImplTest.cpp:130
> +    // Changing these properties only affects how render's surface is drawn

s/'s//

> Source/WebKit/chromium/tests/CCLayerTreeHostImplTest.cpp:2171
> +    mutable OwnPtr<CCRenderPass> renderPassPtr;

C++11's move semantics can't get here soon enough.

> Source/WebKit/chromium/tests/CCLayerTreeHostImplTest.cpp:2379
> +    EXPECT_STREQ(expectedResult, actualResult);

Can you pipe a string about which test case failed, e.g. "EXPECT_STREQ(expectedResult, actualResult) << testName;" so that when this fails there's more indication in the log about which case is failing?

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.