Bug 113040

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.

Created attachment 194509 [details] Patch

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.

(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 on attachment 194509 [details] Patch Clearing flags on attachment: 194509 Committed r146590: <http://trac.webkit.org/changeset/146590>

All reviewed patches have been landed. Closing bug.

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

Created attachment 194559 [details] Patch Alternative patch. Note it is missing build-file update for Mac.

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

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

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

Created attachment 194597 [details] Patch I forgot to update my old code, so it was missing the change to the brightness filter

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

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

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

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