Bug 58834 – [chromium] Draw the root/"non-composited content" in compositor side

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

[James Robinson]

Currently the root content (what the WK2 compositor calls the non-composited content) is special-cased by LayerRendererChromium and rendered differently from how *LayerChromium or CCLayerImpls are rendered.  We need to somehow render this from the compositor side.

I currently think that the easiest way to do this will be to create a CCLayerImpl of the appropriate type (probably the same type as ContentLayerChromiums have) and during the tree syncing phase set it up at the appropriate point.  From the compositor thread side the "root layer" content can be treated the same way as any other layer, IMO.  This will be clearer when the other issues blocking bug 58799 are resolved.

[James Robinson]

Created attachment 102327 [details]
work in progress, scrolling and offscreen compositing don't work

[Adrienne Walker]

Is it too complicated (or too at odds with WK2) to make a graphics layer for the non-composited content and stick that in the graphics layer tree? It just seems like it shouldn't be special on either end.

[James Robinson]

(In reply to comment #2)
> Is it too complicated (or too at odds with WK2) to make a graphics layer for the non-composited content and stick that in the graphics layer tree? It just seems like it shouldn't be special on either end.

That's what this does - NonCompositedContentHost has a GraphicsLayer that is stuck into the tree at the appropriate place. NonCompositedContentHost is that layer's GraphicsLayerClient so it can route paint calls out through the appropriate paint interface.

[Adrienne Walker]

(In reply to comment #3)
> (In reply to comment #2)
> > Is it too complicated (or too at odds with WK2) to make a graphics layer for the non-composited content and stick that in the graphics layer tree? It just seems like it shouldn't be special on either end.
> 
> That's what this does - NonCompositedContentHost has a GraphicsLayer that is stuck into the tree at the appropriate place. NonCompositedContentHost is that layer's GraphicsLayerClient so it can route paint calls out through the appropriate paint interface.

I think I had a misunderstanding and thought this would be simpler on the main thread side.  Sorry for my confusion.

This does nicely clean up a lot of LayerRendererChromium.  :)

[James Robinson]

Created attachment 103728 [details]
Patch

[James Robinson]

Scrolling still doesn't work.  I'm not sure why - as far as I can tell, scrolling a layer is just changing the relationship between layer space and content space.  I'm calling LayerTilerChromium::setLayerPosition(-scrollPosition()) at what I think is the appropriate time, but no dice.  It sort of works when testing manually until I scroll too far then it all goes to hell.

Looking at the code I don't think we've ever called LayerTilerChromium::setLayerPosition(), so maybe that just doesn't work?  Not sure.

[Adrienne Walker]

(In reply to comment #6)
> Scrolling still doesn't work.  I'm not sure why - as far as I can tell, scrolling a layer is just changing the relationship between layer space and content space.  I'm calling LayerTilerChromium::setLayerPosition(-scrollPosition()) at what I think is the appropriate time, but no dice.  It sort of works when testing manually until I scroll too far then it all goes to hell.
> 
> Looking at the code I don't think we've ever called LayerTilerChromium::setLayerPosition(), so maybe that just doesn't work?  Not sure.

setLayerPosition used to be called for scrollbars.  I'm going to change it to setLayerTransform for RTL layers, so I haven't removed it yet.

I'm pretty sure setLayerPosition affects both paint and draw.  If you look at the code you're changing, the scroll is only passed during the draw step.  The root layer here is unfortunately special.  Maybe you could take the scroll out during the WebViewImplContentPainter step so that we can be consistent?

[Adrienne Walker]

(In reply to comment #7)

> setLayerPosition used to be called for scrollbars.  I'm going to change it to setLayerTransform for RTL layers, so I haven't removed it yet.

Also, given that nothing currently uses it, it's likely that it doesn't work.  I'm pretty sure there's at least one place in the AA code where (what the tiler calls) layer space and content space are confused.

[James Robinson]

Created attachment 103997 [details]
Patch

[James Robinson]

All compositing layout tests pass on this version except for 
compositing/reflections/nested-reflection-mask-change.html (lawl), where the mask isn't being applied.  I'll look into that tomorrow.

The scroll offset itself is used:
- to manipulate the draw transform by modifying TiledLayerChromium::tilingTransform
- at paint time by moving visibleLayerRect in paintLayerContents()

does that sound like it should be sufficient? It seems to work.

I also added a bit on layers indicating whether they should get the root layer special treatment, which currently means forcing subpixel text rendering to on (which does not have any impact on DumpRenderTree, since subpixel is always off for all text in DRT) and masking the alpha channel at draw time which according to comments is necessary on windows.

I also had to remove a clip in ContentLayerChromium::paintContentsIfDirty, because otherwise it was throwing away all invalidations outside the content's initial viewable bounds.  I could move this rect by the scroll offset but looking at the code it seems wrong to disregard dirties outside the bounds.  Given that the current code seems to work correctly there must be something I'm missing here :).  Otherwise this is all shuffling things about.

[James Robinson]

Oh, and a few tests have some very small rendering differences that I can't fully explain.  A lot of these are the corners of scrollbars, which makes me hope that I've somehow magically fixed the debug/release linux scrollbar rendering differences.  I haven't confirmed that yet.

[Adrienne Walker]

Comment on attachment 103997 [details]
Patch

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

> Source/WebCore/platform/graphics/chromium/ContentLayerChromium.cpp:-106
> -    dirty.intersect(IntRect(IntPoint(), contentBounds()));

I still don't understand why this line needs to be cut.  Does that mean that invalidations might also need to be adjusted by the scroll offset?

I added this line in the past because sometimes invalidations were outside the bounds of the layer's contents.  Because the tiler has no explicit resize (which is probably a mistake), it just grows to incorporate the size of the invalidations passed to it.  This was causing layers to actually become larger than they should be and drawing incorrectly.

> Source/WebCore/platform/graphics/chromium/LayerTextureUpdater.h:63
> +    virtual void prepareToUpdate(const IntRect& contentRect, const IntSize& tileSize, bool isRootLayer) = 0;

Nit: "isRootLayer" seems weird to pass into prepareToUpdate as a function argument.  Subpixel AA seems more like a property on the tiler, like border texels.  And also one that, when changed, should reset the tiler.  I realize that the root layer won't change, but in the future we might want to speculatively enable subpixel AA on more layers if we know they're opaque and their screen space transforms are kosher.  I suppose I could also just change that later when adding that functionality too.

[James Robinson]

Created attachment 104110 [details]
Patch

[James Robinson]

Created attachment 104113 [details]
Patch

[James Robinson]

This patch works fine as far I can tell - the compositing/ layout tests pass and manual testing things seem to work fine (scrolling works, the expected things have subpixel AA, subframe scrolling works, etc).

One bit that I'm really unsure about is the change to the dirty rect clipping in ContentLayerChromium::paintContentsIfDirty().  Previously the code clipped m_dirtyRect to the rect ((0, 0), contentBounds()), which makes sense.  I had to change that to (scrollPosition(), contentBounds()) or there was a lot of blue whenever the root layer was scrolled slightly.  This showed up in several layout tests that scroll (most that test fixed positioning) and in manual testing.  This feels wrong because I think that contentBounds() should cover the entire content bounds of the layer, not just the currently visible portion.  Do you happen to know what is going on, Adrienne?  Given that this patch works and this area seems pretty well tested I'm OK with landing this, but it seems like something somewhere is confused (possibly just me).

[James Robinson]

Aha, the issue was that the non composited content's layer size was set to the viewport size, not the contents size.  With that fixed the original clipping logic works correctly.  I think we're good to go now!

[Adrienne Walker]

Comment on attachment 104113 [details]
Patch

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

> Source/WebCore/platform/graphics/chromium/LayerTilerChromium.cpp:252
> -void LayerTilerChromium::prepareToUpdate(const IntRect& contentRect, LayerTextureUpdater* textureUpdater)
> +void LayerTilerChromium::prepareToUpdate(const IntRect& layerRect, LayerTextureUpdater* textureUpdater)

Nit: can you also remove this transformation? I think this is a terminology mismatch.  contentRect (in the tiler sense) is right here and there's no need to also have the line in this function to transform from layer to content space.  I should just rename everything in the tiler from content space => layer space and layer space => tile space.

[James Robinson]

(In reply to comment #17)
> (From update of attachment 104113 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=104113&action=review
> 
> > Source/WebCore/platform/graphics/chromium/LayerTilerChromium.cpp:252
> > -void LayerTilerChromium::prepareToUpdate(const IntRect& contentRect, LayerTextureUpdater* textureUpdater)
> > +void LayerTilerChromium::prepareToUpdate(const IntRect& layerRect, LayerTextureUpdater* textureUpdater)
> 
> Nit: can you also remove this transformation? I think this is a terminology mismatch.  contentRect (in the tiler sense) is right here and there's no need to also have the line in this function to transform from layer to content space.  I should just rename everything in the tiler from content space => layer space and layer space => tile space.

Definitely, I'll revert this back.  I made this change when I was more confused about content vs layer space in LayerTilerChromium.

[James Robinson]

Created attachment 104215 [details]
Patch

[Adrienne Walker]

Comment on attachment 104215 [details]
Patch

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

In general, this looks good to me.  I have just one small question and a nit.

The scrollbars changing is really unexpected to me.  Is this your local DRT output, or are the scrollbars really changing on the bots as well?

> Source/WebCore/platform/graphics/chromium/NonCompositedContentHost.cpp:74
> +    // ???

Nit: I think it'd be better to sort out if anything is needed here and leave more informative comments.

[James Robinson]

I don't know how to explain the scrollbar diffs, but they are reliably rendering differently on my box with this patch compared to without. http://knowyourmeme.com/i/3308/original/xjobz5hofa.jpg?1244616067

[James Robinson]

Created attachment 104228 [details]
get rid of ???s in GraphicsLayerClient implementations

[Adrienne Walker]

Unofficially, LGTM.

[James Robinson]

Can I get an official review for this, please?

[WebKit Review Bot]

Comment on attachment 104228 [details]
get rid of ???s in GraphicsLayerClient implementations

Clearing flags on attachment: 104228

Committed r93329: <http://trac.webkit.org/changeset/93329>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[James Robinson]

Created attachment 104405 [details]
now with less blue on windows

[James Robinson]

Patch updated to disable blending when drawing the root layer.  This is necessary on windows because GDI puts garbage in the alpha channel, so in the previous version of the patch all the text on the root layer was ending up blue (from the fullscreen clear to blue at the start of the composite).

There are still some weird scrollbar diffs, but with this patch for some reason most of the release vs debug differences go away (everything except for compositing/geometry/horizontal-scroll-composited.html).  I still don't fully grok this but it makes me happy!

[James Robinson]

Here's the code diff to the previous version:


diff --git a/Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp b/Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp
index 9044997..a7e232c 100644
--- a/Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp
+++ b/Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp
@@ -751,9 +751,6 @@ void LayerRendererChromium::drawLayersInternal()
     GLC(m_context.get(), m_context->disable(GraphicsContext3D::DEPTH_TEST));
     GLC(m_context.get(), m_context->disable(GraphicsContext3D::CULL_FACE));
 
-    // Blending disabled by default. Root layer alpha channel on Windows is incorrect when Skia uses ClearType.
-    GLC(m_context.get(), m_context->disable(GraphicsContext3D::BLEND));
-
     useRenderSurface(m_defaultRenderSurface);
 
     // Clear to blue to make it easier to spot unrendered regions.
diff --git a/Source/WebCore/platform/graphics/chromium/cc/CCTiledLayerImpl.cpp b/Source/WebCore/platform/graphics/chromium/cc/CCTiledLayerImpl.cpp
index 637209b..d217e94 100644
--- a/Source/WebCore/platform/graphics/chromium/cc/CCTiledLayerImpl.cpp
+++ b/Source/WebCore/platform/graphics/chromium/cc/CCTiledLayerImpl.cpp
@@ -53,14 +53,18 @@ void CCTiledLayerImpl::draw()
     const IntRect& layerRect = visibleLayerRect();
     if (!layerRect.isEmpty()) {
         GraphicsContext3D* context = layerRenderer()->context();
-        // Mask out writes to the alpha channel for the root layer, subpixel antialiasing
-        // via Skia results in invalid zero alpha values on text glyphs. The root layer
-        // is always opaque so the alpha channel isn't meaningful anyway.
-        if (isRootLayer())
-            context->colorMask(true, true, true, false);
+        // Mask out writes to the alpha channel and disable blending for the root layer,
+        // subpixel antialiasing on windows results in invalid zero alpha values on text
+        // glyphs. The root layer is always opaque so the alpha channel isn't meaningful anyway.
+        if (isRootLayer()) {
+            GLC(context, context->colorMask(true, true, true, false));
+            GLC(context, context->disable(GraphicsContext3D::BLEND));
+        }
         m_tiler->draw(layerRect, m_tilingTransform, drawOpacity());
-        if (isRootLayer())
+        if (isRootLayer()) {
             context->colorMask(true, true, true, true);
+            GLC(context, context->enable(GraphicsContext3D::BLEND));
+        }
     }
 }

[Adrienne Walker]

Those code changes look good to me, so long as the tests are passing now.

I still don't fully understand the minor scrollbar issues either, but not having debug vs. release issues seems like a positive change.

[Kenneth Russell]

Comment on attachment 104405 [details]
now with less blue on windows

r=me based on enne's unofficial review.

[James Robinson]

Thanks, Ken!

[WebKit Review Bot]

Comment on attachment 104405 [details]
now with less blue on windows

Clearing flags on attachment: 104405

Committed r93360: <http://trac.webkit.org/changeset/93360>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.