Bug 87877 - [chromium] Move drawing code for RenderSurfaces into LayerRendererChromium
Summary: [chromium] Move drawing code for RenderSurfaces into LayerRendererChromium
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Dana Jansens
URL:
Keywords:
Depends on: 87970
Blocks:
  Show dependency treegraph
 
Reported: 2012-05-30 11:06 PDT by Dana Jansens
Modified: 2012-05-31 13:42 PDT (History)
7 users (show)

See Also:


Attachments
Patch (57.15 KB, patch)
2012-05-30 11:12 PDT, Dana Jansens
no flags Details | Formatted Diff | Diff
Patch (57.07 KB, patch)
2012-05-30 11:31 PDT, Dana Jansens
no flags Details | Formatted Diff | Diff
Patch (57.47 KB, patch)
2012-05-30 15:05 PDT, Dana Jansens
no flags Details | Formatted Diff | Diff
Patch for landing (58.12 KB, patch)
2012-05-31 12:05 PDT, Dana Jansens
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dana Jansens 2012-05-30 11:06:35 PDT
[chromium] Move drawing code for RenderSurfaces into LayerRendererChromium
Comment 1 Dana Jansens 2012-05-30 11:12:29 PDT
Created attachment 144874 [details]
Patch
Comment 2 James Robinson 2012-05-30 11:20:23 PDT
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
Comment 3 Dana Jansens 2012-05-30 11:25:41 PDT
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.
Comment 4 Dana Jansens 2012-05-30 11:31:40 PDT
Created attachment 144879 [details]
Patch

Comments addressed, and dropped a couple unneeded headers from CCRenderSurface.cpp.
Comment 5 Adrienne Walker 2012-05-30 14:06:02 PDT
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.)
Comment 6 Dana Jansens 2012-05-30 15:03:44 PDT
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!
Comment 7 Dana Jansens 2012-05-30 15:05:18 PDT
Created attachment 144933 [details]
Patch
Comment 8 James Robinson 2012-05-30 18:55:15 PDT
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
Comment 9 Dana Jansens 2012-05-30 19:49:20 PDT
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!
Comment 10 Dana Jansens 2012-05-31 12:05:38 PDT
Created attachment 145128 [details]
Patch for landing
Comment 11 WebKit Review Bot 2012-05-31 13:41:56 PDT
Comment on attachment 145128 [details]
Patch for landing

Clearing flags on attachment: 145128

Committed r119139: <http://trac.webkit.org/changeset/119139>
Comment 12 WebKit Review Bot 2012-05-31 13:42:01 PDT
All reviewed patches have been landed.  Closing bug.