Bug 113040 - [Texmap] TextureMapperImageBuffer should not use rendering code for filters.
Summary: [Texmap] TextureMapperImageBuffer should not use rendering code for filters.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Noam Rosenthal
URL:
Keywords:
Depends on: 113186
Blocks: 68469
  Show dependency treegraph
 
Reported: 2013-03-22 04:05 PDT by Noam Rosenthal
Modified: 2014-03-02 10:11 PST (History)
16 users (show)

See Also:


Attachments
Patch (2.72 KB, patch)
2013-03-22 04:14 PDT, Noam Rosenthal
no flags Details | Formatted Diff | Diff
Patch (41.35 KB, patch)
2013-03-22 09:03 PDT, Allan Sandfeld Jensen
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from gce-cr-linux-07 (2.41 MB, application/zip)
2013-03-22 09:46 PDT, WebKit Review Bot
no flags Details
Patch (41.12 KB, patch)
2013-03-22 10:51 PDT, Allan Sandfeld Jensen
krit: review-
buildbot: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Noam Rosenthal 2013-03-22 04:05:39 PDT
[Texmap] TextureMapperImageBuffer should not use rendering code for filters.
Comment 1 Noam Rosenthal 2013-03-22 04:06:53 PDT
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.
Comment 2 Noam Rosenthal 2013-03-22 04:14:18 PDT
Created attachment 194509 [details]
Patch
Comment 3 Allan Sandfeld Jensen 2013-03-22 04:45:08 PDT
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.
Comment 4 Allan Sandfeld Jensen 2013-03-22 04:46:40 PDT
(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.
Comment 5 WebKit Review Bot 2013-03-22 05:00:46 PDT
Comment on attachment 194509 [details]
Patch

Clearing flags on attachment: 194509

Committed r146590: <http://trac.webkit.org/changeset/146590>
Comment 6 WebKit Review Bot 2013-03-22 05:00:51 PDT
All reviewed patches have been landed.  Closing bug.
Comment 7 Allan Sandfeld Jensen 2013-03-22 08:59:08 PDT
Allow me to propose an alternative solution. I already had a stashed cleanup of FilterEffectRenderer that would also be suitable here.
Comment 8 Allan Sandfeld Jensen 2013-03-22 09:03:46 PDT
Created attachment 194559 [details]
Patch

Alternative patch. Note it is missing build-file update for Mac.
Comment 9 Build Bot 2013-03-22 09:40:53 PDT
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
Comment 10 WebKit Review Bot 2013-03-22 09:46:16 PDT
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
Comment 11 WebKit Review Bot 2013-03-22 09:46:22 PDT
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
Comment 12 Noam Rosenthal 2013-03-22 09:51:59 PDT
(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.
Comment 13 Allan Sandfeld Jensen 2013-03-22 10:51:28 PDT
Created attachment 194597 [details]
Patch

I forgot to update my old code, so it was missing the change to the brightness filter
Comment 14 Dean Jackson 2013-03-22 11:10:32 PDT
(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.
Comment 15 Dean Jackson 2013-03-22 11:16:09 PDT
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.
Comment 16 Build Bot 2013-03-22 11:16:16 PDT
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
Comment 17 Noam Rosenthal 2013-03-22 11:41:26 PDT
(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).
Comment 18 Dirk Schulze 2013-03-22 11:42:34 PDT
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.
Comment 19 Allan Sandfeld Jensen 2013-03-25 08:52:08 PDT
(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.
Comment 20 Allan Sandfeld Jensen 2014-03-02 10:11:01 PST
This was fixed by telling RenderLayer that we didn't support filters in this mode.