Bug 80046 – [chromium] Background filters for composited layers

• Component: New Bugs
• Status: RESOLVED FIXED

[Dana Jansens]

[chromium] Background filters for composited layers

[Dana Jansens]

Created attachment 129743 [details]
Patch

[Dana Jansens]

Here's how background filters look! Going to see if I can't test some piece of this.

[Dana Jansens]

Created attachment 129985 [details]
Patch

Cleaned up the LRC::useRenderSurface() stuff, added LRC::useManagedTexture() instead. I think this is ready for review.

[Dana Jansens]

Created attachment 131883 [details]
Patch

rebased

[David Reveman]

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 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]

(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]

(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]

Created attachment 136337 [details]
Patch

layout tests included

[Dana Jansens]

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 on attachment 136340 [details]
Patch

Attachment 136340 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12374277

[Dana Jansens]

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 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 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]

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]

Because a picture's worth a thousand words http://i.imgur.com/HNkXV.png

[Adrienne Walker]

(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]

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]

(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]

Created attachment 136966 [details]
Patch

[Adrienne Walker]

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]

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 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 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]

Created attachment 137004 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 137004 [details]
Patch for landing

Clearing flags on attachment: 137004

Committed r114081: <http://trac.webkit.org/changeset/114081>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Simon Fraser (smfr)]

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]

(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?