Extracting the SVG Filter Reference implementation out of the FilterEffectRenderer into its own typ…

Extracting the SVG Filter Reference implementation out of the FilterEffectRenderer into its own type of filter.
This way we don't need to build a FilterEffectRenderer when rendering the SVG reference in the compositor.

BUG=229714

[achicu, 2013-04-10 00:55:20]

This is a draft patch. Most of the functionality should be covered by existing
tests, but I need to double check what are the existing tests.

[Stephen Chennney, 2013-04-10 01:26:51]

Why are the new names "Reference..."? I might be late to the party, but what are
they referencing of referenced by? Or are they "reference" in the sense of a
ref-test? If the names are just coming from the spec, then they don't need the
"Reference" bit.

[Stephen White, 2013-04-10 01:59:43]

https://codereview.chromium.org/13984002/diff/1/Source/WebCore/platform/graph...
File Source/WebCore/platform/graphics/filters/FilterOperation.h (right):

https://codereview.chromium.org/13984002/diff/1/Source/WebCore/platform/graph...
Source/WebCore/platform/graphics/filters/FilterOperation.h:173:
ReferenceFilterRenderer* filterRenderer() const { return m_filterRenderer.get();
}
This is a layering violation, I'm afraid.  We can't have stuff in platform/
depending on stuff in rendering/.  (Don't be fooled by the
CachedSVGDocumentReference* above; that was introduced by krit@ while I wasn't
looking, and should be removed as well.)

Let's work out another way to do this.

[achicu, 2013-04-10 15:55:22]

On 2013/04/10 01:26:51, Stephen Chenney wrote:
> Why are the new names "Reference..."? I might be late to the party, but what
are
> they referencing of referenced by? Or are they "reference" in the sense of a
> ref-test? If the names are just coming from the spec, then they don't need the
> "Reference" bit.

We already have ReferenceFilterOperation and I thought that ReferenceFilter
prefix would make the link between them more obvious. Also, there are other
FilterRenderers and FilterBuilders, so just using the simpler name would not be
any better.

The name of the initial ReferenceFilterOperation comes from the spec, these ones
just try to follow the same pattern.

The "reference" is about a reference to an SVG filter. Any suggestion is
welcomed.

[achicu, 2013-04-10 17:38:55]

On 2013/04/10 01:59:43, Stephen White wrote:
>
https://codereview.chromium.org/13984002/diff/1/Source/WebCore/platform/graph...
> File Source/WebCore/platform/graphics/filters/FilterOperation.h (right):
> 
>
https://codereview.chromium.org/13984002/diff/1/Source/WebCore/platform/graph...
> Source/WebCore/platform/graphics/filters/FilterOperation.h:173:
> ReferenceFilterRenderer* filterRenderer() const { return
m_filterRenderer.get();
> }
> This is a layering violation, I'm afraid.  We can't have stuff in platform/
> depending on stuff in rendering/.  (Don't be fooled by the
> CachedSVGDocumentReference* above; that was introduced by krit@ while I wasn't
> looking, and should be removed as well.)
> 
> Let's work out another way to do this.

I've removed ReferenceFilterRenderer and replaced it with the base class
instead, which is Filter.

I think FilterOperation* classes would better live in the rendering/style
folder, as they are only used as CSS structures and don't really have anything
to do with the platform layer.

Also looking into removing CachedSVGDocumentReference.

[achicu, 2013-04-10 17:43:49]

https://codereview.chromium.org/13984002/diff/7001/Source/WebCore/platform/gr...
File Source/WebCore/platform/graphics/filters/skia/SkiaImageFilterBuilder.cpp
(right):

https://codereview.chromium.org/13984002/diff/7001/Source/WebCore/platform/gr...
Source/WebCore/platform/graphics/filters/skia/SkiaImageFilterBuilder.cpp:201:
filter.reset(SkiaImageFilterBuilder::build(filterEffect));
I will remove this change. Not needed anymore as SkiaImageFilterBuilder::build
already checks for null.

[achicu, 2013-04-10 21:47:42]

I posted a bug to move the FilterOperations from platform to rendering/style/
folder in WebKit. I will start another issue for Blink.

https://bugs.webkit.org/show_bug.cgi?id=114378

[Stephen White, 2013-04-10 22:19:38]

https://codereview.chromium.org/13984002/diff/12001/Source/WebCore/rendering/...
File Source/WebCore/rendering/RenderLayer.cpp (right):

https://codereview.chromium.org/13984002/diff/12001/Source/WebCore/rendering/...
Source/WebCore/rendering/RenderLayer.cpp:6223:
ReferenceFilterRenderer::computeFilter(renderer(), referenceOperation);
OK, bear with me, I'm trying to understand the callgraph here.

FilterEffectRenderer::build() calls ReferenceFilterBuilder::build().  That's ok,
this was occuring before, just now refactored to its own file in rendering/svg.

ReferenceFilterRenderer::build() also calls ReferenceFilterBuilder::build(), as
does ReferenceFilterRenderer::computeFilter().

ReferenceFilterRenderer::computeFilter() is only (AFAICT) called here, from
RenderLayer::computeFilterOperations().

But the latter is called by
RenderLayerBacking::updateOrRemoveFilterEffectRenderer(), just prior to calling
FilterEffectRenderer::build(), which looks like it'll do the build again, and
also by RenderLayerBacking::updateFilters(), which looks like (although I'm not
certain), it'll do the build again before handing it off to the graphics layer.

It seems like we just have too many callsites for this builder code.  It may
work, but the ordering is not clear from the code, and doesn't look performant.

https://codereview.chromium.org/13984002/diff/12001/Source/WebCore/rendering/...
File Source/WebCore/rendering/svg/ReferenceFilterRenderer.cpp (right):

https://codereview.chromium.org/13984002/diff/12001/Source/WebCore/rendering/...
Source/WebCore/rendering/svg/ReferenceFilterRenderer.cpp:60:
filterOperation->setFilterRenderer(filterRenderer.release());
I'm probably being dumb, but why does the filter operation need to know the root
Filter?  I don't see any calls to the filterRenderer() accessor, nor
m_filterRenderer.

https://codereview.chromium.org/13984002/diff/12001/Source/WebCore/rendering/...
File Source/WebCore/rendering/svg/ReferenceFilterRenderer.h (right):

https://codereview.chromium.org/13984002/diff/12001/Source/WebCore/rendering/...
Source/WebCore/rendering/svg/ReferenceFilterRenderer.h:44: class
ReferenceFilterRenderer: public Filter {
I don't mind the Reference part, but the "Renderer" part bothers me.  What does
it render?  (Yes, FilterEffectRenderer has this problem too, and I have the same
problem with it too.)

Since it's a subclass of Filter, how about just ReferenceFilter?

https://codereview.chromium.org/13984002/diff/12001/Source/WebCore/rendering/...
Source/WebCore/rendering/svg/ReferenceFilterRenderer.h:51: static void
computeFilter(RenderObject*, ReferenceFilterOperation*);
It's a personal pet peeve of mine, but on a computer, "compute" is practically a
zero-information word.  :)

[Stephen White, 2013-04-10 22:25:59]

On 2013/04/10 17:38:55, achicu wrote:
> On 2013/04/10 01:59:43, Stephen White wrote:
> >
>
https://codereview.chromium.org/13984002/diff/1/Source/WebCore/platform/graph...
> > File Source/WebCore/platform/graphics/filters/FilterOperation.h (right):
> > 
> >
>
https://codereview.chromium.org/13984002/diff/1/Source/WebCore/platform/graph...
> > Source/WebCore/platform/graphics/filters/FilterOperation.h:173:
> > ReferenceFilterRenderer* filterRenderer() const { return
> m_filterRenderer.get();
> > }
> > This is a layering violation, I'm afraid.  We can't have stuff in platform/
> > depending on stuff in rendering/.  (Don't be fooled by the
> > CachedSVGDocumentReference* above; that was introduced by krit@ while I
wasn't
> > looking, and should be removed as well.)
> > 
> > Let's work out another way to do this.
> 
> I've removed ReferenceFilterRenderer and replaced it with the base class
> instead, which is Filter.
> 
> I think FilterOperation* classes would better live in the rendering/style
> folder, as they are only used as CSS structures and don't really have anything
> to do with the platform layer.

I don't think that'll work either, since platform/graphics/GraphicsLayer*
reference FilterOperation pretty heavily, so that would then become a layering
violation.

> Also looking into removing CachedSVGDocumentReference.

Consider reverting https://bugs.webkit.org/show_bug.cgi?id=102967

[achicu, 2013-04-10 23:05:15]

Ok, let's separate the issues for now. This patch is orthogonal to that change.
The new code should not violate any layering for now.

I will work on reverting CachedSVGDocumentReference in a different patch. 

I was also talking on #WebKit to move GraphicsLayer to FilterEffect, so that it
could handle both SVG trees and CSS Filters without hiding them inside the
ReferenceFilterOperation. But that's a different story.

[achicu, 2013-04-16 21:34:34]

@Stephen White: Can you please review this patch? I will work on reverting the
CachedSVGDocumentReference in a different patch.

[achicu, 2013-04-16 21:58:17]

Added https://codereview.chromium.org/13853009 to track the
CachedSVGDocumentReference issue.

[achicu, 2013-04-16 22:00:43]

Sorry didn't see the previous review. I will take a look.

[achicu, 2013-04-16 22:11:16]

Thanks the review, I will rework based on the comments and submit again.

https://codereview.chromium.org/13984002/diff/12001/Source/WebCore/rendering/...
File Source/WebCore/rendering/RenderLayer.cpp (right):

https://codereview.chromium.org/13984002/diff/12001/Source/WebCore/rendering/...
Source/WebCore/rendering/RenderLayer.cpp:6223:
ReferenceFilterRenderer::computeFilter(renderer(), referenceOperation);
On 2013/04/10 22:19:38, Stephen White wrote:
> But the latter is called by
> RenderLayerBacking::updateOrRemoveFilterEffectRenderer(), just prior to
calling
> FilterEffectRenderer::build(), which looks like it'll do the build again, and
> also by RenderLayerBacking::updateFilters(), which looks like (although I'm
not
> certain), it'll do the build again before handing it off to the graphics
layer.

FilterEffectRenderer::build will only be called when used in software mode. So
the call-path is like this:
1. Resolving the filter operations would update the Filter tree in the
ReferenceFilterOperations
2. GraphicsLayer will try to apply the filter
3. If that fails, it will recompute the filter in the
FilterEffectRenderer::build. This is when the filter is recomputed.

For now we always have to rebuild the SVG filter reference because there's no
way to know if the referenced filter changed or not. If I don't rebuild the
filter every time, then cases when one attribute changes are not covered. I
suppose in a different patch, I could set a flag from the filter tree, saying
that the filters need a rebuild.

> 
> It seems like we just have too many callsites for this builder code.  It may
> work, but the ordering is not clear from the code, and doesn't look
performant.

I guess the current patch is not changing how many times the filter get rebuild,
it's just extracting functionality out FilterEffectRenderer::build.

https://codereview.chromium.org/13984002/diff/12001/Source/WebCore/rendering/...
File Source/WebCore/rendering/svg/ReferenceFilterRenderer.h (right):

https://codereview.chromium.org/13984002/diff/12001/Source/WebCore/rendering/...
Source/WebCore/rendering/svg/ReferenceFilterRenderer.h:44: class
ReferenceFilterRenderer: public Filter {
On 2013/04/10 22:19:38, Stephen White wrote:
> I don't mind the Reference part, but the "Renderer" part bothers me.  What
does
> it render?  (Yes, FilterEffectRenderer has this problem too, and I have the
same
> problem with it too.)
> 
> Since it's a subclass of Filter, how about just ReferenceFilter?

That sounds fine. Will do.

https://codereview.chromium.org/13984002/diff/12001/Source/WebCore/rendering/...
Source/WebCore/rendering/svg/ReferenceFilterRenderer.h:51: static void
computeFilter(RenderObject*, ReferenceFilterOperation*);
On 2013/04/10 22:19:38, Stephen White wrote:
> It's a personal pet peeve of mine, but on a computer, "compute" is practically
a
> zero-information word.  :)

What about simply "create"?

[achicu, 2013-04-17 00:47:17]

I've updated the patch. Move ReferenceFilter to platform, to avoid all the
casts.

Also, the Filter graph is now build outside of the FilterEffectRenderer, meaning
that the input of the FilterEffects would be directly the SourceGraphic, instead
of the FilterEffect previously generated in FitlerEffectRenderer::build. 

When there's another FilterOperation preceding the ReferenceFilterOperation, the
SkImageFilter built out of the FilterEffect would have overwritten any of the
previous FilterOperations. Fixed that by temporarily injecting the SourceGraphic
of the FilterReference into the m_map.

[Stephen White, 2013-04-22 20:01:48]

Sorry it took me so long to get to this.  This looks great.

LGTM.

https://codereview.chromium.org/13984002/diff/31001/Source/core/platform/grap...
File Source/core/platform/graphics/filters/FilterEffect.cpp (right):

https://codereview.chromium.org/13984002/diff/31001/Source/core/platform/grap...
Source/core/platform/graphics/filters/FilterEffect.cpp:207:
m_inputEffects.at(i).get()->setMaxEffectRectRecursive(effectRect);
This will be slightly wasteful where there are "diamond" patterns in the DAG,
but I can live with that.  Looks like clearResultRecursive() has the same
problem.

[Stephen White, 2013-04-22 20:10:04]

https://codereview.chromium.org/13984002/diff/31001/Source/core/platform/grap...
File Source/core/platform/graphics/filters/FilterEffect.h (right):

https://codereview.chromium.org/13984002/diff/31001/Source/core/platform/grap...
Source/core/platform/graphics/filters/FilterEffect.h:202: typedef
Vector<RefPtr<FilterEffect> > FilterEffectList;
Nit: is this still used anywhere?

[achicu, 2013-04-22 20:19:55]

On 2013/04/22 20:10:04, Stephen White wrote:
>
https://codereview.chromium.org/13984002/diff/31001/Source/core/platform/grap...
> File Source/core/platform/graphics/filters/FilterEffect.h (right):
> 
>
https://codereview.chromium.org/13984002/diff/31001/Source/core/platform/grap...
> Source/core/platform/graphics/filters/FilterEffect.h:202: typedef
> Vector<RefPtr<FilterEffect> > FilterEffectList;
> Nit: is this still used anywhere?

Most probably not. I will remove it.

[achicu, 2013-04-22 20:22:18]

On 2013/04/22 20:01:48, Stephen White wrote:
> Sorry it took me so long to get to this.  This looks great.
> 
> LGTM.

Thanks!

> 
>
https://codereview.chromium.org/13984002/diff/31001/Source/core/platform/grap...
> File Source/core/platform/graphics/filters/FilterEffect.cpp (right):
> 
>
https://codereview.chromium.org/13984002/diff/31001/Source/core/platform/grap...
> Source/core/platform/graphics/filters/FilterEffect.cpp:207:
> m_inputEffects.at(i).get()->setMaxEffectRectRecursive(effectRect);
> This will be slightly wasteful where there are "diamond" patterns in the DAG,
> but I can live with that.  Looks like clearResultRecursive() has the same
> problem.

There will be a follow up patch to setup the rect subregion coordinates
correctly on the SVG  filters. Currently that part of the SVG filters referenced
from CSS is not working.

I will need to figure out how that is going to work for the Skia composited
graph too. Is there a way to clip the input of a SkiaFilter to just a part of
it?

[achicu, 2013-04-22 20:25:09]

BTW, can you take a look at https://codereview.chromium.org/13853009/ too? 

I will need to rebase it to the new "core" folder, but that should be
straight-forward change.

[Stephen White, 2013-04-22 20:25:12]

On 2013/04/22 20:22:18, achicu wrote:

> There will be a follow up patch to setup the rect subregion coordinates
> correctly on the SVG  filters. Currently that part of the SVG filters
referenced
> from CSS is not working.

That would be fantastic.  http://crbug.com/229910 is logged for this on the
blink side.

> I will need to figure out how that is going to work for the Skia composited
> graph too. Is there a way to clip the input of a SkiaFilter to just a part of
> it?

Not yet.  We have plans for it, but haven't gotten to it yet.