Bug 113040 – [Texmap] TextureMapperImageBuffer should not use rendering code for filters.

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

[Noam Rosenthal]

[Texmap] TextureMapperImageBuffer should not use rendering code for filters.

[Noam Rosenthal]

Using rendering code is a layering violation. A correct approach to this would be to use a client set from the outside, or to move filter rendering into platform/graphics.
Since this is in the way of further optimization in TextureMapperGL filters, I suggest to remove this option for now, as it's isoteric anyway.

[Noam Rosenthal]

Created attachment 194509 [details]
Patch

[Allan Sandfeld Jensen]

Comment on attachment 194509 [details]
Patch

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

> Source/WebCore/ChangeLog:10
> +        Disable TextureMapperImageBuffer filters, until they can be done the right way.
> +        This should only disrupt accelerated filters in fallback mode, which is a very rare
> +        use case.

It is not a very rare use case. It is the default and most common use-case of QtWebKit.

[Allan Sandfeld Jensen]

(In reply to comment #3)
> (From update of attachment 194509 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=194509&action=review
> 
> > Source/WebCore/ChangeLog:10
> > +        Disable TextureMapperImageBuffer filters, until they can be done the right way.
> > +        This should only disrupt accelerated filters in fallback mode, which is a very rare
> > +        use case.
> 
> It is not a very rare use case. It is the default and most common use-case of QtWebKit.

Sorry, the "fallback mode" is common. The combination of an accelerated layer and a filter is rare.

[WebKit Review Bot]

Comment on attachment 194509 [details]
Patch

Clearing flags on attachment: 194509

Committed r146590: <http://trac.webkit.org/changeset/146590>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Allan Sandfeld Jensen]

Allow me to propose an alternative solution. I already had a stashed cleanup of FilterEffectRenderer that would also be suitable here.

[Allan Sandfeld Jensen]

Created attachment 194559 [details]
Patch

Alternative patch. Note it is missing build-file update for Mac.

[Build Bot]

Comment on attachment 194559 [details]
Patch

Attachment 194559 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-commit-queue.appspot.com/results/17291086

[WebKit Review Bot]

Comment on attachment 194559 [details]
Patch

Attachment 194559 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://webkit-commit-queue.appspot.com/results/17120761

New failing tests:
css3/filters/multiple-filters-invalidation.html
css3/filters/effect-combined.html
css3/filters/null-effect-check.html
css3/filters/effect-brightness.html
css3/filters/effect-brightness-clamping.html

[WebKit Review Bot]

Created attachment 194574 [details]
Archive of layout-test-results from gce-cr-linux-07

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-07  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-3.3.8-gcg-201212281604-x86_64-with-GCEL-10.04-gcel_10.04

[Noam Rosenthal]

(In reply to comment #3)
> (From update of attachment 194509 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=194509&action=review
> 
> > Source/WebCore/ChangeLog:10
> > +        Disable TextureMapperImageBuffer filters, until they can be done the right way.
> > +        This should only disrupt accelerated filters in fallback mode, which is a very rare
> > +        use case.
> 
> It is not a very rare use case. It is the default and most common use-case of QtWebKit.

Accelerated css filters are a rare use case.

[Allan Sandfeld Jensen]

Created attachment 194597 [details]
Patch

I forgot to update my old code, so it was missing the change to the brightness filter

[Dean Jackson]

(In reply to comment #12)
> (In reply to comment #3)
> > (From update of attachment 194509 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=194509&action=review
> > 
> > > Source/WebCore/ChangeLog:10
> > > +        Disable TextureMapperImageBuffer filters, until they can be done the right way.
> > > +        This should only disrupt accelerated filters in fallback mode, which is a very rare
> > > +        use case.
> > 
> > It is not a very rare use case. It is the default and most common use-case of QtWebKit.
> 
> Accelerated css filters are a rare use case.

I'm not sure if you mean in QtWebKit here, but they are definitely not rare in Apple platforms.

[Dean Jackson]

Comment on attachment 194597 [details]
Patch

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

I'd prefer to see two patches here. One that does the refactoring of FilterEffectRenderer, and another for the small change to TextureMapperImageBuffer.

> Source/WebCore/platform/graphics/texmap/TextureMapperImageBuffer.h:31
> +#include "CustomFilterGlobalContext.h"
>  #include "ImageBuffer.h"
>  #include "TextureMapper.h"
>  
>  #if USE(TEXTURE_MAPPER)
>  namespace WebCore {
>  
> +class CustomFilterGlobalContext;
> +

I don't think you need this.

[Build Bot]

Comment on attachment 194597 [details]
Patch

Attachment 194597 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-commit-queue.appspot.com/results/17192818

[Noam Rosenthal]

(In reply to comment #14)
> > Accelerated css filters are a rare use case.
> 
> I'm not sure if you mean in QtWebKit here, but they are definitely not rare in Apple platforms.
This is just my experience of not seeing a lot of CSS filters on the web just yet, but if I'm wrong that's great! I'd love to see more of them and I'm helping to optimize their implementation when we can actually get hardware acceleration... (this codepath is for a pure-software fallback).

[Dirk Schulze]

Comment on attachment 194597 [details]
Patch

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

> Source/WebCore/ChangeLog:9
> +        Split FilterEffectRenderer in a simple filter graphics part and leave only
> +        the parts requiring access to the DOM or Render tree in FilterEffectRenderer.

Can you add more information why you are doing this? It would help ti understand the patch more. After all you are adding another inheritance hierarchy for the general use case (CSS filters on web pages) to reuse the filter logic. Maybe a static function or moveing build() up to Filter.h makes more sense? Depends on the use case.

> Source/WebCore/platform/graphics/filters/ImageBufferFilter.cpp:217
> +    default:
> +        break;

There should probably be an ASSERTION_NOT_REACHED as well here. To be honest, I would like to have all filters listed here and remove the default.

> Source/WebCore/platform/graphics/filters/ImageBufferFilter.cpp:233
> +        switch (filterOperation->getOperationType()) {

I am not sure if that even needs a switch. A simple if should be sufficient enough to catch the three cases.

> Source/WebCore/platform/graphics/filters/ImageBufferFilter.cpp:247
> +        if (effect) {

should be more

if (!effect)
    continue;

The question is, do you really just want to skip the operations you do not want to handle? Shouldn't you rather stop the process? The user does not expect what you do here anyway. But it also depends on the use case. So again, why do you split FilterEffectsRenderer?

> Source/WebCore/platform/graphics/filters/ImageBufferFilter.cpp:259
> +    if (!m_effects.size())
> +        return false;

You probably return false in the case of custom shaders and and references above as well.

[Allan Sandfeld Jensen]

(In reply to comment #18)
> 
> if (!effect)
>     continue;
> 
> The question is, do you really just want to skip the operations you do not want to handle? Shouldn't you rather stop the process? The user does not expect what you do here anyway. But it also depends on the use case. So again, why do you split FilterEffectsRenderer?
> 
I already have a patch to also support custom shaders which only leaves reference filters as unsupported.

[Allan Sandfeld Jensen]

This was fixed by telling RenderLayer that we didn't support filters in this mode.