Bug 87877 – [chromium] Move drawing code for RenderSurfaces into LayerRendererChromium

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

[Dana Jansens]

[chromium] Move drawing code for RenderSurfaces into LayerRendererChromium

[Dana Jansens]

Created attachment 144874 [details]
Patch

[James Robinson]

Comment on attachment 144874 [details]
Patch

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

> Source/WebCore/platform/graphics/chromium/cc/CCLayerImpl.cpp:172
> +Platform3DObject CCLayerImpl::contentsTextureId() const

just use "unsigned" for texture IDs

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:-482
> -        if (layer->maskLayer()) {
> -            renderSurface->setMaskLayer(layer->maskLayer());
> +        if (layer->maskLayer())
>              layer->maskLayer()->setTargetRenderSurface(renderSurface);
> -        } else
> -            renderSurface->setMaskLayer(0);

I'm not sure I understand this change - could you explain?

> Source/WebCore/platform/graphics/chromium/cc/CCRenderSurfaceDrawQuad.h:29
> +#include "GraphicsTypes3D.h"

this is unnecessary, just use unsigned for texture ids

> Source/WebCore/platform/graphics/chromium/cc/CCRenderSurfaceDrawQuad.h:36
> +#include <public/WebFilterOperations.h>
>  #include <wtf/PassOwnPtr.h>
>  
> +namespace WebKit {
> +class WebFilterOperations;
> +}

you don't need a forward declaration if you are #including the header

[Dana Jansens]

Comment on attachment 144874 [details]
Patch

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

>> Source/WebCore/platform/graphics/chromium/cc/CCLayerImpl.cpp:172
>> +Platform3DObject CCLayerImpl::contentsTextureId() const
> 
> just use "unsigned" for texture IDs

done.

>> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:-482
>> -            renderSurface->setMaskLayer(0);
> 
> I'm not sure I understand this change - could you explain?

RenderSurface uses as its mask either either m_owningLayer->maskLayer or m_owningLayer->replicaLayer->maskLayer. So, while being a bit of a lie here, it's also not needed, the surface already has access to this.

>> Source/WebCore/platform/graphics/chromium/cc/CCRenderSurfaceDrawQuad.h:29
>> +#include "GraphicsTypes3D.h"
> 
> this is unnecessary, just use unsigned for texture ids

k

>> Source/WebCore/platform/graphics/chromium/cc/CCRenderSurfaceDrawQuad.h:36
>> +}
> 
> you don't need a forward declaration if you are #including the header

oops yeh, thanks.

[Dana Jansens]

Created attachment 144879 [details]
Patch

Comments addressed, and dropped a couple unneeded headers from CCRenderSurface.cpp.

[Adrienne Walker]

Comment on attachment 144879 [details]
Patch

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

> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:657
> +    // The replica is always drawn first, so free after drawing the contents.
> +    bool shouldReleaseTextures = !quad->isReplica();

What if the contents surface quad is occluded/clipped but the replica is not? How would this texture get unreserved? The previous behavior looks like we always unreserved after the first render surface quad draws (possibly a replica) and happily because nothing else reserved textures (causing potential evictions), then those textures were still "valid" to use for the second render surface quad even if those textures weren't reserved.  (Ack.) However, the previous behavior seems mildly safer in that you couldn't ever have permanently reserved surface textures.

How can this be better? Maybe you should unreserve all render surface textures at the end of a frame? Do you need some sort of dummy quad that unreserves textures? Can you check to see if the quads actually get added and set a shouldReleaseTextures flag on them?

> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:680
> +    if (drawingSurface->hasValidContentsTexture()) {

Since you can't draw the background without a contents texture to draw into, can you move this early out up above with the contentsDeviceTransform.isInvertible check?

> Source/WebCore/platform/graphics/chromium/cc/CCRenderPass.cpp:67
> +    surface->appendQuads(quadCuller, sharedQuadState.get(), false, surfaceDamageRect());

style nit: unnamed boolean function arg.  Can you name this isReplica still like in the previous code or have two different appendQuads functions?

> Source/WebCore/platform/graphics/chromium/cc/CCRenderSurface.cpp:47
> +static const int debugSurfaceBorderWidth = 2;

(We really need a CCColorConstants class... Not in this patch, though.)

[Dana Jansens]

Comment on attachment 144879 [details]
Patch

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

>> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:657
>> +    bool shouldReleaseTextures = !quad->isReplica();
> 
> What if the contents surface quad is occluded/clipped but the replica is not? How would this texture get unreserved? The previous behavior looks like we always unreserved after the first render surface quad draws (possibly a replica) and happily because nothing else reserved textures (causing potential evictions), then those textures were still "valid" to use for the second render surface quad even if those textures weren't reserved.  (Ack.) However, the previous behavior seems mildly safer in that you couldn't ever have permanently reserved surface textures.
> 
> How can this be better? Maybe you should unreserve all render surface textures at the end of a frame? Do you need some sort of dummy quad that unreserves textures? Can you check to see if the quads actually get added and set a shouldReleaseTextures flag on them?

Oh argh.. yeh. Adding an unprotectAllTextures at the end of the frame.

>> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:680
>> +    if (drawingSurface->hasValidContentsTexture()) {
> 
> Since you can't draw the background without a contents texture to draw into, can you move this early out up above with the contentsDeviceTransform.isInvertible check?

yup!

>> Source/WebCore/platform/graphics/chromium/cc/CCRenderPass.cpp:67
>> +    surface->appendQuads(quadCuller, sharedQuadState.get(), false, surfaceDamageRect());
> 
> style nit: unnamed boolean function arg.  Can you name this isReplica still like in the previous code or have two different appendQuads functions?

right, i was kinda like "hm.." about this, thanks.

>> Source/WebCore/platform/graphics/chromium/cc/CCRenderSurface.cpp:47
>> +static const int debugSurfaceBorderWidth = 2;
> 
> (We really need a CCColorConstants class... Not in this patch, though.)

true!

[Dana Jansens]

Created attachment 144933 [details]
Patch

[James Robinson]

Comment on attachment 144933 [details]
Patch

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

R=me, few nits here and there.  Nice work.

> Source/WebCore/platform/graphics/chromium/cc/CCRenderPass.cpp:69
> +    bool isReplica;
> +
>      OwnPtr<CCSharedQuadState> sharedQuadState = surface->createSharedQuadState();
> -    if (layer->hasDebugBorders()) {
> -        Color color(debugSurfaceBorderColorRed, debugSurfaceBorderColorGreen, debugSurfaceBorderColorBlue, debugSurfaceBorderAlpha);
> -        quadCuller.appendSurface(CCDebugBorderDrawQuad::create(sharedQuadState.get(), surface->contentRect(), color, debugSurfaceBorderWidth));
> -    }
> -    bool isReplica = false;
> -    quadCuller.appendSurface(CCRenderSurfaceDrawQuad::create(sharedQuadState.get(), surface->contentRect(), layer, surfaceDamageRect(), isReplica));
> +    isReplica = false;

I'd prefer initializing this bool on the same line as we declare it, just to be a bit clearer what's going on

> Source/WebCore/platform/graphics/chromium/cc/CCRenderSurface.cpp:112
> +    IntSize requiredSize(m_contentRect.size());
> +    return m_contentsTexture && m_contentsTexture->isReserved() && m_contentsTexture->isValid(requiredSize, GraphicsContext3D::RGBA);

why is a temp IntSize needed here? can you just pass m_contentRect.size() to isValid() ?

> Source/WebCore/platform/graphics/chromium/cc/CCRenderSurface.cpp:142
> +    IntSize requiredSize(m_contentRect.size());
> +    return m_backgroundTexture && m_backgroundTexture->isReserved() && m_backgroundTexture->isValid(requiredSize, GraphicsContext3D::RGBA);

same Q here

> Source/WebCore/platform/graphics/chromium/cc/CCRenderSurface.cpp:264
> +        int red = !forReplica ? debugSurfaceBorderColorRed : debugReplicaBorderColorRed;

the ! ? : makes my brain hurt - why not the other way 'round i.e. "forReplica ? debugReplicaBorderColorRed : ..." ?

> Source/WebCore/platform/graphics/chromium/cc/CCRenderSurfaceDrawQuad.cpp:46
> +    m_filters.assign(filters);
> +    m_backgroundFilters.assign(backgroundFilters);

can you put these in the initializer list like other members, or does that not work with WebFilterOperations? i.e. just have

, m_filters(filters)
, m_backgroundFilters(backgroundFilters)

?

If that doesn't work we should add the appropriate c'tor to WebFilterOperations

[Dana Jansens]

Comment on attachment 144933 [details]
Patch

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

thanks!

>> Source/WebCore/platform/graphics/chromium/cc/CCRenderPass.cpp:69
>> +    isReplica = false;
> 
> I'd prefer initializing this bool on the same line as we declare it, just to be a bit clearer what's going on

k

>> Source/WebCore/platform/graphics/chromium/cc/CCRenderSurface.cpp:112
>> +    return m_contentsTexture && m_contentsTexture->isReserved() && m_contentsTexture->isValid(requiredSize, GraphicsContext3D::RGBA);
> 
> why is a temp IntSize needed here? can you just pass m_contentRect.size() to isValid() ?

sure yeh.

>> Source/WebCore/platform/graphics/chromium/cc/CCRenderSurface.cpp:264
>> +        int red = !forReplica ? debugSurfaceBorderColorRed : debugReplicaBorderColorRed;
> 
> the ! ? : makes my brain hurt - why not the other way 'round i.e. "forReplica ? debugReplicaBorderColorRed : ..." ?

haha ok, i liked surface as the "true" case, but that's fine :)

>> Source/WebCore/platform/graphics/chromium/cc/CCRenderSurfaceDrawQuad.cpp:46
>> +    m_backgroundFilters.assign(backgroundFilters);
> 
> can you put these in the initializer list like other members, or does that not work with WebFilterOperations? i.e. just have
> 
> , m_filters(filters)
> , m_backgroundFilters(backgroundFilters)
> 
> ?
> 
> If that doesn't work we should add the appropriate c'tor to WebFilterOperations

Right it doesn't have a copy c'tor. It seemed un-public-api-like, so I didn't question, but we should add that then!

[Dana Jansens]

Created attachment 145128 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 145128 [details]
Patch for landing

Clearing flags on attachment: 145128

Committed r119139: <http://trac.webkit.org/changeset/119139>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.