Skip to content

Commit

Permalink
Revert "Add DeferredPaintObserver to ScopedLayerAnimationSettings."
Browse files Browse the repository at this point in the history 
This reverts commit 7ba7cc8.

Reason for revert: This broke LayerAnimatorTest on Linux Chromium OS ASan LSan Tests.


Original change's description:
> Add DeferredPaintObserver to ScopedLayerAnimationSettings.
> 
> We want to defer layer painting during some UI animations so that we can
> avoid resterization at the same time and we can cache the render suface
> of the animating layer. Adding an observer to
> ScopedLayerAnimationSettings can AddDeferredPaintRequest and
> RemoveDeferredPaintRequest at the beginning and end of the animation.
> 
> Bug: 761109
> Test: LayerAnimatorTest.DeferredPaint*
> Change-Id: If5e7afb7d2564f5ff480438447eabd10fca799ab
> Reviewed-on: https://chromium-review.googlesource.com/685493
> Commit-Queue: Tao Wu <wutao@chromium.org>
> Reviewed-by: Antoine Labour <piman@chromium.org>
> Reviewed-by: David Reveman <reveman@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#506253}

TBR=reveman@chromium.org,ajuma@chromium.org,varkha@chromium.org,piman@chromium.org,wutao@chromium.org

Change-Id: I975cf1d908101bf9f97ca2fe68a74805fc1d17a9
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 761109
Reviewed-on: https://chromium-review.googlesource.com/700214
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Commit-Queue: Kentaro Hara <haraken@chromium.org>
Cr-Commit-Position: refs/heads/master@{#506305}
  • Loading branch information
@xharaken
xharaken authored and Commit Bot committed Oct 4, 2017
1 parent c865767 commit 2a510e5
Show file tree
Hide file tree
Showing 6 changed files with 7 additions and 308 deletions.
2 changes: 0 additions & 2 deletions ui/compositor/layer.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -435,8 +435,6 @@ class COMPOSITOR_EXPORT Layer : public LayerAnimationDelegate,
void AddDeferredPaintRequest();
void RemoveDeferredPaintRequest();

bool IsPaintDeferredForTesting() const { return deferred_paint_requests_; }

// Request trilinear filtering for layer.
void SetTrilinearFiltering(bool trilinear_filtering);

Expand Down
17 changes: 0 additions & 17 deletions ui/compositor/layer_animator.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -370,23 +370,6 @@ void LayerAnimator::RemoveObserver(LayerAnimationObserver* observer) {
}
}

void LayerAnimator::AddOwnedObserver(
std::unique_ptr<ImplicitAnimationObserver> animation_observer) {
owned_observer_list_.push_back(std::move(animation_observer));
}

void LayerAnimator::RemoveAndDestroyOwnedObserver(
ImplicitAnimationObserver* animation_observer) {
owned_observer_list_.erase(
std::remove_if(
owned_observer_list_.begin(), owned_observer_list_.end(),
[animation_observer](
const std::unique_ptr<ImplicitAnimationObserver>& other) {
return other.get() == animation_observer;
}),
owned_observer_list_.end());
}

void LayerAnimator::OnThreadedAnimationStarted(
base::TimeTicks monotonic_time,
cc::TargetProperty::Type target_property,
Expand Down
8 changes: 0 additions & 8 deletions ui/compositor/layer_animator.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,6 @@ class Transform;

namespace ui {
class Compositor;
class ImplicitAnimationObserver;
class Layer;
class LayerAnimationSequence;
class LayerAnimationDelegate;
Expand Down Expand Up @@ -197,11 +196,6 @@ class COMPOSITOR_EXPORT LayerAnimator : public base::RefCounted<LayerAnimator>,
void AddObserver(LayerAnimationObserver* observer);
void RemoveObserver(LayerAnimationObserver* observer);

void AddOwnedObserver(
std::unique_ptr<ImplicitAnimationObserver> animation_observer);
void RemoveAndDestroyOwnedObserver(
ImplicitAnimationObserver* animation_observer);

// Called when a threaded animation is actually started.
void OnThreadedAnimationStarted(base::TimeTicks monotonic_time,
cc::TargetProperty::Type target_property,
Expand Down Expand Up @@ -422,8 +416,6 @@ class COMPOSITOR_EXPORT LayerAnimator : public base::RefCounted<LayerAnimator>,
// aborted.
base::ObserverList<LayerAnimationObserver> observers_;

std::vector<std::unique_ptr<ImplicitAnimationObserver>> owned_observer_list_;

DISALLOW_COPY_AND_ASSIGN(LayerAnimator);
};

Expand Down
222 changes: 3 additions & 219 deletions ui/compositor/layer_animator_unittest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1727,7 +1727,7 @@ TEST(LayerAnimatorTest, CacheRenderSurface) {
// crash when the layer was destroyed.
TEST(LayerAnimatorTest, CacheRenderSurfaceOnWillBeDestroyedLayer) {
// Case 1: layer is a pointer.
auto layer1 = base::MakeUnique<Layer>();
ui::Layer* layer1 = new Layer();
scoped_refptr<LayerAnimator> animator(layer1->GetAnimator());
animator->set_disable_timer_for_test(true);
TestImplicitAnimationObserver observer1(false);
Expand All @@ -1743,7 +1743,8 @@ TEST(LayerAnimatorTest, CacheRenderSurfaceOnWillBeDestroyedLayer) {
}

EXPECT_FALSE(observer1.animations_completed());
layer1.release();
delete layer1;
layer1 = nullptr;
animator->StopAnimatingProperty(LayerAnimationElement::OPACITY);
EXPECT_TRUE(observer1.animations_completed());

Expand Down Expand Up @@ -1865,223 +1866,6 @@ TEST(LayerAnimatorTest, CacheRenderSurfaceInTwoAnimations) {
}
}

// Tests that deferred painting request added to a scoped settings object is
// still reset when the object goes out of scope.
TEST(LayerAnimatorTest, DeferredPaint) {
ui::Layer layer;
scoped_refptr<LayerAnimator> animator(layer.GetAnimator());
animator->set_disable_timer_for_test(true);
TestImplicitAnimationObserver observer(false);

EXPECT_FALSE(observer.animations_completed());
EXPECT_FALSE(layer.IsPaintDeferredForTesting());
animator->SetOpacity(1.0f);

{
ScopedLayerAnimationSettings settings(animator.get());
settings.DeferPaint();
settings.AddObserver(&observer);
animator->SetOpacity(0.0f);
}

EXPECT_FALSE(observer.animations_completed());
EXPECT_TRUE(layer.IsPaintDeferredForTesting());
animator->StopAnimatingProperty(LayerAnimationElement::OPACITY);
EXPECT_TRUE(observer.animations_completed());
EXPECT_TRUE(observer.WasAnimationCompletedForProperty(
LayerAnimationElement::OPACITY));
EXPECT_FLOAT_EQ(0.0f, layer.opacity());
EXPECT_FALSE(layer.IsPaintDeferredForTesting());
}

// Tests that deferred painting request is added to child layers and will be
// removed even the child layer was reparented.
TEST(LayerAnimatorTest, DeferredPaintOnChildLayer) {
ui::Layer layer;
ui::Layer child_layer1;
ui::Layer child_layer2;
layer.Add(&child_layer1);
layer.Add(&child_layer2);

scoped_refptr<LayerAnimator> animator(layer.GetAnimator());
animator->set_disable_timer_for_test(true);
TestImplicitAnimationObserver observer(false);

EXPECT_FALSE(observer.animations_completed());
EXPECT_FALSE(layer.IsPaintDeferredForTesting());
EXPECT_FALSE(child_layer1.IsPaintDeferredForTesting());
EXPECT_FALSE(child_layer2.IsPaintDeferredForTesting());
animator->SetOpacity(1.0f);

{
ScopedLayerAnimationSettings settings(animator.get());
settings.DeferPaint();
settings.AddObserver(&observer);
animator->SetOpacity(0.0f);
}

EXPECT_FALSE(observer.animations_completed());
EXPECT_TRUE(layer.IsPaintDeferredForTesting());
EXPECT_TRUE(child_layer1.IsPaintDeferredForTesting());
EXPECT_TRUE(child_layer2.IsPaintDeferredForTesting());

// Reparent child_layer2.
ui::Layer new_parent_layer;
new_parent_layer.Add(&child_layer2);
EXPECT_TRUE(child_layer2.IsPaintDeferredForTesting());

animator->StopAnimatingProperty(LayerAnimationElement::OPACITY);
EXPECT_TRUE(observer.animations_completed());
EXPECT_TRUE(observer.WasAnimationCompletedForProperty(
LayerAnimationElement::OPACITY));
EXPECT_FLOAT_EQ(0.0f, layer.opacity());
EXPECT_FALSE(layer.IsPaintDeferredForTesting());
EXPECT_FALSE(child_layer1.IsPaintDeferredForTesting());
EXPECT_FALSE(child_layer2.IsPaintDeferredForTesting());
}

// Tests that deffered paint request added to two scoped settings objects is
// still reset when animation finishes.
TEST(LayerAnimatorTest, DeferredPaintInTwoAnimations) {
ui::Layer layer;
scoped_refptr<LayerAnimator> animator(layer.GetAnimator());
animator->set_disable_timer_for_test(true);

// Case 1: the original deferred paint status if false.
EXPECT_FALSE(layer.IsPaintDeferredForTesting());
animator->SetBrightness(1.0f);
animator->SetOpacity(1.0f);

// Start the brightness animation.
{
ScopedLayerAnimationSettings settings(animator.get());
settings.DeferPaint();
animator->SetBrightness(0.0f);
}
EXPECT_TRUE(layer.IsPaintDeferredForTesting());

// Start the opacity animation.
{
ScopedLayerAnimationSettings settings(animator.get());
settings.DeferPaint();
animator->SetOpacity(0.0f);
}
EXPECT_TRUE(layer.IsPaintDeferredForTesting());

// Finish the brightness animation.
{
animator->StopAnimatingProperty(LayerAnimationElement::BRIGHTNESS);
EXPECT_FLOAT_EQ(0.0f, layer.layer_brightness());
EXPECT_TRUE(layer.IsPaintDeferredForTesting());
}

// Finish the opacity animation.
{
animator->StopAnimatingProperty(LayerAnimationElement::OPACITY);
EXPECT_FLOAT_EQ(0.0f, layer.opacity());
EXPECT_FALSE(layer.IsPaintDeferredForTesting());
}

// Case 2: the original cache status if true.
layer.AddDeferredPaintRequest();
EXPECT_TRUE(layer.IsPaintDeferredForTesting());
animator->SetBrightness(1.0f);
animator->SetOpacity(1.0f);

// Start the brightness animation.
{
ScopedLayerAnimationSettings settings(animator.get());
settings.DeferPaint();
animator->SetBrightness(0.0f);
}
EXPECT_TRUE(layer.IsPaintDeferredForTesting());

// Start the opacity animation.
{
ScopedLayerAnimationSettings settings(animator.get());
settings.DeferPaint();
animator->SetOpacity(0.0f);
}
EXPECT_TRUE(layer.IsPaintDeferredForTesting());

// Finish the brightness animation.
{
animator->StopAnimatingProperty(LayerAnimationElement::BRIGHTNESS);
EXPECT_FLOAT_EQ(0.0f, layer.layer_brightness());
EXPECT_TRUE(layer.IsPaintDeferredForTesting());
}

// Finish the opacity animation.
{
animator->StopAnimatingProperty(LayerAnimationElement::OPACITY);
EXPECT_FLOAT_EQ(0.0f, layer.opacity());
EXPECT_TRUE(layer.IsPaintDeferredForTesting());
}
}

// Tests that deferred paint request added to a scoped settings object will
// not crash when the layer was destroyed.
TEST(LayerAnimatorTest, DeferredPaintOnWillBeDestroyedLayer) {
// Case 1: layer is a pointer.
auto layer1 = base::MakeUnique<Layer>();
scoped_refptr<LayerAnimator> animator(layer1->GetAnimator());
animator->set_disable_timer_for_test(true);
TestImplicitAnimationObserver observer1(false);

EXPECT_FALSE(observer1.animations_completed());
animator->SetOpacity(1.0f);

{
ScopedLayerAnimationSettings settings(animator.get());
settings.DeferPaint();
settings.AddObserver(&observer1);
animator->SetOpacity(0.0f);
}

EXPECT_FALSE(observer1.animations_completed());
layer1.release();
animator->StopAnimatingProperty(LayerAnimationElement::OPACITY);
EXPECT_TRUE(observer1.animations_completed());

// Case 2: layer is a local variable.
TestImplicitAnimationObserver observer2(false);
{
ui::Layer layer2;
animator = layer2.GetAnimator();

EXPECT_FALSE(observer2.animations_completed());
animator->SetBrightness(1.0f);

ScopedLayerAnimationSettings settings(animator.get());
settings.DeferPaint();
settings.AddObserver(&observer2);
animator->SetBrightness(0.0f);
}

EXPECT_FALSE(observer2.animations_completed());
animator->StopAnimatingProperty(LayerAnimationElement::BRIGHTNESS);
EXPECT_TRUE(observer2.animations_completed());

// Case 3: Animation finishes before layer is destroyed.
ui::Layer layer3;
animator = layer3.GetAnimator();
TestImplicitAnimationObserver observer3(false);

EXPECT_FALSE(observer3.animations_completed());
animator->SetGrayscale(1.0f);

{
ScopedLayerAnimationSettings settings(animator.get());
settings.DeferPaint();
settings.AddObserver(&observer3);
animator->SetGrayscale(0.0f);
}

EXPECT_FALSE(observer3.animations_completed());
animator->StopAnimatingProperty(LayerAnimationElement::GRAYSCALE);
EXPECT_TRUE(observer3.animations_completed());
}

// Tests that an observer added to a scoped settings object is still notified
// when the object goes out of scope due to the animation being interrupted.
TEST(LayerAnimatorTest, InterruptedImplicitAnimationObservers) {
Expand Down
63 changes: 4 additions & 59 deletions ui/compositor/scoped_layer_animation_settings.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -33,10 +33,9 @@ class CacheRenderSurfaceObserver : public ui::ImplicitAnimationObserver,
// If animation finishes before |layer_| is destoyed, we will reset the
// cache and remove |this| from the |layer_| observer list when deleting
// |this|.
if (layer_) {
if (layer_)
layer_->RemoveCacheRenderSurfaceRequest();
layer_->GetAnimator()->RemoveAndDestroyOwnedObserver(this);
}
delete this;
}

// ui::LayerObserver overrides:
Expand All @@ -54,54 +53,6 @@ class CacheRenderSurfaceObserver : public ui::ImplicitAnimationObserver,
DISALLOW_COPY_AND_ASSIGN(CacheRenderSurfaceObserver);
};

class DeferredPaintObserver : public ui::ImplicitAnimationObserver,
public ui::LayerObserver {
public:
DeferredPaintObserver(ui::Layer* layer) : layer_(layer) {
layer_->AddObserver(this);
layer_->AddDeferredPaintRequest();
}
~DeferredPaintObserver() override {
if (layer_)
layer_->RemoveObserver(this);
}

// ui::ImplicitAnimationObserver overrides:
void OnImplicitAnimationsCompleted() override {
// If animation finishes before |layer_| is destoyed, we will remove the
// request of deferred painting and remove |this| from the |layer_|
// observer list when deleting |this|.
if (layer_) {
layer_->RemoveDeferredPaintRequest();
layer_->GetAnimator()->RemoveAndDestroyOwnedObserver(this);
}
}

// ui::LayerObserver overrides:
void LayerDestroyed(ui::Layer* layer) override {
// If the animation is still going past layer destruction then we want the
// layer too keep being deferred paint until the animation has finished. We
// will defer deleting |this| until the animation finishes.
layer_->RemoveObserver(this);
layer_ = nullptr;
}

private:
ui::Layer* layer_;

DISALLOW_COPY_AND_ASSIGN(DeferredPaintObserver);
};

void AddDeferredPaintObserverRecursive(
ui::Layer* layer,
ui::ScopedLayerAnimationSettings* settings) {
auto observer = base::MakeUnique<DeferredPaintObserver>(layer);
settings->AddObserver(observer.get());
layer->GetAnimator()->AddOwnedObserver(std::move(observer));
for (auto* child : layer->children())
AddDeferredPaintObserverRecursive(child, settings);
}

} // namespace

namespace ui {
Expand Down Expand Up @@ -151,14 +102,8 @@ void ScopedLayerAnimationSettings::SetTransitionDuration(
}

void ScopedLayerAnimationSettings::CacheRenderSurface() {
auto observer = base::MakeUnique<CacheRenderSurfaceObserver>(
animator_->delegate()->GetLayer());
AddObserver(observer.get());
animator_->AddOwnedObserver(std::move(observer));
}

void ScopedLayerAnimationSettings::DeferPaint() {
AddDeferredPaintObserverRecursive(animator_->delegate()->GetLayer(), this);
AddObserver(
new CacheRenderSurfaceObserver(animator_->delegate()->GetLayer()));
}

void ScopedLayerAnimationSettings::LockTransitionDuration() {
Expand Down
3 changes: 0 additions & 3 deletions ui/compositor/scoped_layer_animation_settings.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -34,9 +34,6 @@ class COMPOSITOR_EXPORT ScopedLayerAnimationSettings {
// This will request render surface caching on the animating layer. The cache
// request will be removed at the end of the animation.
void CacheRenderSurface();
// This will defer painting on the animating layer. The deferred paint
// request will be removed at the end of the animation.
void DeferPaint();
base::TimeDelta GetTransitionDuration() const;

// Locks transition duration in |animator_|. When transition duration
Expand Down

0 comments on commit 2a510e5

Please sign in to comment.