Bug 68472 – Move/copy/merge SVG filter rendering code to generic rendering

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Dean Jackson]

This will allow filter operations to render HTML elements.

[Radar WebKit Bug Importer]

<rdar://problem/10155660>

[Chris Marrin]

At the moment SVGRenderSupport::prepareToRenderSVGContent and SVGRenderSupport::finishRenderSVGContent are the places that call into the generic filter stuff. This bug is about moving that out to RenderLayer or wherever.

[Dean Jackson]

Created attachment 111693 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 111693 [details]
Patch

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

I think there are two big chunks missing here, and I'm curious about how you plan to implement them:
1. incremental repaints: you need to map a rect through the filter chain. Filters need the ability to say how they affect a given rect. This rect mapping should be used in the repaint code path.
2. How filters expand the visual overflow of an element (also repaint-related).

> Source/WebCore/rendering/RenderFilter.h:44
> +class RenderFilter : public Filter {

I'm not sure that RenderFilter is the best name for this, since it doesn't inherit from RenderObject. Maybe RendererFilter? EffectsFilter?

> Source/WebCore/rendering/RenderFilter.h:52
> +    virtual void setSourceImageRect(const FloatRect& sourceImageRect) { m_sourceDrawingRegion = sourceImageRect; }
> +    virtual FloatRect sourceImageRect() const { return m_sourceDrawingRegion; }

Should these all be LayoutRects?

> Source/WebCore/rendering/RenderFilter.h:57
> +    virtual bool effectBoundingBoxMode() const { return false; }

This method has a terrible name. What kind of mode is this?

> Source/WebCore/rendering/RenderLayer.cpp:2696
> +        m_filter->setSavedContext(p);

Why does the filter have to know what the original destination context is?

> Source/WebCore/rendering/RenderLayer.cpp:2709
> +        p->save();
> +        p->concatCTM(transform.toAffineTransform());

I don't really like the save here, and the restore in some other code block a long way below.

Can you do what we do for transforms, and call back into paintLayer?

Does this filter before or after CSS shadows are applied? Which should it be?

> Source/WebCore/rendering/RenderLayer.cpp:2875
> +        p->drawImageBuffer(m_filter->effect()->asImageBuffer(), renderer()->style()->colorSpace(), layerBounds, CompositeSourceOver);

Doesn't the color space need to be passed through the filter chain somehow?

> Source/WebCore/rendering/RenderLayer.cpp:4385
> +void RenderLayer::createFilter()
> +{
> +    ASSERT(!m_filter);
> +    m_filter = RenderFilter::create();
> +}
> +
> +void RenderLayer::removeFilter()
> +{
> +    m_filter = 0;
> +}
> +

It's not clear to me that RenderLayer is the right thing to own the filter. Maybe the RenderObject should own it?

> Source/WebCore/rendering/RenderLayer.cpp:4398
> +    // For the moment all filter effects are hardcoded to hue-rotate(180deg).
> +    // See https://bugs.webkit.org/show_bug.cgi?id=68474 and
> +    // https://bugs.webkit.org/show_bug.cgi?id=68475
> +
> +    RefPtr<SourceGraphic> sourceGraphic = SourceGraphic::create(m_filter.get());
> +    m_filter->setSourceGraphic(sourceGraphic);
> +    Vector<float> inputParameters;
> +    inputParameters.append(180);
> +    RefPtr<FEColorMatrix> matrix = FEColorMatrix::create(m_filter.get(), FECOLORMATRIX_TYPE_HUEROTATE, inputParameters);
> +    matrix->inputEffects().append(sourceGraphic);
> +    m_filter->setEffect(matrix);

I don't think it's appropriate to check in a hard-coded filter effect.

> Source/WebCore/rendering/RenderObject.h:926
> +    bool m_hasFilter                 : 1;

See comment higher up:
    // 32 bits have been used here. THERE ARE NO FREE BITS AVAILABLE.

[Chris Marrin]

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

Just a few nits. Overall looks good...

> Source/WebCore/rendering/RenderFilter.h:57
> +    virtual bool effectBoundingBoxMode() const { return false; }

I'm not sure what this mode does. Maybe a comment?

> Source/WebCore/rendering/RenderFilter.h:69
> +    RenderFilter();

Don't at least some compilers require an explicit virtual dtor if you have virtual methods?

> Source/WebCore/rendering/RenderInline.cpp:127
> +    setHasReflection(false);

What's the diff here?

> Source/WebCore/rendering/RenderLayer.cpp:4232
> +    if (!hasFilter() && m_filter)

I've always disliked testing a variable directly and then using a function to change it. Maybe just get rid of the "&& m_filter" and do and early exit from removeFilter() if it's null?"

> Source/WebCore/rendering/RenderLayer.cpp:4235
> +        if (!m_filter)

Same here

> Source/WebCore/rendering/RenderLayer.cpp:4388
> +    // For the moment all filter effects are hardcoded to hue-rotate(180deg).

This should be a FIXME

> Source/WebCore/rendering/RenderLayer.cpp:4396
> +    RefPtr<FEColorMatrix> matrix = FEColorMatrix::create(m_filter.get(), FECOLORMATRIX_TYPE_HUEROTATE, inputParameters);

Does the HueRotate filter expect its angle in degrees? It doesn't allow units?

> Source/WebCore/rendering/RenderLayer.cpp:4411
> +            m_filter->setSourceImage(ImageBuffer::create(layoutSize));

It seems odd to set these 4 values here, when all you're doing is updating the value of sourceImageRect. It would be better if you just set sourceImageRect and let it set the other 3.

[Dean Jackson]

(In reply to comment #4)
> (From update of attachment 111693 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=111693&action=review
> 
> I think there are two big chunks missing here, and I'm curious about how you plan to implement them:
> 1. incremental repaints: you need to map a rect through the filter chain. Filters need the ability to say how they affect a given rect. This rect mapping should be used in the repaint code path.

Yep. We can do that, but obviously not with the patch I submitted. We'll have to get the Filters to tell us what the output region is so we can expand the overflow.

> 2. How filters expand the visual overflow of an element (also repaint-related).

Each filter effect will need to tell us what the output region is for a given input. I think that's easy for the shorthand filters as only blur expands the region and we know by how much. For generic SVG, we'll have a trickier time, and some filters have infinite output (e.g. flood and turbulence). 

> > Source/WebCore/rendering/RenderFilter.h:44
> > +class RenderFilter : public Filter {
> 
> I'm not sure that RenderFilter is the best name for this, since it doesn't inherit from RenderObject. Maybe RendererFilter? EffectsFilter?

Good point.

> 
> > Source/WebCore/rendering/RenderFilter.h:52
> > +    virtual void setSourceImageRect(const FloatRect& sourceImageRect) { m_sourceDrawingRegion = sourceImageRect; }
> > +    virtual FloatRect sourceImageRect() const { return m_sourceDrawingRegion; }
> 
> Should these all be LayoutRects?

Sure.
 
> > Source/WebCore/rendering/RenderFilter.h:57
> > +    virtual bool effectBoundingBoxMode() const { return false; }
> 
> This method has a terrible name. What kind of mode is this?

TBH, I haven't even looked. I'll follow the virtual function and see what it actually does. Maybe it is something we can use.

> 
> > Source/WebCore/rendering/RenderLayer.cpp:2696
> > +        m_filter->setSavedContext(p);
> 
> Why does the filter have to know what the original destination context is?

It doesn't. It should just be a local variable in the paint.

> 
> > Source/WebCore/rendering/RenderLayer.cpp:2709
> > +        p->save();
> > +        p->concatCTM(transform.toAffineTransform());
> 
> I don't really like the save here, and the restore in some other code block a long way below.
> 
> Can you do what we do for transforms, and call back into paintLayer?

OK.

> 
> Does this filter before or after CSS shadows are applied? Which should it be?

It filters after shadows.

> 
> > Source/WebCore/rendering/RenderLayer.cpp:2875
> > +        p->drawImageBuffer(m_filter->effect()->asImageBuffer(), renderer()->style()->colorSpace(), layerBounds, CompositeSourceOver);
> 
> Doesn't the color space need to be passed through the filter chain somehow?

Yes, but not in this hardcoded patch. 

> 
> > Source/WebCore/rendering/RenderLayer.cpp:4385
> > +void RenderLayer::createFilter()
> > +{
> > +    ASSERT(!m_filter);
> > +    m_filter = RenderFilter::create();
> > +}
> > +
> > +void RenderLayer::removeFilter()
> > +{
> > +    m_filter = 0;
> > +}
> > +
> 
> It's not clear to me that RenderLayer is the right thing to own the filter. Maybe the RenderObject should own it?

OK.

> 
> I don't think it's appropriate to check in a hard-coded filter effect.

I just didn't want to have this patch include the code for building a filter graph. Maybe it could start with only supporting hue-rotate? (or invert, that's an easy enough one)

> 
> > Source/WebCore/rendering/RenderObject.h:926
> > +    bool m_hasFilter                 : 1;
> 
> See comment higher up:
>     // 32 bits have been used here. THERE ARE NO FREE BITS AVAILABLE.

Yikes.

[Dean Jackson]

(In reply to comment #5)
> View in context: https://bugs.webkit.org/attachment.cgi?id=111693&action=review
> 
> Just a few nits. Overall looks good...
> 
> > Source/WebCore/rendering/RenderFilter.h:57
> > +    virtual bool effectBoundingBoxMode() const { return false; }
> 
> I'm not sure what this mode does. Maybe a comment?

I don't know what it does either :)

> 
> > Source/WebCore/rendering/RenderFilter.h:69
> > +    RenderFilter();
> 
> Don't at least some compilers require an explicit virtual dtor if you have virtual methods?


> 
> > Source/WebCore/rendering/RenderInline.cpp:127
> > +    setHasReflection(false);
> 
> What's the diff here?

Spaces at the end of the line that the stylebot was complaining about.

> 
> > Source/WebCore/rendering/RenderLayer.cpp:4232
> > +    if (!hasFilter() && m_filter)
> 
> I've always disliked testing a variable directly and then using a function to change it. Maybe just get rid of the "&& m_filter" and do and early exit from removeFilter() if it's null?"

OK.

> 
> > Source/WebCore/rendering/RenderLayer.cpp:4235
> > +        if (!m_filter)
> 
> Same here
> 
> > Source/WebCore/rendering/RenderLayer.cpp:4388
> > +    // For the moment all filter effects are hardcoded to hue-rotate(180deg).
> 
> This should be a FIXME

OK.

> 
> > Source/WebCore/rendering/RenderLayer.cpp:4396
> > +    RefPtr<FEColorMatrix> matrix = FEColorMatrix::create(m_filter.get(), FECOLORMATRIX_TYPE_HUEROTATE, inputParameters);
> 
> Does the HueRotate filter expect its angle in degrees? It doesn't allow units?

Correct.

> 
> > Source/WebCore/rendering/RenderLayer.cpp:4411
> > +            m_filter->setSourceImage(ImageBuffer::create(layoutSize));
> 
> It seems odd to set these 4 values here, when all you're doing is updating the value of sourceImageRect. It would be better if you just set sourceImageRect and let it set the other 3.

OK

[Dean Jackson]

(In reply to comment #6)

> > I think there are two big chunks missing here, and I'm curious about how you plan to implement them:
> > 1. incremental repaints: you need to map a rect through the filter chain. Filters need the ability to say how they affect a given rect. This rect mapping should be used in the repaint code path.
> 
> Yep. We can do that, but obviously not with the patch I submitted. We'll have to get the Filters to tell us what the output region is so we can expand the overflow.

I don't want to do that in this patch. 

> > 2. How filters expand the visual overflow of an element (also repaint-related).
> 
> Each filter effect will need to tell us what the output region is for a given input. I think that's easy for the shorthand filters as only blur expands the region and we know by how much. For generic SVG, we'll have a trickier time, and some filters have infinite output (e.g. flood and turbulence). 

Ditto. At the moment we don't change the element dimensions.

> > > +class RenderFilter : public Filter {
> > 
> > I'm not sure that RenderFilter is the best name for this, since it doesn't inherit from RenderObject. Maybe RendererFilter? EffectsFilter?

FilterEffectsRenderer. I bet we'll come up with a better name soon.

> > > +    virtual void setSourceImageRect(const FloatRect& sourceImageRect) { m_sourceDrawingRegion = sourceImageRect; }
> > > +    virtual FloatRect sourceImageRect() const { return m_sourceDrawingRegion; }
> > 
> > Should these all be LayoutRects?
> 
> Sure.

They can't. These are the virtual methods from FilterEffect.


> > Does this filter before or after CSS shadows are applied? Which should it be?
> 
> It filters after shadows.

Right now filtering comes after everything (including outline, shadows, reflections, masking). We'll coordinate on www-style as to where it really should happen.


> > I don't think it's appropriate to check in a hard-coded filter effect.
> 
> I just didn't want to have this patch include the code for building a filter graph. Maybe it could start with only supporting hue-rotate? (or invert, that's an easy enough one)

No longer hardcoded, but it still only supports hue-rotate.

> > > Source/WebCore/rendering/RenderObject.h:926
> > > +    bool m_hasFilter                 : 1;
> > 
> > See comment higher up:
> >     // 32 bits have been used here. THERE ARE NO FREE BITS AVAILABLE.
> 
> Yikes.

Moved this to a method on RenderStyle.

[Dean Jackson]

Created attachment 114179 [details]
Patch

[Dean Jackson]

Created attachment 114331 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 114331 [details]
Patch

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

> Source/WebCore/WebCore.vcproj/WebCore.vcproj:33261
> +					ExcludedFromBuild="true"

I don't think you want this here.

> Source/WebCore/rendering/FilterEffectRenderer.h:69
> +    virtual void setSourceGraphic(PassRefPtr<SourceGraphic> sourceGraphic) { m_sourceGraphic = sourceGraphic; }
> +    virtual RefPtr<SourceGraphic> sourceGraphic() const { return m_sourceGraphic; }

I don't get why the input isn't an ImageBuffer, but that's just me.

> Source/WebCore/rendering/RenderLayer.cpp:2691
> +        GraphicsContext* sourceGraphicsContext = m_filter->sourceImage()->context();

This all seems rather weird. I filed bug 71962.

> Source/WebCore/rendering/RenderLayer.cpp:2693
> +        FloatRect paintRect = FloatRect(0, 0, size().width(), size().height());

FloatRect paintRect(FloatPoint(), size()).
Should this be in LayoutRects?

> Source/WebCore/rendering/RenderLayer.cpp:2706
> +        LayoutRect filterBounds = LayoutRect(layerOrigin.x(), layerOrigin.y(), size().width(), size().height());

filterBounds(layerOrigin(), size())?

> Source/WebCore/rendering/RenderLayer.cpp:2709
> +        p->drawImageBuffer(m_filter->effect()->asImageBuffer(), renderer()->style()->colorSpace(), filterBounds, CompositeSourceOver);

Should we clear the filter here? Does that throw away buffers?

> Source/WebCore/rendering/RenderLayer.cpp:4244
> +    if (hasFilter())
> +        updateFilterEffect();

You could have updateFilterEffect() just do the hasFilter() test.

> Source/WebCore/rendering/RenderLayer.cpp:4393
> +    // For the moment we only support a single hue-rotate() filter.
> +    // See https://bugs.webkit.org/show_bug.cgi?id=68474 and
> +    // https://bugs.webkit.org/show_bug.cgi?id=68475
> +
> +    RefPtr<FilterEffect> effect;
> +    FilterOperations filterOperations = renderer()->style()->filter();

It would be nice to be able to optimize this to do nothing if the filter style hasn't changed.

> Source/WebCore/rendering/RenderLayer.cpp:4416
> +void RenderLayer::updateFilterBackingStore()
> +{

Seems pretty bogus to have to do this every time. Maybe covered by bug 71962.

> LayoutTests/css3/filters/add-filter-rendering-expected.txt:9
> +      text run at (0,0) width 572: "This paragraph will have a filter applied. Once applied, it should have a green background."

Try to avoid text in pixel results. You could put the text in an HTML comment.

[Dean Jackson]

(In reply to comment #11)
> (From update of attachment 114331 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=114331&action=review
> 
> > Source/WebCore/WebCore.vcproj/WebCore.vcproj:33261
> > +					ExcludedFromBuild="true"
> 
> I don't think you want this here.

I added the file to the AllInOne when committing, so left this in.


> > Source/WebCore/rendering/RenderLayer.cpp:2693
> > +        FloatRect paintRect = FloatRect(0, 0, size().width(), size().height());
> 
> FloatRect paintRect(FloatPoint(), size()).
> Should this be in LayoutRects?

The filter code expects FloatRects.

> You could have updateFilterEffect() just do the hasFilter() test.

Done

> It would be nice to be able to optimize this to do nothing if the filter style hasn't changed.

Yeah.

> Try to avoid text in pixel results. You could put the text in an HTML comment.

Done.

[Dean Jackson]

http://trac.webkit.org/changeset/99893

[Nikolas Zimmermann]

Comment on attachment 114331 [details]
Patch

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

Nice start Dean! Just a quick note regarding the Filter class:

> Source/WebCore/rendering/FilterEffectRenderer.h:63
> +    virtual bool effectBoundingBoxMode() const { return false; }

I removed this today from Filter.h, you can remove this function.

[Dean Jackson]

(In reply to comment #14)
> (From update of attachment 114331 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=114331&action=review
> 
> Nice start Dean! Just a quick note regarding the Filter class:

Small steps, but we'll get there.

> > Source/WebCore/rendering/FilterEffectRenderer.h:63
> > +    virtual bool effectBoundingBoxMode() const { return false; }
> 
> I removed this today from Filter.h, you can remove this function.

Excellent. I'll do that.

Dean