Bug 88908

Created attachment 147178 [details] Patch

Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

Attachment 147178 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/Platform/ChangeLog', u'Source/Platf..." exit_code: 1 Source/WebCore/ChangeLog:12: You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible. [changelog/nonewtests] [5] Total errors found: 1 in 11 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 147178 [details] Patch I'll take the first look to save reviewers' time, removing the r=? flag for now. =)

Comment on attachment 147178 [details] Patch In addition to comments below, let's add a layout test that reproduces the problem and gets fixed by this patch. You can probably do this by assimilating the test case that we were given originally, and making it in the style of a webkit layout test. View in context: https://bugs.webkit.org/attachment.cgi?id=147178&action=review > Source/Platform/ChangeLog:15 > + * chromium/public/WebContentLayer.h: > + (WebContentLayer): > + * chromium/public/WebLayer.h: > + (WebLayer): I wonder if its possible to take different approach that keeps this fix contained to graphicsLayerChromium? I'm not sure it would work, but we should try the following: - find a solution that doesn't require setIsContentsLayer - don't remove setIsDoubleSided from WebContentsLayer - instead, add the complexity to GraphicsLayerChromium::setBackfaceVisibility(). > Source/Platform/chromium/public/WebContentLayer.h:-58 > - WEBKIT_EXPORT void setDoubleSided(bool); I'm guessing we shouldn't remove this. Did you remove it thinking that this corresponded to GraphicsLayerChromium::m_contentsLayer? looking at the code, its actually backwards - GraphicsLayerChromium:m_layer is actually a WebContentsLayer, and GraphicsLayerChromium::m_contentsLayer is actually a WebLayer. (!!!!) I'm not sure if the naming should change or not, we should ask jamesr about that. All the more reason to keep the fix within GraphicsLayerChromium if that works. > Source/WebCore/platform/graphics/chromium/GraphicsLayerChromium.cpp:822 > + m_contentsLayer.setDoubleSided(m_layer.doubleSided()); In addition to placing it here, I think adding this logic to GraphicsLayerChromium::setBackfaceVisibility might be the solution we're looking for. Note however, I don't think its as simple as setting it to m_layer.doubleSided(). You can see if it passes all layout tests, and if it doesnt, we'll probably have to visit the W3C spec to understand how to set m_contentsLayer.setDoubleSided(...) correctly. > Source/WebCore/platform/graphics/chromium/LayerChromium.h:346 > + bool m_isContentsLayer; isContentsLayer might be a bit vague by itself. Hopefully we won't even need this flag, if GraphicsLayerChromium can do the correct W3C logic on its own to decide how to set doubleSided. but if we need this, perhaps we should rename it to "m_isContentsLayerOfGraphicsLayer"

(In reply to comment #5) > (From update of attachment 147178 [details]) > > In addition to comments below, let's add a layout test that reproduces the problem and gets fixed by this patch. You can probably do this by assimilating the test case that we were given originally, and making it in the style of a webkit layout test. > > View in context: https://bugs.webkit.org/attachment.cgi?id=147178&action=review > > > Source/Platform/ChangeLog:15 > > + * chromium/public/WebContentLayer.h: > > + (WebContentLayer): > > + * chromium/public/WebLayer.h: > > + (WebLayer): > > I wonder if its possible to take different approach that keeps this fix contained to graphicsLayerChromium? I'm not sure it would work, but we should try the following: > - find a solution that doesn't require setIsContentsLayer > - don't remove setIsDoubleSided from WebContentsLayer > - instead, add the complexity to GraphicsLayerChromium::setBackfaceVisibility(). I like this idea a lot. While you're doing that, could you construct test cases for other content layer types (canvas 2d, webgl, etc) and see if they suffer from the same issue? > > > Source/Platform/chromium/public/WebContentLayer.h:-58 > > - WEBKIT_EXPORT void setDoubleSided(bool); > > I'm guessing we shouldn't remove this. Did you remove it thinking that this corresponded to GraphicsLayerChromium::m_contentsLayer? looking at the code, its actually backwards - GraphicsLayerChromium:m_layer is actually a WebContentsLayer, and GraphicsLayerChromium::m_contentsLayer is actually a WebLayer. (!!!!) I'm not sure if the naming should change or not, we should ask jamesr about that. All the more reason to keep the fix within GraphicsLayerChromium if that works. Terminology inversion here. In GraphicsLayer terms, m_layer = the layer representing the GraphicsLayerClient's painted stuff. m_contentsLayer = the special stuff inside the layer (video, webgl, whatever). In the WebLayer or LayerChromium hierarchy, we use a different terminology. *ContentLayer* means a layer type that draws via a paintContents() call. WebLayer/LayerChromium are the base class representing any time of layer. Thus, GraphicsLayer::m_layer is a *ContentLayer* layer because it renders with a paint() call. GraphicLayer::m_contentLayer is just of the base class type since we don't know which concrete type it is (WebGL, video, whatever). I agree it sucks but I'm not completely sure what to do about it. We could rename GraphicsLayerChromium's members, but we want them to be somewhat consistent with the cross-platform GraphicsLayer interface.

(In reply to comment #6) > (In reply to comment #5) > > let's add a layout test that reproduces the problem and gets fixed by this patch. > > While you're doing that, could you construct test cases for other content layer types (canvas 2d, webgl, etc) and see if they suffer from the same issue? Working on that now. Btw, this does indeed reproduce with WebGL (and I assume it will reproduce with canvas2D) > > I wonder if its possible to take different approach that keeps this fix contained to graphicsLayerChromium? > > I like this idea a lot. I'll try to find an alternative plumbing arrangement. > > > Source/Platform/chromium/public/WebContentLayer.h:-58 > > > - WEBKIT_EXPORT void setDoubleSided(bool); > > > > I'm guessing we shouldn't remove this. > > Terminology inversion here. > > In the WebLayer or LayerChromium hierarchy, we use a different terminology. *ContentLayer* means a layer type that draws via a paintContents() call. I'll need to understand these structures and their target forms a bit better. In its current form, WebContentLayer is hard to distinguish from WebLayer. I'll discuss more offline.

(In reply to comment #7) > (In reply to comment #6) > > (In reply to comment #5) > > > > let's add a layout test that reproduces the problem and gets fixed by this patch. > > > > While you're doing that, could you construct test cases for other content layer types (canvas 2d, webgl, etc) and see if they suffer from the same issue? > > Working on that now. Btw, this does indeed reproduce with WebGL (and I assume it will reproduce with canvas2D) Sounds quite likely - that implies doing a general fix for GraphicsLayerChromium::m_contentsLayer is likely to be what we want. > > > > Source/Platform/chromium/public/WebContentLayer.h:-58 > > > > - WEBKIT_EXPORT void setDoubleSided(bool); > > > > > > I'm guessing we shouldn't remove this. > > > > Terminology inversion here. > > > > In the WebLayer or LayerChromium hierarchy, we use a different terminology. *ContentLayer* means a layer type that draws via a paintContents() call. > > I'll need to understand these structures and their target forms a bit better. In its current form, WebContentLayer is hard to distinguish from WebLayer. I'll discuss more offline. That's putting it mildly :). Feel free to grab me and try to brainstorm an improvement.

(In reply to comment #8) > Feel free to grab me and try to brainstorm an improvement. Thanks! Fyi, it looks like canvas 2d isn't impacted by this bug. WebGL is impacted by the bug, and the patch I attached doesn't fix the issue for WebGL. Also, the patch doesn't seem to work on video unless preserve-3d is set for the element as well. I'll sort out these issues and then interrogate you about mechanisms for reorganizing the fix.

(In reply to comment #9) > (In reply to comment #8) > > Feel free to grab me and try to brainstorm an improvement. > > Thanks! > > Fyi, it looks like canvas 2d isn't impacted by this bug. WebGL is impacted by the bug, and the patch I attached doesn't fix the issue for WebGL. Also, the patch doesn't seem to work on video unless preserve-3d is set for the element as well. > > I'll sort out these issues and then interrogate you about mechanisms for reorganizing the fix. Some possibilities: - canvas 2d might not actually use a separate m_layer and m_contentsLayer ? - by fluke, maybe the test cases you set up are not triggering accelerated compositing? (I think its forced on these days, though, so probably this isn't it) you can double check by using --show-fps-counter or setting that option in about:flags. the fps counter only shows up when chromium triggers accelerated compositing mode

(In reply to comment #10) > Some possibilities: > - canvas 2d might not actually use a separate m_layer and m_contentsLayer ? > - by fluke, maybe the test cases you set up are not triggering accelerated compositing? (I think its forced on these days, though, so probably this isn't it) you can double check by using --show-fps-counter or setting that option in about:flags. the fps counter only shows up when chromium triggers accelerated compositing mode There's a min size for a canvas to be accelerated right? Might be that as well.

(In reply to comment #11) > (In reply to comment #10) > > Some possibilities: > > - canvas 2d might not actually use a separate m_layer and m_contentsLayer ? > > - by fluke, maybe the test cases you set up are not triggering accelerated compositing? > > There's a min size for a canvas to be accelerated right? Might be that as well. Indeed in my particular example the canvas 2d doesn't create a separate m_contentsLayer (even with compositing forced). It so-happened that with the WebGL example I created, the snapshot of the backface-visibility that the contents layer did happened before the parent was initialized to the value specified in the HTML. In principle though, any dynamic change of the backface-visibility would have doomed the patch. A patch which has the contents layer reach into its parent layer to determine its visibility produces the correct results and is robust to dynamic changes. I'll upload once I've cleaned it up (then we can torch it for structural reasons).

(In reply to comment #12) > (In reply to comment #11) > > (In reply to comment #10) > > > Some possibilities: > > > - canvas 2d might not actually use a separate m_layer and m_contentsLayer ? > > > - by fluke, maybe the test cases you set up are not triggering accelerated compositing? > > > > There's a min size for a canvas to be accelerated right? Might be that as well. > > Indeed in my particular example the canvas 2d doesn't create a separate m_contentsLayer (even with compositing forced). Accelerated 2d canvas is blacklisted in chrome on linux (poor driver quality), so that might explain it if you're testing on linux. Check about:gpu to see what you are getting (and if you are brave, try --ignore-gpu-blacklist) > > It so-happened that with the WebGL example I created, the snapshot of the backface-visibility that the contents layer did happened before the parent was initialized to the value specified in the HTML. In principle though, any dynamic change of the backface-visibility would have doomed the patch. > > A patch which has the contents layer reach into its parent layer to determine its visibility produces the correct results and is robust to dynamic changes. I'll upload once I've cleaned it up (then we can torch it for structural reasons).

Created attachment 147459 [details] Patch

Comment on attachment 147459 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=147459&action=review > Source/WebCore/platform/graphics/chromium/LayerChromium.h:178 > + void setIsContentsLayer(bool isContentsLayer) { m_isContentsLayer = isContentsLayer; } > + bool isContentsLayer() const { return m_isContentsLayer; } At first glance I'm not a big fan of adding these to LayerChromium and beyond - it is a quirk of GraphicsLayerChromium that the doubleSided property applies to both a layer and its immediate child in some cases, GraphicsLayerChromium should take care of just setting the bits (like doubleSided) on the appropriate LayerChromiums. The compositor itself doesn't need to know about the concept of "contents layer" - it's way too confusing given the other naming collisions.

(In reply to comment #15) > (From update of attachment 147459 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=147459&action=review > > > Source/WebCore/platform/graphics/chromium/LayerChromium.h:178 > > + void setIsContentsLayer(bool isContentsLayer) { m_isContentsLayer = isContentsLayer; } > > + bool isContentsLayer() const { return m_isContentsLayer; } > > At first glance I'm not a big fan of adding these to LayerChromium and beyond - it is a quirk of GraphicsLayerChromium that the doubleSided property applies to both a layer and its immediate child in some cases, GraphicsLayerChromium should take care of just setting the bits (like doubleSided) on the appropriate LayerChromiums. The compositor itself doesn't need to know about the concept of "contents layer" The concern I have with that approach is that in order to get the correct results we must mark the parent as having preserve-3d as well (because the culling is done with respect to the transformation, which is applied to the parent). This seems to break correctness because other children may be counting on preserve-3d not being set. > it's way too confusing given the other naming collisions. Another alternative would be to have a name the variable/method to express it does, "I will use my parent's backface culling properties" (m_useParentCullingLogic) instead of expressing when it is used "I'm a contents layer" (m_isContentsLayer).

I've gone ahead and add a uploaded a patch which adds a useParentBackfaceVisibility flag to WebCore::LayerChromium, WebKit::WebLayer, and WebCore::CCLayerImpl. Capturing some offline discussion with Shawn and James: This may not be the right long-term direction. Ideally the WebKit::WebLayer compositor interface should be able to express this "use my parent's backface visibility calculation" concept with adding this new interface. We need to do two things for this. The first thing is that we need to propagate the backface-visibility property from the parent layer to the content layer (we can do this already). The second thing that we need to do is to specify that the parent layer should have preserve-3d... BUT, only for the contents layer -- if there are any other child layers, they should not have preserve-3d set. And this is where things break down with the current API. To implement this feature against the WebKit::WebLayer compositor API, we would need to change its idea of preserve-3d to be a property that a child specifies regarding its relationship to its parent, rather than being a property that a parent specifies regarding its relationship to all of its children. This is a substantial change in terms of diff surface area, so we're punting on it for the moment.

Created attachment 147668 [details] Patch

The next piece of the puzzle is tests - we require that all fixes have regression tests. In this instance, I think there are a few tests that we could/should write: 1.) Write one or more unit tests that this property has the proper behavior in our backface testing code. See Source/WebKit/chromium/tests/CCLayerTreeHostCommonTest.cpp's current backface tests to see how these work. 2.) Write some layout tests to verify that the CSS properties have the desired effect on rendering through the entire stack. See LayoutTests/compositing/backface-visibility/ for an example of how these work. I'd like to see at least one test for <video>, WebGL, canvas, and composited images to make sure we're doing the right thing for all of these cases and to make sure that our behavior is consistent with other ports (like Safari).

Comment on attachment 147668 [details] Patch Few comments in addition to the testing todo =) View in context: https://bugs.webkit.org/attachment.cgi?id=147668&action=review > Source/Platform/chromium/public/WebLayer.h:112 > + // Set whether this layer is a content layer (e.g, canvas, plugin, WebGL, or video) probably should update this comment > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:250 > + layer = layer->parent(); I feel uncomfortable doing this, what if someone blindly adds another condition for skpping the layer below this poitn in code? Perhaps its better to make the two cases more explicit without changing any state (i.e. which layer is being considered) within this function.

Created attachment 147675 [details] Patch

(In reply to comment #20) > (From update of attachment 147668 [details]) > Few comments in addition to the testing todo =) > > > View in context: https://bugs.webkit.org/attachment.cgi?id=147668&action=review > > > Source/Platform/chromium/public/WebLayer.h:112 > > + // Set whether this layer is a content layer (e.g, canvas, plugin, WebGL, or video) > > probably should update this comment > > > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:250 > > + layer = layer->parent(); > > I feel uncomfortable doing this, what if someone blindly adds another condition for skpping the layer below this poitn in code? Perhaps its better to make the two cases more explicit without changing any state (i.e. which layer is being considered) within this function. Good point, that's pretty sloppy. I've uploaded a new patch with an explicit variable for the layer being tested for backface culling -- does this seem better? + LayerType* backfaceTestLayer = layer; + if (layer->useParentBackfaceVisibility()) { + ASSERT(layer->parent()); + backfaceTestLayer = layer->parent(); + }

Comment on attachment 147675 [details] Patch Yeah, much better, thanks! I think its probably appropriate to remove the review? flag for now, and we can ask reviewers to look at it when the patch includes a good battery of tests.

Created attachment 148471 [details] Patch

Comment on attachment 148471 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=148471&action=review (You can't set R+ yourself. The webkit-review bot will come by and complain.) The image test looks good to me. Are there tests for video/canvas/webgl too? I realize that Chromium's implementation sends all these down the same setupContents path, but the tests will be run by other ports and they might behave differently. If you haven't yet, I'll run this patch through Safari and see if this test should be marked as failing for them, since I know there's some lingering backface visibility bugs that Shawn filed earlier. > Source/Platform/chromium/public/WebLayer.h:112 > + // Set whether this layer is a content layer (e.g, canvas, plugin, WebGL, or video) nit: I think this comment is talking too much about the implementation details of GraphicsLayerChromium and its backface visibility behavior. There are other consumers of WebLayer (e.g. ash), so maybe it'd be better to not have a comment at all; I think the function is well-named enough to describe exactly what it does. > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:252 > + if (layer->useParentBackfaceVisibility()) { > + ASSERT(layer->parent()); > + backfaceTestLayer = layer->parent(); > + } This is kind of an edge case, but what happens if a child and its parent both useParentBackfaceVisibility()?

(In reply to comment #25) > (From update of attachment 148471 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=148471&action=review > > The image test looks good to me. Are there tests for video/canvas/webgl too? I realize that Chromium's implementation sends all these down the same setupContents path, but the tests will be run by other ports and they might behave differently. I have WebGL and canvas tests which I can add. I'm still working on video tests. It was suggested that I just add the one test -- I'll discuss which to add in a bit. > If you haven't yet, I'll run this patch through Safari and see if this test should be marked as failing for them, since I know there's some lingering backface visibility bugs that Shawn filed earlier. Thanks! No, haven't done that yet. > > Source/Platform/chromium/public/WebLayer.h:112 > > + // Set whether this layer is a content layer (e.g, canvas, plugin, WebGL, or video) > > nit: I think this comment is talking too much about the implementation details of GraphicsLayerChromium and its backface visibility behavior. There are other consumers of WebLayer (e.g. ash), so maybe it'd be better to not have a comment at all; I think the function is well-named enough to describe exactly what it does. Oops, that comment stuck around from an earlier naming of the variable. I'll update it to say something more relevant. That said, I'd like to draw attention in the comment that this is an idiosyncratic API call which exists to support a particular application issue. If ash wants to do something that uses this functionality, we should change the preserve-3d property to be per-child (since the per-parent issue mirrors the somewhat-restricted behavior allowed by the WebKit APIs). > This is kind of an edge case, but what happens if a child and its parent both useParentBackfaceVisibility()? I don't think that WebKit will generate such a situation (since I don't think a m_contents layer can have an m_contents layer itself). I'll add an assertion to this effect. It is an argument in favor of implementing the per-child solution (in which case this property vanishes).

(In reply to comment #26) > (In reply to comment #25) > > (From update of attachment 148471 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=148471&action=review > > > > The image test looks good to me. Are there tests for video/canvas/webgl too? I realize that Chromium's implementation sends all these down the same setupContents path, but the tests will be run by other ports and they might behave differently. > > I have WebGL and canvas tests which I can add. I'm still working on video tests. It was suggested that I just add the one test -- I'll discuss which to add in a bit. If you have other tests you'd have and can add, that'd be great. If you just have the image test, I guess that seems fine to commit too. Video tests have been flaky in the past, so an image test would probably be more reliable. > > If you haven't yet, I'll run this patch through Safari and see if this test should be marked as failing for them, since I know there's some lingering backface visibility bugs that Shawn filed earlier. > > Thanks! No, haven't done that yet. Looks like it runs just fine. > > > Source/Platform/chromium/public/WebLayer.h:112 > > > + // Set whether this layer is a content layer (e.g, canvas, plugin, WebGL, or video) > > > > nit: I think this comment is talking too much about the implementation details of GraphicsLayerChromium and its backface visibility behavior. There are other consumers of WebLayer (e.g. ash), so maybe it'd be better to not have a comment at all; I think the function is well-named enough to describe exactly what it does. > > Oops, that comment stuck around from an earlier naming of the variable. I'll update it to say something more relevant. > > That said, I'd like to draw attention in the comment that this is an idiosyncratic API call which exists to support a particular application issue. If ash wants to do something that uses this functionality, we should change the preserve-3d property to be per-child (since the per-parent issue mirrors the somewhat-restricted behavior allowed by the WebKit APIs). Yeah, I totally agree. Maybe there's some better interface at the WebLayer level that allows for WebKit's behavior but is somewhat more sane. You're welcome to leave a comment; I guess for me it's less about whether something is a content layer or not and more about trying to satisfy WebKit's backfacing needs regardless of layer structure. > > This is kind of an edge case, but what happens if a child and its parent both useParentBackfaceVisibility()? > > I don't think that WebKit will generate such a situation (since I don't think a m_contents layer can have an m_contents layer itself). I'll add an assertion to this effect. It is an argument in favor of implementing the per-child solution (in which case this property vanishes). Yeah, I don't think WebKit will generate it right now either, but I just wanted to be robust to the possibility.

I think it's OK to not handle cases like this so long as we fail loudly and not silently - comment is good, ASSERT() is better.

Created attachment 148511 [details] Patch

Comment on attachment 148511 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=148511&action=review I have a few comment nits, and then this is good to go. > Source/Platform/chromium/public/WebLayer.h:118 > + WEBKIT_EXPORT void setUseParentBackfaceVisibility(bool); Can you also mention the limitations of this function that if this is true, this layer must have a parent and that parent can't have this flag set? > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:246 > + // If this is a contents layer, then we should evaluate double-sidedness based Hrm. Contents layer means a bit of a different thing in CCLayerTreeHostCommon than it does in GraphicsLayerChromium. I could imagine somebody thinking this meant an instance of ContentLayerChromium and not GraphicsLayerChromium::m_contentsLayer. I think the property speaks for itself and this comment only makes it confusing. Can you remove it?

(In reply to comment #30) > (From update of attachment 148511 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=148511&action=review > > I have a few comment nits, and then this is good to go. > > > Source/Platform/chromium/public/WebLayer.h:118 > > + WEBKIT_EXPORT void setUseParentBackfaceVisibility(bool); > > Can you also mention the limitations of this function that if this is true, this layer must have a parent and that parent can't have this flag set? Added. > I think the property speaks for itself and this comment only makes it confusing. Can you remove it? Yes, this only made sense when the the property was called isContentsLayer, and the action needed description. Torched. I went ahead and added the WebGL test as well.

Created attachment 148611 [details] Patch

Comment on attachment 148611 [details] Patch Thanks! R=me.

Comment on attachment 148611 [details] Patch Clearing flags on attachment: 148611 Committed r120847: <http://trac.webkit.org/changeset/120847>

All reviewed patches have been landed. Closing bug.