Bug 45812

Created attachment 67655 [details] Draft patch

Comment on attachment 67655 [details] Draft patch View in context: https://bugs.webkit.org/attachment.cgi?id=67655&action=prettypatch > WebCore/rendering/RenderSVGResourceFilterPrimitive.h:39 > + RenderSVGResourceFilterPrimitive(SVGFilterPrimitiveStandardAttributes* filterPrimitiveElement); You should omit the argument name here, it's redundant. > WebCore/svg/graphics/filters/SVGFilterBuilder.cpp:77 > + return effect; I'd prefer a return 0 here. > WebCore/svg/graphics/filters/SVGFilterBuilder.cpp:85 > + if (m_lastEffectReference1.get() != effect) No need for the .get() here. > WebCore/svg/graphics/filters/SVGFilterBuilder.cpp:91 > + if (m_lastEffectReference1.get() == effect || m_lastEffectReference2.get() == effect) Ditto. RefPtr defines operator*. > WebCore/svg/graphics/filters/SVGFilterBuilder.cpp:95 > + if (m_lastEffectFurtherReferences[i].get() == effect) Ditto. > WebCore/svg/graphics/filters/SVGFilterBuilder.cpp:105 > + ASSERT(m_effectReferences.find(effect) == m_effectReferences.end()); ASSERT(!m_effectReferences.contains(effect)): ? > WebCore/svg/graphics/filters/SVGFilterBuilder.cpp:108 > + if (m_lastEffectReference1.get()) You can omit the .get() > WebCore/svg/graphics/filters/SVGFilterBuilder.cpp:110 > + if (m_lastEffectReference2.get()) Ditto. > WebCore/svg/graphics/filters/SVGFilterBuilder.cpp:112 > + if (m_lastEffectFurtherReferences.size()) { Use !isEmpty() > WebCore/svg/graphics/filters/SVGFilterBuilder.cpp:128 > + ASSERT(!m_lastEffectFurtherReferences.size()); ASSERT(m_last..isEmpty()) > WebCore/svg/graphics/filters/SVGFilterBuilder.h:46 > FilterEffect* getEffectById(const AtomicString& id) const; The id parameter is redundant. > WebCore/svg/graphics/filters/SVGFilterBuilder.h:48 > + FilterEffect* getAndRecordEffectById(const AtomicString& id); Ditto. > WebCore/svg/graphics/filters/SVGFilterBuilder.h:55 > + ASSERT(m_effectReferences.find(effect) != m_effectReferences.end()); ASSERT(m_effectReferences.contains(...) > WebCore/svg/graphics/filters/SVGFilterBuilder.h:67 > + ASSERT(m_effectReferences.find(effect) != m_effectReferences.end()); ASSERT(m_effectReferences.contains(...) > WebCore/svg/graphics/filters/SVGFilterBuilder.h:85 > + RefPtr<FilterEffect> m_lastEffectReference1; It's not immediately clear to me, why those have to be refcounted at all. I'll leave the actual review to Dirk, just wanted to share some notes.

Created attachment 67671 [details] Draft patch 2 Based on discussion on #webkit

Comment on attachment 67671 [details] Draft patch 2 View in context: https://bugs.webkit.org/attachment.cgi?id=67671&action=prettypatch > WebCore/svg/graphics/filters/SVGFilterBuilder.cpp:102 > +void SVGFilterBuilder::appendEffectToEffectReferences(RefPtr<FilterEffect> effect) > +{ > + // The effect must be a newly created filter effect. > + ASSERT(!m_effectReferences.contains(effect)); > + m_effectReferences.add(effect, EffectReferences()); > + > + int size = m_recordedEffects.size(); > + for (int i = 0; i < size; ++i) { > + // Since targetEffect is a newly created object, it cannot already be added to the list. > + ASSERT(m_effectReferences.contains(m_recordedEffects[i])); > + m_effectReferences.find(m_recordedEffects[i])->second.append(effect); > + } > + m_recordedEffects.clear(); > +} > + I won't review the code like Niko right now. I'm more thinking about the process itself. At the moment you record every request of filter effects in m_recordedEffects. Create the new effect and add the new created effect to the requester list of the requested effects afterwards (compilcated phrasing, I know ;-)). I wounder if it won't be easier and more understandable, if we skip the recording of the requested effects and ask the new created effect directly for its requested effects. This may cause us to let effects store their input effects in a Vector. Not sure if this is a noticeable performance or memory waste. But it could help to make the layout of primitives much easier. When you look at the current FilterEffect code, we have a lot of functions to determine the subregion, just because filter effects have different counts of input effects. We could simplify the code a lot if we allways use a vector for input effects. This would also mean that we can get rid of a lot of code in the FE* objects. IIRC Firefox is going the same way. What do you think?

I added a patch for the input effect Vector to bug 45612.

Created attachment 68354 [details] Draft patch 3 Updated patch

Created attachment 68357 [details] Draft patch 4

Comment on attachment 68357 [details] Draft patch 4 View in context: https://bugs.webkit.org/attachment.cgi?id=68357&action=review Please add a ChangeLog with detailed descriptions next time. The patch looks great and I'm looking forward to see the next patch. > WebCore/svg/graphics/filters/SVGFilterBuilder.cpp:89 > +void SVGFilterBuilder::appendEffectToEffectReferences(RefPtr<FilterEffect> effect) > +{ > + // The effect must be a newly created filter effect. > + ASSERT(!m_effectReferences.contains(effect)); > + m_effectReferences.add(effect, FilterEffectVector()); > + > + FilterEffectVector& inputEffects = effect->inputEffects(); > + HashSet<RefPtr<FilterEffect> > seenEffects; > + int size = inputEffects.size(); > + > + for (int i = 0; i < size; ++i) { > + // Since targetEffect is a newly created object, it cannot already be added to the list. > + ASSERT(m_effectReferences.contains(inputEffects[i])); > + if (seenEffects.contains(inputEffects[i])) > + continue; > + m_effectReferences.find(inputEffects[i])->second.append(effect); > + seenEffects.add(inputEffects[i]); > + } > +} I'm not sure why you want to have a FilterEffectVector instead of a Set? At the moment you create a set for every call of appendEffectToEffectReferences. Wouldn't it be more useful to put a set into m_effectReferences: HashMap<RefPtr<FilterEffect>, Set<RefPtr<FilterEffect> > > ? So you just need check the set, if the effect is already in there: contains(effect), and it if not. Niko already asked it, do we need the RefPtr's? Can't we just store the pointer? > WebCore/svg/graphics/filters/SVGFilterBuilder.h:51 > + inline FilterEffectVector& getReferences(FilterEffect* effect) > + { > + // Only allowed for effects belongs to this builder. > + ASSERT(m_effectReferences.contains(effect)); > + return m_effectReferences.find(effect)->second; > + } Who is using that? Also this one liner don't need to be in an extra function. > WebCore/svg/graphics/filters/SVGFilterBuilder.h:63 > + inline void addBuiltinEffects() > + { > + HashMap<AtomicString, RefPtr<FilterEffect> >::iterator end = m_builtinEffects.end(); > + for (HashMap<AtomicString, RefPtr<FilterEffect> >::iterator iterator = m_builtinEffects.begin(); iterator != end; ++iterator) > + m_effectReferences.add(iterator->second, FilterEffectVector()); > + } You should call this during initialization. Not sure if we get a leak if we leaf it in on clearEffects and call the DTor of FilterBuilder.

> > WebCore/svg/graphics/filters/SVGFilterBuilder.cpp:89 > > +void SVGFilterBuilder::appendEffectToEffectReferences(RefPtr<FilterEffect> effect) > > +{ > > + // The effect must be a newly created filter effect. > > + ASSERT(!m_effectReferences.contains(effect)); > > + m_effectReferences.add(effect, FilterEffectVector()); > > + > > + FilterEffectVector& inputEffects = effect->inputEffects(); > > + HashSet<RefPtr<FilterEffect> > seenEffects; > > + int size = inputEffects.size(); > > + > > + for (int i = 0; i < size; ++i) { > > + // Since targetEffect is a newly created object, it cannot already be added to the list. > > + ASSERT(m_effectReferences.contains(inputEffects[i])); > > + if (seenEffects.contains(inputEffects[i])) > > + continue; > > + m_effectReferences.find(inputEffects[i])->second.append(effect); > > + seenEffects.add(inputEffects[i]); > > + } > > +} > > I'm not sure why you want to have a FilterEffectVector instead of a Set? At the moment you create a set for every call of appendEffectToEffectReferences. Wouldn't it be more useful to put a set into m_effectReferences: > HashMap<RefPtr<FilterEffect>, Set<RefPtr<FilterEffect> > > ? So you just need check the set, if the effect is already in there: contains(effect), and it if not. Niko already asked it, do we need the RefPtr's? Can't we just store the pointer? If we would use a 'set', we would also create a new 'set' for each effect: + m_effectReferences.add(effect, FilterEffectVector()); would be + m_effectReferences.add(effect, set()); Using a vector here is a speed optimization, since iterating through a vector (needed by the 'invalidation' process) is much cheaper than iterating through a set. Moreover, we only append items during initialization, but invalidation will use the iteration many times. > > WebCore/svg/graphics/filters/SVGFilterBuilder.h:51 > > + inline FilterEffectVector& getReferences(FilterEffect* effect) > > + { > > + // Only allowed for effects belongs to this builder. > > + ASSERT(m_effectReferences.contains(effect)); > > + return m_effectReferences.find(effect)->second; > > + } > > Who is using that? Also this one liner don't need to be in an extra function. Nobody at the moment, but it will be needed in the future, I am sure. > > WebCore/svg/graphics/filters/SVGFilterBuilder.h:63 > > + inline void addBuiltinEffects() > > + { > > + HashMap<AtomicString, RefPtr<FilterEffect> >::iterator end = m_builtinEffects.end(); > > + for (HashMap<AtomicString, RefPtr<FilterEffect> >::iterator iterator = m_builtinEffects.begin(); iterator != end; ++iterator) > > + m_effectReferences.add(iterator->second, FilterEffectVector()); > > + } > > You should call this during initialization. Not sure if we get a leak if we leaf it in on clearEffects and call the DTor of FilterBuilder. It is called from the constructor: SVGFilterBuilder::SVGFilterBuilder() { m_builtinEffects.add(SourceGraphic::effectName(), SourceGraphic::create()); m_builtinEffects.add(SourceAlpha::effectName(), SourceAlpha::create()); addBuiltinEffects(); } What do you mean by 'initialization'? I am agree with the other things.

(In reply to comment #9) > > > WebCore/svg/graphics/filters/SVGFilterBuilder.cpp:89 > > > +void SVGFilterBuilder::appendEffectToEffectReferences(RefPtr<FilterEffect> effect) > > > +{ > > > + // The effect must be a newly created filter effect. > > > + ASSERT(!m_effectReferences.contains(effect)); > > > + m_effectReferences.add(effect, FilterEffectVector()); > > > + > > > + FilterEffectVector& inputEffects = effect->inputEffects(); > > > + HashSet<RefPtr<FilterEffect> > seenEffects; > > > + int size = inputEffects.size(); > > > + > > > + for (int i = 0; i < size; ++i) { > > > + // Since targetEffect is a newly created object, it cannot already be added to the list. > > > + ASSERT(m_effectReferences.contains(inputEffects[i])); > > > + if (seenEffects.contains(inputEffects[i])) > > > + continue; > > > + m_effectReferences.find(inputEffects[i])->second.append(effect); > > > + seenEffects.add(inputEffects[i]); > > > + } > > > +} > > > > I'm not sure why you want to have a FilterEffectVector instead of a Set? At the moment you create a set for every call of appendEffectToEffectReferences. Wouldn't it be more useful to put a set into m_effectReferences: > > HashMap<RefPtr<FilterEffect>, Set<RefPtr<FilterEffect> > > ? So you just need check the set, if the effect is already in there: contains(effect), and it if not. Niko already asked it, do we need the RefPtr's? Can't we just store the pointer? > > If we would use a 'set', we would also create a new 'set' for each effect: > + m_effectReferences.add(effect, FilterEffectVector()); > would be > + m_effectReferences.add(effect, set()); Your're doing it anyway two lines later. And it doesn't matter if you check it with you temporary set or with the set of EffectReferences. Or do I get something wrong? Later you want to update the ReferenceList and it makes no sense to write your own iterator for the vector, if you could just call contains(..) > > Using a vector here is a speed optimization, since iterating through a vector (needed by the 'invalidation' process) is much cheaper than iterating through a set. Moreover, we only append items during initialization, but invalidation will use the iteration many times. > > > > WebCore/svg/graphics/filters/SVGFilterBuilder.h:51 > > > + inline FilterEffectVector& getReferences(FilterEffect* effect) > > > + { > > > + // Only allowed for effects belongs to this builder. > > > + ASSERT(m_effectReferences.contains(effect)); > > > + return m_effectReferences.find(effect)->second; > > > + } > > > > Who is using that? Also this one liner don't need to be in an extra function. > > Nobody at the moment, but it will be needed in the future, I am sure. Please add it at this time when we need it. > > > > WebCore/svg/graphics/filters/SVGFilterBuilder.h:63 > > > + inline void addBuiltinEffects() > > > + { > > > + HashMap<AtomicString, RefPtr<FilterEffect> >::iterator end = m_builtinEffects.end(); > > > + for (HashMap<AtomicString, RefPtr<FilterEffect> >::iterator iterator = m_builtinEffects.begin(); iterator != end; ++iterator) > > > + m_effectReferences.add(iterator->second, FilterEffectVector()); > > > + } > > > > You should call this during initialization. Not sure if we get a leak if we leaf it in on clearEffects and call the DTor of FilterBuilder. > > It is called from the constructor: > > SVGFilterBuilder::SVGFilterBuilder() > { > m_builtinEffects.add(SourceGraphic::effectName(), SourceGraphic::create()); > m_builtinEffects.add(SourceAlpha::effectName(), SourceAlpha::create()); > addBuiltinEffects(); > } > > What do you mean by 'initialization'? Exactly that :-) But we call clearFilterBuilder in the destructor. And clearFilterBuilder calls your function that adds the pseudo effects. That needs to change somehow. Not sure why we call clear on RenderSVGResourceFilter right after creating the Builder. Maybe that should be removed as well. > > I am agree with the other things.

Created attachment 68673 [details] patch

Comment on attachment 68673 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=68673&action=review Otherwise looks great! > WebCore/svg/graphics/filters/SVGFilterBuilder.cpp:82 > + m_effectReferences.add(effect, FilterEffectSet()); > + > + FilterEffectVector& inputEffects = effect->inputEffects(); > + int size = inputEffects.size(); > + > + // It is not possible to add the same value to a set twice. > + for (int i = 0; i < size; ++i) > + getEffectReferences(effect.get()).add(inputEffects[i].get()); Please use numberOfInputEffects and inputEffect(unsigned) instead of the EffectVector. Store the pointer effect.get() in a own variable. > WebCore/svg/graphics/filters/SVGFilterBuilder.cpp:90 > + addBuiltinEffects(); Please remove this from clearEffects().

Created attachment 68896 [details] Patch 2

Comment on attachment 68896 [details] Patch 2 View in context: https://bugs.webkit.org/attachment.cgi?id=68896&action=review Great ptach! Please change the two issues above before landing. r=me. > WebCore/svg/graphics/filters/SVGFilterBuilder.cpp:78 > + int numberOfInputEffects = effect->inputEffects().size(); should be unsigned > WebCore/svg/graphics/filters/SVGFilterBuilder.cpp:81 > + for (int i = 0; i < numberOfInputEffects; ++i) ditto.

Landed in http://trac.webkit.org/changeset/68385 Closing bug