Bug 81435 – [CSS Shaders] Make CSS Shaders compile on Chromium

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Alexandru Chiculita]

In this patch I will add the window.layoutTestController.overridePreference("WebKitCSSCustomFilterEnabled", "1"); and make the custom shader filter tests run on Chromium.

[Alexandru Chiculita]

Created attachment 132432 [details]
Patch V1

[WebKit Review Bot]

Please wait for approval from [e-mail] before submitting because this patch contains changes to the Chromium public API.

[WebKit Review Bot]

Comment on attachment 132432 [details]
Patch V1

Attachment 132432 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11970376

New failing tests:
css3/filters/custom/effect-custom-parameters.html
css3/filters/custom/custom-filter-property-computed-style.html
css3/filters/custom/custom-filter-shader-cache.html
css3/filters/custom/effect-custom-combined-missing.html
css3/filters/custom/custom-filter-property-parsing.html
css3/filters/custom/missing-custom-filter-shader.html
css3/filters/custom/effect-custom.html

[WebKit Review Bot]

Comment on attachment 132432 [details]
Patch V1

Attachment 132432 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11960974

New failing tests:
css3/filters/custom/effect-custom-parameters.html
css3/filters/custom/custom-filter-property-computed-style.html
css3/filters/custom/custom-filter-shader-cache.html
css3/filters/custom/effect-custom-combined-missing.html
css3/filters/custom/custom-filter-property-parsing.html
css3/filters/custom/missing-custom-filter-shader.html
css3/filters/custom/effect-custom.html

[Darin Fisher (:fishd, Google)]

Comment on attachment 132432 [details]
Patch V1

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

> Source/WebKit/chromium/public/WebSettings.h:101
> +    virtual void setExperimentalCSSCustomFilterEnabled(bool) = 0;

Chromium WebKit API change LGTM

[Alexandru Chiculita]

Created attachment 132730 [details]
Patch V2

Updated the test results for linux.

[Stephen White]

Comment on attachment 132730 [details]
Patch V2

One thing I've noticed (not new to this patch, but would be brought into Chrome with it) is that FECustomFilter.cpp introduces a dependency on Document.h.  This is a layering violation, since the  platform/ layer is not supposed to depend on files in dom/.  You should probably either pass down the document's HostWindow directly (this is ok since HostWindow is in the platform/ layer), or for Chromium you can just pass a NULL HostWindow to GraphicsContext3D::create() for an offscreen context (not sure if you can do that in Safari, though).

Similarly, the dependencies on CachedShader and StyleCachedShader are problematic, but I'm less sure how to remove them.

[Alexandru Chiculita]

(In reply to comment #7)
> (From update of attachment 132730 [details])
> One thing I've noticed (not new to this patch, but would be brought into Chrome with it) is that FECustomFilter.cpp introduces a dependency on Document.h.  This is a layering violation, since the  platform/ layer is not supposed to depend on files in dom/.  You should probably either pass down the document's HostWindow directly (this is ok since HostWindow is in the platform/ layer), or for Chromium you can just pass a NULL HostWindow to GraphicsContext3D::create() for an offscreen context (not sure if you can do that in Safari, though).
> 
In the end I will want to use the same GraphicsContext3D for all the shaders rendered in software, so that we can cache compiled shaders and mesh vertex/index buffers between elements. So it's just a matter of time until we have Document.h removed from this file. Is this something that can wait? 

I think it's also a good idea to use the same GraphicsContext3D that Skia is using. With that in place we can have a special implementation that will just use the texture allocated by Skia instead of reading it back from GPU just to move it back there.

> Similarly, the dependencies on CachedShader and StyleCachedShader are problematic, but I'm less sure how to remove them.

Thanks for catching this. We already fixed that dependency, but the headers are still included with no real reason. Do you mind if I remove those part of this patch?

[Alexandru Chiculita]

(In reply to comment #8)
> (In reply to comment #7)
> > (From update of attachment 132730 [details] [details])
> > One thing I've noticed (not new to this patch, but would be brought into Chrome with it) is that FECustomFilter.cpp introduces a dependency on Document.h.  This is a layering violation, since the  platform/ layer is not supposed to depend on files in dom/.  You should probably either pass down the document's HostWindow directly (this is ok since HostWindow is in the platform/ layer), or for Chromium you can just pass a NULL HostWindow to GraphicsContext3D::create() for an offscreen context (not sure if you can do that in Safari, though).
> > 
> In the end I will want to use the same GraphicsContext3D for all the shaders rendered in software, so that we can cache compiled shaders and mesh vertex/index buffers between elements. So it's just a matter of time until we have Document.h removed from this file. Is this something that can wait? 
> 
> I think it's also a good idea to use the same GraphicsContext3D that Skia is using. With that in place we can have a special implementation that will just use the texture allocated by Skia instead of reading it back from GPU just to move it back there.
> 
Actually, I'm trying to fix that right now. What about passing the FrameView instead of the Document. Is that still a violation? I want to avoid passing a fixed value that might change without a notice (ie. layers moved to a different host window maybe).

[Alexandru Chiculita]

Created attachment 132938 [details]
Patch V3

1. Removed the Document.h and used the ScrollView.h instead
2. Removed references to CachedShader.h and StyleCachedShader.h as they were not used in that file anymore

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[Alexandru Chiculita]

(In reply to comment #11)
> Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

It's the same API from before -> https://bugs.webkit.org/show_bug.cgi?id=81435#c5

[Sam Weinig]

> Actually, I'm trying to fix that right now. What about passing the FrameView instead of the Document. Is that still a violation? I want to avoid passing a fixed value that might change without a notice (ie. layers moved to a different host window maybe).

FrameView would also be a layering violation.

[James Robinson]

Comment on attachment 132938 [details]
Patch V3

ScrollView is in platform/ so it's not a layering violation, but it doesn't make very much sense  Filters don't have anything to do with scrolling.  Why not HostWindow?

What are filters tied to - I'm assuming that they can't move between documents / windows?

[Alexandru Chiculita]

(In reply to comment #14)
> (From update of attachment 132938 [details])
> ScrollView is in platform/ so it's not a layering violation, but it doesn't make very much sense  Filters don't have anything to do with scrolling.  Why not HostWindow?
> 
> What are filters tied to - I'm assuming that they can't move between documents / windows?

Filters are tied to the RenderLayer, so I assumed that the RenderLayers will not be recreated when for example the HostWindow is changed, that's why I want to avoid giving the HostWindow to the filters. 

Anyway it seems like something is still wrong, because it creates the context and reuses it even though the HostWindow of the ScrollView might have changed, so I still need to find a way to invalidate the filters when that happens.

I will also check if the HostWindow is needed at all when creating offscreen GraphicsContext3D's. On Chromium I understand that it is not needed, so I will check on other platforms. If it is not needed, I will just remove the reference and post a new patch.

[James Robinson]

(In reply to comment #15)
> (In reply to comment #14)
> > (From update of attachment 132938 [details] [details])
> > ScrollView is in platform/ so it's not a layering violation, but it doesn't make very much sense  Filters don't have anything to do with scrolling.  Why not HostWindow?
> > 
> > What are filters tied to - I'm assuming that they can't move between documents / windows?
> 
> Filters are tied to the RenderLayer, so I assumed that the RenderLayers will not be recreated when for example the HostWindow is changed, that's why I want to avoid giving the HostWindow to the filters. 
> 

RenderLayers will be recreated when the HostWindow changes, so no problem there.

> Anyway it seems like something is still wrong, because it creates the context and reuses it even though the HostWindow of the ScrollView might have changed, so I still need to find a way to invalidate the filters when that happens.
> 

No you don't.

> I will also check if the HostWindow is needed at all when creating offscreen GraphicsContext3D's. On Chromium I understand that it is not needed, so I will check on other platforms. If it is not needed, I will just remove the reference and post a new patch.

It is not in Chromium. I'm not sure about other platforms.

[Stephen White]

(In reply to comment #8)
> In the end I will want to use the same GraphicsContext3D for all the shaders rendered in software, so that we can cache compiled shaders and mesh vertex/index buffers between elements. So it's just a matter of time until we have Document.h removed from this file. Is this something that can wait? 

I'd prefer to fix it now, if possible.  We have some code reorg plans that this dependency would prevent.

> I think it's also a good idea to use the same GraphicsContext3D that Skia is using. With that in place we can have a special implementation that will just use the texture allocated by Skia instead of reading it back from GPU just to move it back there.

Actually, I don't think this is a good idea.  Skia makes assumptions about GL context state, which means you'd have to call a reset on Skia each time the custom filters changed GL state, and vice versa, which could impact performance.

That said, it shouldn't be too much trouble to create a context for custom filter use which shares textures with the skia context.  That way, you wouldn't need a readback.  Just set "shareResources" to true in your GraphicsContext3D::Attributes (all contexts which set this flag are in the same share group).

> > Similarly, the dependencies on CachedShader and StyleCachedShader are problematic, but I'm less sure how to remove them.
> 
> Thanks for catching this. We already fixed that dependency, but the headers are still included with no real reason. Do you mind if I remove those part of this patch?

That would be great.

[Alexandru Chiculita]

Created attachment 133082 [details]
Patch V4

In this patch I change the Document reference to HostWindow instead. As per previous comments the RenderLayers will get recreated anyway so there's no need to make sure the Filters are invalidated when the HostWindow changes.

Also addressed all the other comments (removed unused headers from FECustomFilter.cpp).

[Stephen White]

Comment on attachment 133082 [details]
Patch V4

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

> LayoutTests/platform/chromium/test_expectations.txt:-3081
> -// CSS custom() filters are not currently supported
> -BUGWK71392 SKIP : css3/filters/custom = FAIL
> -

Since your change only includes mac and linux results, I'd leave this line in, but change the SKIP to WIN.  That way the win bots won't go red on your change.

[Alexandru Chiculita]

Created attachment 133103 [details]
Patch V5

Done. I've added the skip for Windows.

[Stephen White]

Comment on attachment 133103 [details]
Patch V5

OK.  Thanks for the extra effort.  r=me

[WebKit Review Bot]

Comment on attachment 133103 [details]
Patch V5

Clearing flags on attachment: 133103

Committed r111610: <http://trac.webkit.org/changeset/111610>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.