Closed Bug 1332211 Opened 4 years ago Closed 4 years ago

Animation utility for compositor

Categories

(Core :: Graphics: WebRender, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox53 --- affected
firefox54 --- fixed

People

(Reporter: pchang, Assigned: pchang)

References

Details

(Whiteboard: [gfx-noted])

Attachments

(3 files)

For Quantum render project, we still need the OMTA but there is no 'layer' concept in parent(WebRenderBridgeParent). In order to share the code between gecko and quantum render, I would like to put animation related functions (without using layer) into animation utility for composition.

For example, I will move the following functions to gfx/layer/composition/AnimationComposition.
[1] Layer::SetAnimations
[2] SampleAnimationForEachNode
[3] SampleValue


[1]http://searchfox.org/mozilla-central/source/gfx/layers/Layers.cpp#454
[2]http://searchfox.org/mozilla-central/source/gfx/layers/composite/AsyncCompositionManager.cpp#696
[3]http://searchfox.org/mozilla-central/source/gfx/layers/composite/AsyncCompositionManager.cpp#585
Brian and Hiro, please let me know your thought about this and any naming suggestion for this utility, like
gfx/layer/composite/AnimationComposition or gfx/layer/composite/AnimationHelper.
Whiteboard: [gfx-noted]
Blocks: 1328231
Assignee: nobody → howareyou322
(In reply to Peter Chang[:pchang] from comment #1)
> Brian and Hiro, please let me know your thought about this and any naming
> suggestion for this utility, like
> gfx/layer/composite/AnimationComposition or
> gfx/layer/composite/AnimationHelper.

In layout XXUtils is popular name for such purpose but XXHelper seems to be popular in gfx (There are Helper and Helpers though). So, +1 for AnimationHelper.
AnimationComposition looks like it contains a real class.
I also prefer AnimationHelper or AnimationUtils.
Comment on attachment 8830631 [details]
Bug 1332211 - Move Layer::SetAnimation into AnimationHelper,

https://reviewboard.mozilla.org/r/107374/#review108502

Looks good to me,  but MozReview does not show moving lines at all, so I maybe miss something...
Anyway, thanks!

::: gfx/layers/AnimationHelper.h:7
(Diff revision 1)
> +#ifndef mozilla_animationhelper_h
> +#define mozilla_animationhelper_h

This should be mozilla_layers_AnimationHelper_h.

::: gfx/layers/AnimationHelper.h:12
(Diff revision 1)
> +#ifndef mozilla_animationhelper_h
> +#define mozilla_animationhelper_h
> +
> +#include "mozilla/ComputedTimingFunction.h" // for ComputedTimingFunction
> +#include "mozilla/TimeStamp.h"          // for TimeStamp
> +#include "mozilla/StyleAnimationValue.h" // for StyleAnimationValue, etc

Use forward declaration instead of including this header.

::: gfx/layers/AnimationHelper.h:15
(Diff revision 1)
> +#include "mozilla/ComputedTimingFunction.h" // for ComputedTimingFunction
> +#include "mozilla/TimeStamp.h"          // for TimeStamp
> +#include "mozilla/StyleAnimationValue.h" // for StyleAnimationValue, etc
> +
> +namespace mozilla {
> +class ComputedTimingFunction;

I wonder if this forward declaration is necessary.

::: gfx/layers/AnimationHelper.h:20
(Diff revision 1)
> +class AnimationData;
> +class TimingFunction;

These two classes are necessary?

::: gfx/layers/AnimationHelper.h:33
(Diff revision 1)
> +  InfallibleTArray<Maybe<mozilla::ComputedTimingFunction>> mFunctions;
> +};
> +
> +class AnimationHelper
> +{
> +

Nit: drop a blank line here.

::: gfx/layers/AnimationHelper.h:38
(Diff revision 1)
> +
> +public:
> +  static void
> +  SetAnimations(AnimationArray& aAnimations,
> +                InfallibleTArray<AnimData>& aAnimData,
> +                StyleAnimationValue& aValue);

Please rename |aValue| to |aBaseAnimationStyle|.

::: gfx/layers/AnimationHelper.cpp:8
(Diff revision 1)
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +#include "AnimationHelper.h"
> +#include "mozilla/dom/Animation.h"      // for ComputedTimingFunction

Nit: Include ComputedTimingFunction.h instead of dom/Animation.h.

::: gfx/layers/AnimationHelper.cpp:193
(Diff revision 1)
> +}
> +
> +void
> +AnimationHelper::SetAnimations(AnimationArray& aAnimations,
> +                               InfallibleTArray<AnimData>& aAnimData,
> +                               StyleAnimationValue& aValue)

Please rename |aValue| to |aBaseAnimationStyle|.

::: gfx/layers/Layers.h:105
(Diff revision 1)
>  class SpecificLayerAttributes;
>  class Compositor;
>  class FrameUniformityData;
>  class PersistentBufferProvider;
>  class GlyphArray;
> +class WebRenderLayerManager;

I wonder where this class came from.

::: gfx/layers/composite/AsyncCompositionManager.cpp:24
(Diff revision 1)
>  #include "mozilla/dom/KeyframeEffectBinding.h" // for dom::IterationComposite
>  #include "mozilla/gfx/BaseRect.h"       // for BaseRect
>  #include "mozilla/gfx/Point.h"          // for RoundedToInt, PointTyped
>  #include "mozilla/gfx/Rect.h"           // for RoundedToInt, RectTyped
>  #include "mozilla/gfx/ScaleFactor.h"    // for ScaleFactor
> +#include "mozilla/layers/AnimationHelper.h"

Nit: this change should be in the next patch?
Attachment #8830631 - Flags: review?(hikezoe) → review+
Comment on attachment 8830632 [details]
Bug 1332211 - refactor animation code in AsyncCompositionManager,

https://reviewboard.mozilla.org/r/107376/#review108508

LGTM.  Thanks for doing this!
Attachment #8830632 - Flags: review?(hikezoe) → review+
Comment on attachment 8830631 [details]
Bug 1332211 - Move Layer::SetAnimation into AnimationHelper,

https://reviewboard.mozilla.org/r/107374/#review108502

> Nit: this change should be in the next patch?

I still keep the following function in AsyncCompositionManager.cpp. So this header is needed because of the declaration AnimationData 

static void                                                                                                                                                                                                                                                                
ApplyAnimatedValue(Layer* aLayer,                                                                                                                                                                                                                                          
                   nsCSSPropertyID aProperty,                                                                                                                                                                                                                              
                   const AnimationData& aAnimationData,                                                                                                                                                                                                                    
                   const StyleAnimationValue& aValue)
Comment on attachment 8830631 [details]
Bug 1332211 - Move Layer::SetAnimation into AnimationHelper,

https://reviewboard.mozilla.org/r/107374/#review110204

::: gfx/layers/Layers.h:105
(Diff revision 1)
>  class SpecificLayerAttributes;
>  class Compositor;
>  class FrameUniformityData;
>  class PersistentBufferProvider;
>  class GlyphArray;
> +class WebRenderLayerManager;

I should not include this.
(In reply to Peter Chang[:pchang] from comment #13)
> Submit another try to fix unified build problem.
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=68ffb9e3624f737e3f6a8a16bc1240f751d5b9e7
Build got passed.
Comment on attachment 8833286 [details]
Bug 1332211 - fix build break because of unified build,

https://reviewboard.mozilla.org/r/109532/#review111070
Attachment #8833286 - Flags: review?(mtseng) → review+
Pushed by pchang@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/4fe2c1b809e8
Move Layer::SetAnimation into AnimationHelper, r=hiro
https://hg.mozilla.org/integration/autoland/rev/61dc0a725d0f
refactor animation code in AsyncCompositionManager, r=hiro
https://hg.mozilla.org/integration/autoland/rev/8c4afac0dcba
fix build break because of unified build, r=mtseng
Blocks: 1333355
You need to log in before you can comment on or make changes to this bug.