RESOLVED FIXED 80046
[chromium] Background filters for composited layers
https://bugs.webkit.org/show_bug.cgi?id=80046
Summary [chromium] Background filters for composited layers
Dana Jansens
Reported 2012-03-01 14:12:57 PST
[chromium] Background filters for composited layers
Attachments
Patch (44.22 KB, patch)
2012-03-01 14:27 PST, Dana Jansens
no flags
Patch (44.41 KB, patch)
2012-03-02 18:38 PST, Dana Jansens
no flags
Patch (43.86 KB, patch)
2012-03-14 10:58 PDT, Dana Jansens
no flags
Patch (59.84 KB, patch)
2012-04-09 16:40 PDT, Dana Jansens
no flags
Patch (59.95 KB, patch)
2012-04-09 16:46 PDT, Dana Jansens
no flags
Patch (59.97 KB, patch)
2012-04-09 17:46 PDT, Dana Jansens
no flags
Patch (57.38 KB, patch)
2012-04-10 10:03 PDT, Dana Jansens
no flags
Patch (79.15 KB, patch)
2012-04-12 13:56 PDT, Dana Jansens
no flags
Patch for landing (79.54 KB, patch)
2012-04-12 17:58 PDT, Dana Jansens
no flags
Dana Jansens
Comment 1 2012-03-01 14:27:04 PST
Dana Jansens
Comment 2 2012-03-01 14:27:49 PST
Here's how background filters look! Going to see if I can't test some piece of this.
Dana Jansens
Comment 3 2012-03-02 18:38:31 PST
Created attachment 129985 [details] Patch Cleaned up the LRC::useRenderSurface() stuff, added LRC::useManagedTexture() instead. I think this is ready for review.
Dana Jansens
Comment 4 2012-03-14 10:58:58 PDT
Created attachment 131883 [details] Patch rebased
David Reveman
Comment 5 2012-03-14 11:44:11 PDT
Comment on attachment 131883 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=131883&action=review Could you add some comments about the number of copies and drawing passes that is used to implement background filters this way? > Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:527 > + drawingSurface->drawBackgroundTexture(this, filteredDeviceBackgroundTextureId, deviceRect, contentsDeviceTransform); I'm not sure I understand what's going on here. Is this drawing the filtered background to the CCLayerImpl render target? In that case you want texture coordinates to just be scaled and biased fragment coordinates, right? How is this achieved?
Adrienne Walker
Comment 6 2012-04-02 15:16:21 PDT
Comment on attachment 131883 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=131883&action=review Needs tests. Unfortunately, I don't think this is something that's easily testable in a unit test, since correctness is really about final pixel results and less about what calls get made on the way there. One option might be exposing background filters to window.internals somehow and writing some layout tests. > Source/WebCore/platform/graphics/chromium/LayerChromium.h:117 > + // regions in this layer. They are used through the WebLayer interface, and are not exposed to HTML. Can you expose them to the WebLayer interface in this patch? > Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:497 > + bool drawingToRootRenderSurface = m_currentRenderSurface == m_defaultRenderSurface; > + if (!drawingToRootRenderSurface) > + return; I kind of wish this case could be handled better and not silently fail, but I am unsure about how to do that. Even an assert might be overkill, since different content could fall down this path in a way that's unexpected. Perhaps a comment on WebLayer::setBackgroundFilters about when this is valid to be used and a comment here about why we're not handling this case would help? > Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:512 > + if (applyBackgroundFilters) { style nit: early out here instead of indenting. > Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:516 > + if (filteredDeviceBackground.getTexture()) { style nit: early out here instead of indenting. > Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:1211 > + if (m_currentRenderSurface == renderSurface && !m_currentManagedTexture) There's at least one other place in LRC that makes this same equality check, and we should be consistent in how we check for this state. > Source/WebCore/platform/graphics/chromium/cc/CCRenderSurface.cpp:116 > + if (!m_contentsBackgroundTexture->reserve(requiredSize, GraphicsContext3D::RGBA)) { > + m_skipsDraw = true; > + return false; > + } I'm not entirely convinced that we should skip drawing the surface here. If we drew the rest of the surface we'd not have the filter, but would still be able to show all the rest of the content. You don't skip the surface if getFramebufferTexture fails, for example. > Source/WebCore/platform/graphics/chromium/cc/CCRenderSurface.cpp:129 > +bool CCRenderSurface::prepareReplicaBackgroundTexture(LayerRendererChromium* layerRenderer) This looks a lot like prepareContentsBackgroundTexture. For the sake of "don't repeat yourself", can you combine these somehow?
Dana Jansens
Comment 7 2012-04-02 17:46:26 PDT
(In reply to comment #5) > (From update of attachment 131883 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=131883&action=review > > Could you add some comments about the number of copies and drawing passes that is used to implement background filters this way? done. > > Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:527 > > + drawingSurface->drawBackgroundTexture(this, filteredDeviceBackgroundTextureId, deviceRect, contentsDeviceTransform); > > I'm not sure I understand what's going on here. Is this drawing the filtered background to the CCLayerImpl render target? In that case you want texture coordinates to just be scaled and biased fragment coordinates, right? How is this achieved? This step it drawing from the filtered readback pixels, using the inverse transform of the surface, to make filtered pixels in the content space of the render surface. This implicitly clips to the bounds of the render surface contents, so that when we draw it back into the target, we only replace pixels that will be under the contents of the surface.
Dana Jansens
Comment 8 2012-04-09 14:49:02 PDT
(In reply to comment #6) > (From update of attachment 131883 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=131883&action=review > > Needs tests. > > Unfortunately, I don't think this is something that's easily testable in a unit test, since correctness is really about final pixel results and less about what calls get made on the way there. One option might be exposing background filters to window.internals somehow and writing some layout tests. ok will add layout tests and upload once they are done. > > Source/WebCore/platform/graphics/chromium/LayerChromium.h:117 > > + // regions in this layer. They are used through the WebLayer interface, and are not exposed to HTML. > > Can you expose them to the WebLayer interface in this patch? As discussed offline, coming in second patch. > > Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:497 > > + bool drawingToRootRenderSurface = m_currentRenderSurface == m_defaultRenderSurface; > > + if (!drawingToRootRenderSurface) > > + return; > > I kind of wish this case could be handled better and not silently fail, but I am unsure about how to do that. Even an assert might be overkill, since different content could fall down this path in a way that's unexpected. Perhaps a comment on WebLayer::setBackgroundFilters about when this is valid to be used and a comment here about why we're not handling this case would help? FIXME Comment added, and will add a comment on WebLayer too. > > Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:512 > > + if (applyBackgroundFilters) { > > style nit: early out here instead of indenting. Done. > > Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:516 > > + if (filteredDeviceBackground.getTexture()) { > > style nit: early out here instead of indenting. Done. > > Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:1211 > > + if (m_currentRenderSurface == renderSurface && !m_currentManagedTexture) > > There's at least one other place in LRC that makes this same equality check, and we should be consistent in how we check for this state. Oh thanks, totally missed that. Added bool isCurrentRenderSurface() to make this consistent. > > Source/WebCore/platform/graphics/chromium/cc/CCRenderSurface.cpp:116 > > + if (!m_contentsBackgroundTexture->reserve(requiredSize, GraphicsContext3D::RGBA)) { > > + m_skipsDraw = true; > > + return false; > > + } > > I'm not entirely convinced that we should skip drawing the surface here. If we drew the rest of the surface we'd not have the filter, but would still be able to show all the rest of the content. You don't skip the surface if getFramebufferTexture fails, for example. Agree, changed. > > Source/WebCore/platform/graphics/chromium/cc/CCRenderSurface.cpp:129 > > +bool CCRenderSurface::prepareReplicaBackgroundTexture(LayerRendererChromium* layerRenderer) > > This looks a lot like prepareContentsBackgroundTexture. For the sake of "don't repeat yourself", can you combine these somehow? Due to quad refactoring, the whole "drawing filters" needs to be done twice now, once for surface once for replica. This is more correct and better etc anyways. We will (lazily perhaps) cache these things in the RenderSurface when we are able to save the filtered texture (pending the same bug as for the foreground filters).
Dana Jansens
Comment 9 2012-04-09 16:40:34 PDT
Created attachment 136337 [details] Patch layout tests included
Dana Jansens
Comment 10 2012-04-09 16:46:55 PDT
Created attachment 136340 [details] Patch - release the background texture if we fail to copy the filtered background into the render surface's backgroundTexture() so we don't end up copying bad texture data back to the screen.
Build Bot
Comment 11 2012-04-09 17:24:45 PDT
Dana Jansens
Comment 12 2012-04-09 17:46:24 PDT
Created attachment 136353 [details] Patch attmpting to fix mac build. the output wasn't too helpful but i am guessing it was the unused variabled in the function causing the problem.
Adrienne Walker
Comment 13 2012-04-09 22:46:35 PDT
Comment on attachment 136353 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=136353&action=review Just so I'm reading this right: framebuffer => (readback) deviceBackgroundTexture deviceBackgroundTexture => (filter) filteredDeviceBackground filteredDeviceBackground => (copy) drawingSurface->backgroundTexture() drawingSurface->backgroundTexture() => (copy) targetRenderSurface It kind of seems like there's an extra copy (or two?) here. Is there a reason you can't just copy directly from the filteredBackgroundTexture into the targetRenderSurface? > Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:564 > + drawingSurface->releaseBackgroundTexture(); I'm a little confused why you only release the background texture here, but not on either of the two returns above this. > Source/WebCore/testing/Internals.cpp:81 > +#if USE(ACCELERATED_COMPOSITING) && PLATFORM(CHROMIUM) Chromium always uses accelerated compositing. > LayoutTests/ChangeLog:13 > + * compositing/filters/background-filter-blur-outsets.html: Added. > + * compositing/filters/background-filter-blur.html: Added. I think these html files should be in platform/chromium/compositing/filters/, since they're not really testing anything for non-Chromium platforms. > LayoutTests/compositing/filters/background-filter-blur-outsets.html:22 > + .yellow-parent { > + width: 600px; > + height: 600px; > + -webkit-filter: blur(10px); > + } > + .centered { > + width: 200px; > + height: 200px; > + position: absolute; > + left: 100px; > + top: 100px; > + } Unused? > LayoutTests/compositing/filters/background-filter-blur-outsets.html:33 > + window.setTimeout(setBlur, 0); Can you write these tests without using setTimeout? > LayoutTests/compositing/filters/background-filter-blur-outsets.html:47 > +<div class="composited green-border" style="position:absolute; left:50px; top:50px; width:200px; height:200px;"></div> > +<div id="blur" style="position:absolute; left:59px; top:59px; width:200px; height:200px; border:1px solid green;"></div> Can you remove the border from the blur node, so that it's clear that it's not using its own border?
Dana Jansens
Comment 14 2012-04-10 09:11:42 PDT
Comment on attachment 136353 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=136353&action=review The third step is missing its key component, which is the clipping. framebuffer => (readback) deviceBackgroundTexture deviceBackgroundTexture => (filter) filteredDeviceBackground filteredDeviceBackground => (copy and clip) drawingSurface->backgroundTexture() drawingSurface->backgroundTexture() => (copy) targetRenderSurface I can make a picture for you after lunch if you like :) Basically, the surface may not be a rect in the device, but we readback and filter a rect. So we need to clip it to whatever shape the surface is so that we only replace pixels that will be under the surface. I couldn't think of how to do that without making a texture in the surface's content space and drawing it the same way as the surface itself. Perhaps this can be done more cleverly, but we can do that in a followup? There is for sure an extra copy in the last step and a FIXME in the code. We could blend the background texture with the contents texture and draw them both in a single pass via shader magic. But I figured that would be a nice followup. >> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:564 >> + drawingSurface->releaseBackgroundTexture(); > > I'm a little confused why you only release the background texture here, but not on either of the two returns above this. Because in the above 2 returns, the texture has not been successfully prepared yet (attempted in line 556). >> Source/WebCore/testing/Internals.cpp:81 >> +#if USE(ACCELERATED_COMPOSITING) && PLATFORM(CHROMIUM) > > Chromium always uses accelerated compositing. We do guard the GraphicsLayerChromium.cpp file like this, if not others. Is it a case of historical ifdefs and future code should not use it? >> LayoutTests/ChangeLog:13 >> + * compositing/filters/background-filter-blur.html: Added. > > I think these html files should be in platform/chromium/compositing/filters/, since they're not really testing anything for non-Chromium platforms. Oh! Yeh makes sense. >> LayoutTests/compositing/filters/background-filter-blur-outsets.html:22 >> + } > > Unused? Oops, yeh. From the test I copied. >> LayoutTests/compositing/filters/background-filter-blur-outsets.html:33 >> + window.setTimeout(setBlur, 0); > > Can you write these tests without using setTimeout? I tried to, but had difficulty making the blur happen. It seemed that DRT was completing the test before the blur got applied. >> LayoutTests/compositing/filters/background-filter-blur-outsets.html:47 >> +<div id="blur" style="position:absolute; left:59px; top:59px; width:200px; height:200px; border:1px solid green;"></div> > > Can you remove the border from the blur node, so that it's clear that it's not using its own border? Without the border, the element is clear and just gets culled from the render tree. I could use something other than a border, but didn't want to do something like text, and the background needs to remain clear. Maybe I can use a transparent background-image.. or any other preference?
Dana Jansens
Comment 15 2012-04-10 10:03:59 PDT
Created attachment 136476 [details] Patch Comments addressed. Problem solved with a black border! I tried not using setTimeout again, and just setting the blur directly (and moving the script below the html so it can find the element), but it just fails to work that way.
Dana Jansens
Comment 16 2012-04-10 10:19:07 PDT
Because a picture's worth a thousand words http://i.imgur.com/HNkXV.png
Adrienne Walker
Comment 17 2012-04-10 20:06:03 PDT
(In reply to comment #15) > Created an attachment (id=136476) [details] > Patch > > Comments addressed. Problem solved with a black border! Ooh, the black border is even better. It makes it clear it's blurring what's beneath and the black border is over top. > I tried not using setTimeout again, and just setting the blur directly (and moving the script below the html so it can find the element), but it just fails to work that way. Ah, put the setBlur in an onLoad handler for the document, and it should do what you expect. getElementById would fail if you called setBlur immediately in that script block. (In reply to comment #14) > (From update of attachment 136353 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=136353&action=review > > The third step is missing its key component, which is the clipping. > > framebuffer => (readback) deviceBackgroundTexture > deviceBackgroundTexture => (filter) filteredDeviceBackground > filteredDeviceBackground => (copy and clip) drawingSurface->backgroundTexture() > drawingSurface->backgroundTexture() => (copy) targetRenderSurface > > I can make a picture for you after lunch if you like :) Basically, the surface may not be a rect in the device, but we readback and filter a rect. So we need to clip it to whatever shape the surface is so that we only replace pixels that will be under the surface. I couldn't think of how to do that without making a texture in the surface's content space and drawing it the same way as the surface itself. Perhaps this can be done more cleverly, but we can do that in a followup? Oh, I see! Thanks for the diagram. Needing an extra copy for clipping when you're not axis-aligned makes sense. Can you then add one last test that involves a background filter and a perspective transform, where it demonstrates that it is blurring in device space and that non-axis-aligned layers are clipped properly (or not too much)? > >> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:564 > >> + drawingSurface->releaseBackgroundTexture(); > > > > I'm a little confused why you only release the background texture here, but not on either of the two returns above this. > > Because in the above 2 returns, the texture has not been successfully prepared yet (attempted in line 556). Oops, quite right! I misread which function was reserving. > >> Source/WebCore/testing/Internals.cpp:81 > >> +#if USE(ACCELERATED_COMPOSITING) && PLATFORM(CHROMIUM) > > > > Chromium always uses accelerated compositing. > > We do guard the GraphicsLayerChromium.cpp file like this, if not others. Is it a case of historical ifdefs and future code should not use it? It's my opinion that it's historical and we make assumptions elsewhere that Chromium always has this on. Maybe jamesr has an opinion here?
James Robinson
Comment 18 2012-04-10 21:09:04 PDT
We haven't always compiled chromium with USE(ACCELERATED_COMPOSITING), but we have for the past few years. I think in chromium-specific code there is no value in keeping USE(ACCELERATED_COMPOSITING) guards and would recommend not adding any more. In code that's not specific to chromium, of course, you need USE(ACCELERATED_COMPOSITING).
Dana Jansens
Comment 19 2012-04-12 10:17:06 PDT
(In reply to comment #17) > (In reply to comment #15) > > Created an attachment (id=136476) [details] [details] > > Patch > > > > Comments addressed. Problem solved with a black border! > > Ooh, the black border is even better. It makes it clear it's blurring what's beneath and the black border is over top. Cool thanks. > Ah, put the setBlur in an onLoad handler for the document, and it should do what you expect. getElementById would fail if you called setBlur immediately in that script block. Attempting... When I use an onload handler, I get this. CONSOLE MESSAGE: line 15: Uncaught Error: INVALID_STATE_ERR: DOM Exception 11 Meaning, the render object does not have a renderLayer or it is not composited yet. Can we just use the setTimeout(0)? > (In reply to comment #14) > Oh, I see! Thanks for the diagram. Needing an extra copy for clipping when you're not axis-aligned makes sense. Can you then add one last test that involves a background filter and a perspective transform, where it demonstrates that it is blurring in device space and that non-axis-aligned layers are clipped properly (or not too much)? k > It's my opinion that it's historical and we make assumptions elsewhere that Chromium always has this on. Maybe jamesr has an opinion here? removed the USE() checks.
Dana Jansens
Comment 20 2012-04-12 13:56:30 PDT
Adrienne Walker
Comment 21 2012-04-12 16:32:57 PDT
Comment on attachment 136966 [details] Patch R=me. Thanks for the additional test. I think removing copies and testing this outside of layout tests are both things we can do in the future to improve this, but this is a great start.
Dana Jansens
Comment 22 2012-04-12 16:33:43 PDT
I'm a bit concerned. Regressions: Unexpected image mismatch : (20) compositing/backface-visibility-hierarchical-transform.html = IMAGE compositing/geometry/fixed-position.html = IMAGE compositing/masks/direct-image-mask.html = IMAGE compositing/masks/masked-ancestor.html = IMAGE compositing/masks/multiple-masks.html = IMAGE compositing/masks/simple-composited-mask.html = IMAGE compositing/reflections/load-video-in-reflection.html = IMAGE compositing/reflections/reflection-positioning.html = IMAGE compositing/reflections/reflection-positioning2.html = IMAGE compositing/reflections/remove-add-reflection.html = IMAGE compositing/reflections/transform-inside-reflection.html = IMAGE compositing/webgl/webgl-reflection.html = IMAGE css3/filters/effect-blur-hw.html = IMAGE fast/canvas/webgl/webgl-composite-modes-repaint.html = IMAGE fast/forms/placeholder-position.html = IMAGE fast/text/drawBidiText.html = IMAGE fast/text/international/bidi-listbox-atsui.html = IMAGE fast/text/international/thai-line-breaks.html = IMAGE platform/chromium-linux/fast/text/international/complex-joining-using-gpos.html = IMAGE platform/chromium/compositing/plugins/webplugin-reflection.html = IMAGE None of these repro if I run them one at a time. But they end up failing with empty compositor output if I run them as a set. What does this mean?
Dana Jansens
Comment 23 2012-04-12 16:53:12 PDT
Comment on attachment 136966 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=136966&action=review > Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:1323 > + if (isCurrentRenderSurface(renderSurface)) > + return true; This is causing the test failures.
Dana Jansens
Comment 24 2012-04-12 17:56:27 PDT
Comment on attachment 136966 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=136966&action=review >> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:1323 >> + return true; > > This is causing the test failures. This was essentially reverting http://trac.webkit.org/changeset/110741. Will remove this check.
Dana Jansens
Comment 25 2012-04-12 17:58:41 PDT
Created attachment 137004 [details] Patch for landing
WebKit Review Bot
Comment 26 2012-04-12 22:29:04 PDT
Comment on attachment 137004 [details] Patch for landing Clearing flags on attachment: 137004 Committed r114081: <http://trac.webkit.org/changeset/114081>
WebKit Review Bot
Comment 27 2012-04-12 22:29:10 PDT
All reviewed patches have been landed. Closing bug.
Simon Fraser (smfr)
Comment 28 2013-03-13 09:49:36 PDT
Comment on attachment 137004 [details] Patch for landing View in context: https://bugs.webkit.org/attachment.cgi?id=137004&action=review > Source/WebCore/testing/Internals.idl:75 > + void setBackgroundBlurOnNode(in Node node, in long blurLength) raises(DOMException); This is abuse of the internals object.
Dana Jansens
Comment 29 2013-03-13 10:08:15 PDT
(In reply to comment #28) > (From update of attachment 137004 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=137004&action=review > > > Source/WebCore/testing/Internals.idl:75 > > + void setBackgroundBlurOnNode(in Node node, in long blurLength) raises(DOMException); > > This is abuse of the internals object. How would you have tested a feature not exposed to the web like this?
Note You need to log in before you can comment on or make changes to this bug.