[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 #22 from Dana Jansens <danakj at chromium.org>  2012-06-13 12:43:47 PST ---
(From update of attachment 147153)
View in context: https://bugs.webkit.org/attachment.cgi?id=147153&action=review

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:399
> +    // The pass was alreday removed by another quad - probably the original, and we are the replica

nit: "already" and end the comment with a period.

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:409
> +    while (quadListIterator != quadList.backToFrontEnd()) {

for loop? even if the variable is initialized the line above, it lets you put the increment up here

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:411
> +        if (currentQuad->material() == CCDrawQuad::RenderPass) {

Use != and continue to nest less.

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:426
> +    ssize_t passIndex = passes.size() - 1;
> +    while (passIndex >= 0) {

for loop?

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:430
> +        while (quadListIterator != quadList.backToFrontEnd()) {

for loop?

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:431
> +            CCDrawQuad* currentQuad = (*quadListIterator).get();

quadListIterator->get()?

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:432
> +            if (currentQuad->material() == CCDrawQuad::RenderPass) {

Use != and continue to nest less.

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:435
> +                if (!renderPassQuad->contentsChanged() && targetSurface->hasCachedContentsTexture()) {

Same here, can use continue instead of nesting.

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:445
> +                        // We are changing the vector in the middle of reverse iteration.
> +                        // We are guaranteed that any data from iterator to the end will not change.
> +                        // Capture the iterator position from the end, and restore it after the change.
> +                        int positionFromEnd = passes.size() - passIndex;
> +                        removeRenderPassesRecursive(passes, passIndex, renderPassQuad->renderPass());
> +                        passIndex = passes.size() - positionFromEnd;
> +                        ASSERT(passIndex >= 0);

Can we simplify this? Perhaps make removeRenderPassesRecursive() return the number of passes removed (including recusion) and then move iterator by that amount?

>>>> Source/WebCore/platform/graphics/chromium/cc/CCRenderSurface.cpp:92
>>>> +    if (layerRenderer) {
>>> 
>>> Can layerRenderer be null now? This method is only called from LayerRendererChromium so this seems weird. I assume this is for testing, but can you do the tests in a way that don't call this with null instead?
>> 
>> I think it makes sense in the contract of this method. When you are asking to prepare texture, you signal to the surface whether it should create it if non-existent or not. When reserving a texture that you just checked that exists (like we do now in CCLayerTreeHost::removePassesWithCachedTextures), passing, and therefore getting access to, an instance of LayerRendererChromium is redundant.
>> 
>> In fact I'd prefer to get rid of CCRenderSurface -> LayerRendererChromium dependency entirely. First, because it's circular, and second because surface should not have a knowledge of the way it's being used/drawn.
> 
> I'm working on that last part.

Oh I see, you're using this to reserve, I missed that, thanks. It might make sense to make a reserve() method instead and have prepareContentsTexture() call that? Then again I'm (re)moving this code entirely soon.

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