| Summary: | Implement hardware animation of CSS filters | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Chris Marrin <cmarrin> | ||||||||
| Component: | Layout and Rendering | Assignee: | Chris Marrin <cmarrin> | ||||||||
| Status: | RESOLVED FIXED | ||||||||||
| Severity: | Normal | CC: | dino | ||||||||
| Priority: | P2 | Keywords: | InRadar | ||||||||
| Version: | 528+ (Nightly build) | ||||||||||
| Hardware: | Unspecified | ||||||||||
| OS: | Unspecified | ||||||||||
| Attachments: |
|
||||||||||
|
Description
Chris Marrin
2012-02-08 14:16:15 PST
InRadar <rdar://problem/10422163> Created attachment 126198 [details]
Patch
Created attachment 126206 [details]
Patch fixing Windows build
Comment on attachment 126206 [details] Patch fixing Windows build View in context: https://bugs.webkit.org/attachment.cgi?id=126206&action=review very minor comments. one confusion/error about the return type of the validate function. > Source/WebCore/page/animation/AnimationBase.cpp:1055 > gPropertyWrappers->append(new PropertyWrapper<const FilterOperations&>(CSSPropertyWebkitFilter, &RenderStyle::filter, &RenderStyle::setFilter)); > #endif > +#endif > + Seems like you added a blank line here by accident. > Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp:468 > size_t numAnimations = propertyAnimations.size(); > for (size_t i = 0; i < numAnimations; ++i) { > const LayerPropertyAnimation& currAnimation = propertyAnimations[i]; > - if (currAnimation.m_property == property) > + > + if (currAnimation.m_property == AnimatedPropertyWebkitTransform || currAnimation.m_property == AnimatedPropertyOpacity > + || currAnimation.m_property == AnimatedPropertyBackgroundColor > +#if ENABLE(CSS_FILTERS) > + || currAnimation.m_property == AnimatedPropertyWebkitFilter > +#endif > + ) question: this is now copying those four properties for the layers, but the calling sites previously didn't copy all four. Will there be a case where we're now copying something we didn't before, and will it cause a problem? > Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp:1398 > + moveOrCopyAnimations(Move, m_layer.get(), m_structuralLayer.get()); extra space > Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp:1863 > +bool GraphicsLayerCA::appendToUncommittedAnimations(const KeyframeValueList& valueList, const FilterOperations* operations, const Animation* animation, const String& animationName, int animationIndex, double timeOffset) Why not pass in the FilterOperation directly rather than FilterOperations and index? I see you need the index to generate the keypath, but maybe just that. > Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp:1865 > + bool additive = animationIndex > 0; Do these ever need to be additive? Aren't we always animating a different keypath? > Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp:1898 > + > +// get list of properties to animate for this operation (from PlatformCALayer) > +// for each property: > +// create kf or basic animation > +// animation name is propertyIdToString(valueList.property()) + "." + "filter_" + animationIndex + property > +// maybe get this from PlatformCALayer > +// setFilterAnimationKeyframes or setFilterAnimationEndpoints > +// need valueList for values, operations and animationIndex (or operation), property index > +// maybe need to ask PlatformCALayer to return values given an operation and index? > + I think you meant to remove this. If not, indentation is off. > Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp:1906 > + int listIndex = validateFilterOperations(valueList); validateFilterOperations returns a boolean - not an int. I guess this means you were always getting the 0th entry in the KeyframeValueList. > Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp:1918 > + if (!appendToUncommittedAnimations(valueList, operations, animation, animationName, animationIndex, timeOffset)) As above, why not pass in operations->operations().at(animationIndex) (and maybe the index)? > Source/WebCore/platform/graphics/ca/GraphicsLayerCA.h:187 > + PassRefPtr<PlatformCAAnimation> createBasicAnimation(const Animation*, const String& keyPath, bool additive); I guess you don't need keyPath here. > Source/WebCore/platform/graphics/ca/mac/PlatformCAAnimationMac.mm:451 > + if (propertyIndex < 3) { > + double multiplier = 1 - op->amount() * 2; > + value.adoptNS([[NSArray arrayWithObjects: > + [NSNumber numberWithDouble:invertConstants[propertyIndex][0] * multiplier], I'm confused here. What's with this *2 thing? And why do this when propertyIndex < 3? I'm also slightly confused about propertyIndex. I'm not sure what it means. OK. I've looked elsewhere and worked it out. It's the index into the number of animations needed for each effect. So it isn't anything to do with CSS properties. Maybe we could think of a better name? > Source/WebCore/platform/graphics/ca/mac/PlatformCAAnimationMac.mm:715 > + default: return ""; > + case 0: return "inputRVector"; > + case 1: return "inputGVector"; > + case 2: return "inputBVector"; > + } > + case FilterOperation::SATURATE: return "inputSaturation"; > + case FilterOperation::HUE_ROTATE: return "inputAngle"; > + case FilterOperation::INVERT: > + switch(propertyIndex) { > + default: return ""; > + case 0: return "inputRVector"; > + case 1: return "inputGVector"; > + case 2: return "inputBVector"; > + case 3: return "inputBiasVector"; Shouldn't default be last? > Source/WebCore/rendering/RenderLayer.h:527 > #if ENABLE(CSS_FILTERS) > + bool hasFilter() const { return renderer()->hasFilter(); } > virtual void filterNeedsRepaint(); hasFilter() might be able to go outside the ENABLE. I did the same for style->hasFilter. That saves the ugly code below. > Source/WebCore/rendering/RenderLayerBacking.cpp:1208 > + if (!hasOpacity && !hasTransform > +#if ENABLE(CSS_FILTERS) > + && !hasFilter > +#endif > + ) This is the ugly bit. I think we can do it outside the ENABLE. (In reply to comment #4) > (From update of attachment 126206 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=126206&action=review > > very minor comments. one confusion/error about the return type of the validate function. > > > Source/WebCore/page/animation/AnimationBase.cpp:1055 > > gPropertyWrappers->append(new PropertyWrapper<const FilterOperations&>(CSSPropertyWebkitFilter, &RenderStyle::filter, &RenderStyle::setFilter)); > > #endif > > +#endif > > + Fixed > > Seems like you added a blank line here by accident. > > > Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp:468 > > size_t numAnimations = propertyAnimations.size(); > > for (size_t i = 0; i < numAnimations; ++i) { > > const LayerPropertyAnimation& currAnimation = propertyAnimations[i]; > > - if (currAnimation.m_property == property) > > + > > + if (currAnimation.m_property == AnimatedPropertyWebkitTransform || currAnimation.m_property == AnimatedPropertyOpacity > > + || currAnimation.m_property == AnimatedPropertyBackgroundColor > > +#if ENABLE(CSS_FILTERS) > > + || currAnimation.m_property == AnimatedPropertyWebkitFilter > > +#endif > > + ) > > question: this is now copying those four properties for the layers, but the calling sites previously didn't copy all four. Will there be a case where we're now copying something we didn't before, and will it cause a problem? Before, it was always copying transform and opacity and in some cases background color. So now background color will always get copied and I think that's the only difference. I think the fact that it didn't copy background color in some places before was an oversight, so I made it consistent. But I am pretty sure that we don't currently animate background color ever, so all this code for background color is just future proofing. > > > Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp:1398 > > + moveOrCopyAnimations(Move, m_layer.get(), m_structuralLayer.get()); > > extra space Fixed > > > Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp:1863 > > +bool GraphicsLayerCA::appendToUncommittedAnimations(const KeyframeValueList& valueList, const FilterOperations* operations, const Animation* animation, const String& animationName, int animationIndex, double timeOffset) > > Why not pass in the FilterOperation directly rather than FilterOperations and index? I see you need the index to generate the keypath, but maybe just that. I could pass FilterOperation, but I'd still need the index. There are places where you need to index into another list of filter functions and the index is used for that. That's in addition to generating the keypath name. So I'll pass in the FilterOperation and keep the index. > > > Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp:1865 > > + bool additive = animationIndex > 0; > > Do these ever need to be additive? Aren't we always animating a different keypath? Right, that's code left over from the transform logic. Gone… > > > Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp:1898 > > + > > +// get list of properties to animate for this operation (from PlatformCALayer) > > +// for each property: > > +// create kf or basic animation > > +// animation name is propertyIdToString(valueList.property()) + "." + "filter_" + animationIndex + property > > +// maybe get this from PlatformCALayer > > +// setFilterAnimationKeyframes or setFilterAnimationEndpoints > > +// need valueList for values, operations and animationIndex (or operation), property index > > +// maybe need to ask PlatformCALayer to return values given an operation and index? > > + > > I think you meant to remove this. If not, indentation is off. Yep, it's gone > > > Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp:1906 > > + int listIndex = validateFilterOperations(valueList); > > validateFilterOperations returns a boolean - not an int. I guess this means you were always getting the 0th entry in the KeyframeValueList. Ack! You're right. That function is supposed to return the index of the first non-default keyframe. I'll fix the call and add a test for this to the test case. > > > Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp:1918 > > + if (!appendToUncommittedAnimations(valueList, operations, animation, animationName, animationIndex, timeOffset)) > > As above, why not pass in operations->operations().at(animationIndex) (and maybe the index)? Now passing in FilterOperation* and index. > > > Source/WebCore/platform/graphics/ca/GraphicsLayerCA.h:187 > > + PassRefPtr<PlatformCAAnimation> createBasicAnimation(const Animation*, const String& keyPath, bool additive); > > I guess you don't need keyPath here. You mean I shouldn't have the parameter name in the signature? I think it's not obvious the string is a keyPath without it. > > > Source/WebCore/platform/graphics/ca/mac/PlatformCAAnimationMac.mm:451 > > + if (propertyIndex < 3) { > > + double multiplier = 1 - op->amount() * 2; > > + value.adoptNS([[NSArray arrayWithObjects: > > + [NSNumber numberWithDouble:invertConstants[propertyIndex][0] * multiplier], > > I'm confused here. What's with this *2 thing? And why do this when propertyIndex < 3? I added documentation to filterValueForOperation explaining what is being animated and how this does the invert operation > > I'm also slightly confused about propertyIndex. I'm not sure what it means. Some filters need to animate multipl CI properties, like the ones that do ColorMatrix animation. So there is a 1 to many relationship between filters and animations. This param reflects that. In the logic about this, it asks PlatformCAAnimation how many properties need to be animated for a given filter and then it generates that many animations passing the right propertyIndex to that I added some documentation to GraphicsLayerCA::appendToUncommittedAnimations to explain it. > > OK. I've looked elsewhere and worked it out. It's the index into the number of animations needed for each effect. So it isn't anything to do with CSS properties. Maybe we could think of a better name? I've changed it to internalFilterPropertyIndex as requested > > > Source/WebCore/platform/graphics/ca/mac/PlatformCAAnimationMac.mm:715 > > + default: return ""; > > + case 0: return "inputRVector"; > > + case 1: return "inputGVector"; > > + case 2: return "inputBVector"; > > + } > > + case FilterOperation::SATURATE: return "inputSaturation"; > > + case FilterOperation::HUE_ROTATE: return "inputAngle"; > > + case FilterOperation::INVERT: > > + switch(propertyIndex) { > > + default: return ""; > > + case 0: return "inputRVector"; > > + case 1: return "inputGVector"; > > + case 2: return "inputBVector"; > > + case 3: return "inputBiasVector"; > > Shouldn't default be last? done > > > Source/WebCore/rendering/RenderLayer.h:527 > > #if ENABLE(CSS_FILTERS) > > + bool hasFilter() const { return renderer()->hasFilter(); } > > virtual void filterNeedsRepaint(); > > hasFilter() might be able to go outside the ENABLE. I did the same for style->hasFilter. That saves the ugly code below. > > > Source/WebCore/rendering/RenderLayerBacking.cpp:1208 > > + if (!hasOpacity && !hasTransform > > +#if ENABLE(CSS_FILTERS) > > + && !hasFilter > > +#endif > > + ) > > This is the ugly bit. I think we can do it outside the ENABLE. done Patch on the way Created attachment 126393 [details]
Patch with review changes, crash fixes and tests for more cases
Comment on attachment 126393 [details] Patch with review changes, crash fixes and tests for more cases View in context: https://bugs.webkit.org/attachment.cgi?id=126393&action=review very minor change requests. awesome! > Source/WebCore/ChangeLog:17 > + Reviewed by NOBODY (OOPS!). Needs newline. > Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp:1906 > + if (listIndex < 0) > + return false; > + > + const FilterOperations* operations = (listIndex >= 0) ? static_cast<const FilterAnimationValue*>(valueList.at(listIndex))->value() : 0; No longer need the ternary operation. > Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp:2220 > + // We toss the last tfArray value because it has to one shorter than the others. Nits: might as well say "timing function" rather than "tfArray value". And "has to *be* one". > Source/WebCore/platform/graphics/ca/mac/PlatformCAAnimationMac.mm:441 > + double t = 0; Nit: for consistency, you use amount above and below, but t here. Comment on attachment 126393 [details] Patch with review changes, crash fixes and tests for more cases Attachment 126393 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/11496149 Committed r107422: <http://trac.webkit.org/changeset/107422> |