Bug 58840 - [chromium] Decouple layer visibility calculations from render surfaces / computed draw transforms
Summary: [chromium] Decouple layer visibility calculations from render surfaces / comp...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: James Robinson
URL:
Keywords:
Depends on: 58830 65354
Blocks: 58799
  Show dependency treegraph
 
Reported: 2011-04-18 15:05 PDT by James Robinson
Modified: 2011-08-02 16:21 PDT (History)
6 users (show)

See Also:


Attachments
Patch (107.24 KB, patch)
2011-07-28 19:17 PDT, James Robinson
no flags Details | Formatted Diff | Diff
compile fixes (108.59 KB, patch)
2011-07-29 10:09 PDT, James Robinson
no flags Details | Formatted Diff | Diff
Patch (87.21 KB, patch)
2011-07-29 18:33 PDT, James Robinson
no flags Details | Formatted Diff | Diff
Patch (86.82 KB, patch)
2011-08-01 15:48 PDT, James Robinson
no flags Details | Formatted Diff | Diff
fix copyright year (86.82 KB, patch)
2011-08-01 16:59 PDT, James Robinson
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description James Robinson 2011-04-18 15:05:16 PDT
Currently in ContentLayerChromium we determine the visible portion of the layer using the render surface scissor rect and the computed draw transform for the layer.  In the multithreaded compositor world, however, we'll be generating render surfaces and the final draw transform in the compositor thread since they might change from frame to frame (due to animation or whatnot).

We need to figure out some way to calculate visibility on the main thread, preferably without having to duplicate all of the draw transform + render surface bound calculations.
Comment 1 James Robinson 2011-05-02 17:59:13 PDT
I'm taking a look into this now.  My rough plan is as follows:

- land https://bugs.webkit.org/show_bug.cgi?id=58830 so the property pushing aspect of LRC::updatePropertiesAndRenderSurfaces() is separate from the transform portion

- make LayerRendererChromium::updatePropertiesAndRenderSurfaces() a generic function to calculate the draw transforms, visible regions, and z-order for a set of layers templated for LayerChromium+??? and CCLayerImpl+RenderSurfaceChromium so we can use the same codepath for LayerChromiums and CCLayerImpl.  This will also involve abstracting the "holds on to a z-order list of layers, transform and bounds" portion of RenderSurfaceChromium out from the "is usable as a render target and can draw itself" of RenderSurfaceChromium.

We'll run the algorithm to calculate draw transforms, visible regions, and z-order in the main thread portion using the LayerChromium tree and aggregate results into some class name TBD.  We'll then run the same code on the CCLayerImpl tree in the compositor thread and aggregate into real RenderSurfaceChromiums.
Comment 2 Adrienne Walker 2011-05-03 08:45:49 PDT
(In reply to comment #1)

> - make LayerRendererChromium::updatePropertiesAndRenderSurfaces() a generic function to calculate the draw transforms, visible regions, and z-order for a set of layers templated for LayerChromium+??? and CCLayerImpl+RenderSurfaceChromium so we can use the same codepath for LayerChromiums and CCLayerImpl.  This will also involve abstracting the "holds on to a z-order list of layers, transform and bounds" portion of RenderSurfaceChromium out from the "is usable as a render target and can draw itself" of RenderSurfaceChromium.

I was looking at some of this same code yesterday to try to better fix https://bugs.webkit.org/show_bug.cgi?id=59020.  The scissor rect / visible region calculation is wrong in one particular case, but I think there also needs to be some refactoring about where we calculate and store that scissor rect.  However, this sounds like exactly the same patch of code that you're about to uproot.

Would it make sense for you to try to address both at the same time? Otherwise, maybe I should postpone that fix until you're done reorganizing this?
Comment 3 James Robinson 2011-07-28 19:17:00 PDT
Created attachment 102323 [details]
Patch
Comment 4 James Robinson 2011-07-28 19:18:20 PDT
This patch might be a touch large to review (hah!).  I can think of at least one preliminary patch that I can land ahead of time to make the diff easier to read, which I'll file as a dependent bug on this one, but for the most part these changes have to land atomically, I'm afraid.
Comment 5 WebKit Review Bot 2011-07-28 19:55:15 PDT
Comment on attachment 102323 [details]
Patch

Attachment 102323 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/9269278
Comment 6 WebKit Review Bot 2011-07-28 20:00:16 PDT
Comment on attachment 102323 [details]
Patch

Attachment 102323 [details] did not pass cr-mac-ews (chromium):
Output: http://queues.webkit.org/results/9266366
Comment 7 James Robinson 2011-07-29 10:09:51 PDT
Created attachment 102372 [details]
compile fixes
Comment 8 James Robinson 2011-07-29 18:33:36 PDT
Created attachment 102417 [details]
Patch
Comment 9 James Robinson 2011-07-29 18:33:54 PDT
With http://trac.webkit.org/changeset/92037 landed this patch is still large, but a bit more manageable.
Comment 10 Vangelis Kokkevis 2011-08-01 10:54:13 PDT
Comment on attachment 102417 [details]
Patch

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

Looks good!  I suppose additional work will need to be done to deal with the texture manager used by CCRenderSurface?

> Source/WebCore/ChangeLog:408
> +2011-07-29  James Robinson  <jamesr@chromium.org>

Needs to be removed

> Source/WebCore/platform/graphics/chromium/LayerChromium.cpp:410
> +bool LayerChromium::descendantsDrawsContent()

nit: descendantDrawsContent() ?

> Source/WebCore/platform/graphics/chromium/LayerChromium.h:-42
> -#include "RenderSurfaceChromium.h"

Why change the order here?  Headers were alphabetically sorted.  If the need to be rearranged then there are hidden dependencies that should be eliminated.

> Source/WebCore/platform/graphics/chromium/cc/CCRenderSurface.cpp:2
> + * Copyright (C) 2010 Google Inc. All rights reserved.

2011

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

cc/CCRenderSurface.h  ?
Comment 11 James Robinson 2011-08-01 13:57:36 PDT
(In reply to comment #10)
> (From update of attachment 102417 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=102417&action=review
> 
> Looks good!  I suppose additional work will need to be done to deal with the texture manager used by CCRenderSurface?

Yes, there's some more work needed here - https://bugs.webkit.org/show_bug.cgi?id=64772 is the next piece of that puzzle.

> 
> > Source/WebCore/ChangeLog:408
> > +2011-07-29  James Robinson  <jamesr@chromium.org>
> 
> Needs to be removed

fixed

> 
> > Source/WebCore/platform/graphics/chromium/LayerChromium.cpp:410
> > +bool LayerChromium::descendantsDrawsContent()
> 
> nit: descendantDrawsContent() ?

good idea, fixed

> 
> > Source/WebCore/platform/graphics/chromium/LayerChromium.h:-42
> > -#include "RenderSurfaceChromium.h"
> 
> Why change the order here?  Headers were alphabetically sorted.  If the need to be rearranged then there are hidden dependencies that should be eliminated.

resorted

> 
> > Source/WebCore/platform/graphics/chromium/cc/CCRenderSurface.cpp:2
> > + * Copyright (C) 2010 Google Inc. All rights reserved.
> 
> 2011

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

done
Comment 12 James Robinson 2011-08-01 15:48:36 PDT
Created attachment 102579 [details]
Patch
Comment 13 Vangelis Kokkevis 2011-08-01 16:30:14 PDT
Comment on attachment 102579 [details]
Patch

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

Couple more small things but otherwise and unofficial LGTM from me.

> Source/WebCore/platform/graphics/chromium/LayerChromium.cpp:66
> +    , m_debugBorderWidth(0)

I don't see m_debugBorderWidth used here.

> Source/WebCore/platform/graphics/chromium/cc/CCRenderSurface.h:2
> + * Copyright (C) 2010 Google Inc. All rights reserved.

2011
Comment 14 James Robinson 2011-08-01 16:59:12 PDT
Created attachment 102592 [details]
fix copyright year
Comment 15 James Robinson 2011-08-01 18:26:50 PDT
(In reply to comment #13)
> (From update of attachment 102579 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=102579&action=review
> 
> Couple more small things but otherwise and unofficial LGTM from me.
> 
> > Source/WebCore/platform/graphics/chromium/LayerChromium.cpp:66
> > +    , m_debugBorderWidth(0)
> 
> I don't see m_debugBorderWidth used here.
> 

It's used in pushPropertiesTo() and later used on the CC side of life when drawing layers.
Comment 16 Kenneth Russell 2011-08-02 14:34:44 PDT
Comment on attachment 102592 [details]
fix copyright year

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

Overall this looks like good progress. A couple of minor comments. r=me

> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.h:245
>      OwnPtr<CCCanvasLayerImpl::Program> m_canvasLayerProgram;
> +    OwnPtr<CCPluginLayerImpl::Program> m_pluginLayerProgram;
> +    OwnPtr<CCRenderSurface::MaskProgram> m_renderSurfaceMaskProgram;
> +    OwnPtr<CCRenderSurface::Program> m_renderSurfaceProgram;
>      OwnPtr<CCVideoLayerImpl::RGBAProgram> m_videoLayerRGBAProgram;
>      OwnPtr<CCVideoLayerImpl::YUVProgram> m_videoLayerYUVProgram;

It seems to me that these references are abstraction violations. The LayerRendererChromium shouldn't reference these programs directly. Is there a plan to move these entirely into the cc/ package? If so, is it worth a FIXME?

> Source/WebCore/platform/graphics/chromium/cc/CCRenderSurface.h:107
> +    OwnPtr<LayerTexture> m_contentsTexture;

Is this reference an abstraction violation? It seems to me that the compositor's classes should use those in cc/ but not the other way around.
Comment 17 James Robinson 2011-08-02 15:38:29 PDT
You are correct on both points.  Nat is working on splitting up LayerRendererChromium into (at least) two halves, part of that will involve moving the program management into a cc/ class.  For now, though, there isn't a good cc/ class to hold the programs on.

The LayerTexture dependency is also a bit wonky because we are using the TextureManager concept for both content textures (which aren't currently a cc/ concept) and for render surfaces (which are).  It's going to take a bit more work over the next few weeks to fully clean this up.
Comment 18 WebKit Review Bot 2011-08-02 16:20:58 PDT
Comment on attachment 102592 [details]
fix copyright year

Clearing flags on attachment: 102592

Committed r92245: <http://trac.webkit.org/changeset/92245>
Comment 19 WebKit Review Bot 2011-08-02 16:21:04 PDT
All reviewed patches have been landed.  Closing bug.