Skip to content

Commit

Permalink
Revert of Make ui::Compositor use ui::Scheduler (patchset #2 id:20001…
Browse files Browse the repository at this point in the history 
… of https://codereview.chromium.org/638653003/)

This reverts commit 36b7fc7.

Reason for revert:
Speculatively reverting as I suspect it may be the cause of test failures like this:

[16079:16079:1009/052004:FATAL:compositor.cc(303)] Check failed: !IsLocked().

http://build.chromium.org/p/chromium.linux/builders/Linux%20Tests%20%28dbg%29%281%29%2832%29/builds/6451

Original issue's description:
> Make ui::Compositor use ui::Scheduler
>
> Taken from enne's CL 535733002 and rebased. It has been taken out of CL 134623005.
>
> BUG=329552
>
> Committed: https://crrev.com/36b7fc7f8b05ea627873e58a162c1c26784e472d
> Cr-Commit-Position: refs/heads/master@{#298779}

TBR=ben@chromium.org,danakj@chromium.org,sky@chromium.org,weiliangc@chromium.org
NOTREECHECKS=true
NOTRY=true
BUG=329552

Review URL: https://codereview.chromium.org/640293003

Cr-Commit-Position: refs/heads/master@{#298919}
  • Loading branch information
@weiliangc
weiliangc authored and Commit bot committed Oct 9, 2014
1 parent bca02c0 commit 1f27b28
Show file tree
Hide file tree
Showing 12 changed files with 186 additions and 116 deletions.
5 changes: 2 additions & 3 deletions chrome/browser/ui/views/menu_view_drag_and_drop_test.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,9 +17,8 @@ namespace {

// Borrowed from chrome/browser/ui/views/bookmarks/bookmark_bar_view_test.cc,
// since these are also disabled on Linux for drag and drop.
// TODO(erg): Fix DND tests on linux_aura. http://crbug.com/163931
// TODO(enne): Windows version has flaky timeouts. http://crbug.com/401226
#if defined(USE_AURA)
// TODO(erg): Fix DND tests on linux_aura. crbug.com/163931
#if defined(OS_LINUX) && defined(USE_AURA)
#define MAYBE(x) DISABLED_##x
#else
#define MAYBE(x) x
Expand Down
127 changes: 103 additions & 24 deletions ui/compositor/compositor.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,12 @@ void CompositorLock::CancelLock() {
compositor_ = NULL;
}

} // namespace ui

namespace {} // namespace

namespace ui {

Compositor::Compositor(gfx::AcceleratedWidget widget,
ui::ContextFactory* context_factory,
scoped_refptr<base::SingleThreadTaskRunner> task_runner)
Expand All @@ -72,9 +78,16 @@ Compositor::Compositor(gfx::AcceleratedWidget widget,
task_runner_(task_runner),
vsync_manager_(new CompositorVSyncManager()),
device_scale_factor_(0.0f),
last_started_frame_(0),
last_ended_frame_(0),
disable_schedule_composite_(false),
compositor_lock_(NULL),
layer_animator_collection_(this) {
defer_draw_scheduling_(false),
waiting_on_compositing_end_(false),
draw_on_compositing_end_(false),
swap_state_(SWAP_NONE),
layer_animator_collection_(this),
schedule_draw_factory_(this) {
root_web_layer_ = cc::Layer::Create();

CommandLine* command_line = CommandLine::ForCurrentProcess();
Expand Down Expand Up @@ -122,6 +135,7 @@ Compositor::Compositor(gfx::AcceleratedWidget widget,

settings.impl_side_painting = IsUIImplSidePaintingEnabled();
settings.use_zero_copy = IsUIZeroCopyEnabled();
settings.single_thread_proxy_scheduler = false;

base::TimeTicks before_create = base::TimeTicks::Now();
if (compositor_thread_loop_.get()) {
Expand Down Expand Up @@ -162,7 +176,14 @@ Compositor::~Compositor() {
}

void Compositor::ScheduleDraw() {
host_->SetNeedsCommit();
if (compositor_thread_loop_.get()) {
host_->SetNeedsCommit();
} else if (!defer_draw_scheduling_) {
defer_draw_scheduling_ = true;
task_runner_->PostTask(
FROM_HERE,
base::Bind(&Compositor::Draw, schedule_draw_factory_.GetWeakPtr()));
}
}

void Compositor::SetRootLayer(Layer* root_layer) {
Expand All @@ -183,19 +204,44 @@ void Compositor::SetHostHasTransparentBackground(
host_->set_has_transparent_background(host_has_transparent_background);
}

void Compositor::Draw() {
DCHECK(!compositor_thread_loop_.get());

defer_draw_scheduling_ = false;
if (waiting_on_compositing_end_) {
draw_on_compositing_end_ = true;
return;
}
if (!root_layer_)
return;

TRACE_EVENT_ASYNC_BEGIN0("ui", "Compositor::Draw", last_started_frame_ + 1);

DCHECK_NE(swap_state_, SWAP_POSTED);
swap_state_ = SWAP_NONE;

waiting_on_compositing_end_ = true;
last_started_frame_++;
if (!IsLocked()) {
// TODO(nduca): Temporary while compositor calls
// compositeImmediately() directly.
cc::BeginFrameArgs args =
cc::BeginFrameArgs::Create(gfx::FrameTime::Now(),
base::TimeTicks(),
cc::BeginFrameArgs::DefaultInterval());
BeginMainFrame(args);
host_->Composite(args.frame_time);
}
if (swap_state_ == SWAP_NONE)
NotifyEnd();
}

void Compositor::ScheduleFullRedraw() {
// TODO(enne): Some callers (mac) call this function expecting that it
// will also commit. This should probably just redraw the screen
// from damage and not commit. ScheduleDraw/ScheduleRedraw need
// better names.
host_->SetNeedsRedraw();
host_->SetNeedsCommit();
}

void Compositor::ScheduleRedrawRect(const gfx::Rect& damage_rect) {
// TODO(enne): Make this not commit. See ScheduleFullRedraw.
host_->SetNeedsRedrawRect(damage_rect);
host_->SetNeedsCommit();
}

void Compositor::FinishAllRendering() {
Expand Down Expand Up @@ -307,30 +353,45 @@ void Compositor::DidCommit() {
}

void Compositor::DidCommitAndDrawFrame() {
base::TimeTicks start_time = gfx::FrameTime::Now();
FOR_EACH_OBSERVER(CompositorObserver,
observer_list_,
OnCompositingStarted(this, start_time));
}

void Compositor::DidCompleteSwapBuffers() {
// DidPostSwapBuffers is a SingleThreadProxy-only feature. Synthetically
// generate OnCompositingStarted messages for the threaded case so that
// OnCompositingStarted/OnCompositingEnded messages match.
if (compositor_thread_loop_.get()) {
base::TimeTicks start_time = gfx::FrameTime::Now();
FOR_EACH_OBSERVER(CompositorObserver,
observer_list_,
OnCompositingStarted(this, start_time));
NotifyEnd();
} else {
DCHECK_EQ(swap_state_, SWAP_POSTED);
NotifyEnd();
swap_state_ = SWAP_COMPLETED;
}
FOR_EACH_OBSERVER(
CompositorObserver, observer_list_, OnCompositingEnded(this));
}

void Compositor::ScheduleComposite() {
if (!disable_schedule_composite_)
ScheduleDraw();
}

void Compositor::ScheduleAnimation() {
ScheduleComposite();
}

void Compositor::DidPostSwapBuffers() {
base::TimeTicks start_time = gfx::FrameTime::Now();
FOR_EACH_OBSERVER(CompositorObserver,
observer_list_,
OnCompositingStarted(this, start_time));
DCHECK(!compositor_thread_loop_.get());
DCHECK_EQ(swap_state_, SWAP_NONE);
swap_state_ = SWAP_POSTED;
}

void Compositor::DidAbortSwapBuffers() {
if (!compositor_thread_loop_.get()) {
if (swap_state_ == SWAP_POSTED) {
NotifyEnd();
swap_state_ = SWAP_COMPLETED;
}
}

FOR_EACH_OBSERVER(CompositorObserver,
observer_list_,
OnCompositingAborted(this));
Expand All @@ -348,7 +409,8 @@ void Compositor::SetLayerTreeDebugState(
scoped_refptr<CompositorLock> Compositor::GetCompositorLock() {
if (!compositor_lock_) {
compositor_lock_ = new CompositorLock(this);
host_->SetDeferCommits(true);
if (compositor_thread_loop_.get())
host_->SetDeferCommits(true);
FOR_EACH_OBSERVER(CompositorObserver,
observer_list_,
OnCompositingLockStateChanged(this));
Expand All @@ -359,7 +421,8 @@ scoped_refptr<CompositorLock> Compositor::GetCompositorLock() {
void Compositor::UnlockCompositor() {
DCHECK(compositor_lock_);
compositor_lock_ = NULL;
host_->SetDeferCommits(false);
if (compositor_thread_loop_.get())
host_->SetDeferCommits(false);
FOR_EACH_OBSERVER(CompositorObserver,
observer_list_,
OnCompositingLockStateChanged(this));
Expand All @@ -370,4 +433,20 @@ void Compositor::CancelCompositorLock() {
compositor_lock_->CancelLock();
}

void Compositor::NotifyEnd() {
last_ended_frame_++;
TRACE_EVENT_ASYNC_END0("ui", "Compositor::Draw", last_ended_frame_);
waiting_on_compositing_end_ = false;
if (draw_on_compositing_end_) {
draw_on_compositing_end_ = false;

// Call ScheduleDraw() instead of Draw() in order to allow other
// CompositorObservers to be notified before starting another
// draw cycle.
ScheduleDraw();
}
FOR_EACH_OBSERVER(
CompositorObserver, observer_list_, OnCompositingEnded(this));
}

} // namespace ui
22 changes: 22 additions & 0 deletions ui/compositor/compositor.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,8 @@
#include "ui/gfx/size.h"
#include "ui/gfx/vector2d.h"

class SkBitmap;

namespace base {
class MessageLoopProxy;
class RunLoop;
Expand Down Expand Up @@ -157,6 +159,9 @@ class COMPOSITOR_EXPORT Compositor
// compositing layers on.
float device_scale_factor() const { return device_scale_factor_; }

// Draws the scene created by the layer tree and any visual effects.
void Draw();

// Where possible, draws are scissored to a damage region calculated from
// changes to layer properties. This bypasses that and indicates that
// the whole frame needs to be drawn.
Expand Down Expand Up @@ -247,6 +252,9 @@ class COMPOSITOR_EXPORT Compositor
virtual void DidPostSwapBuffers() override;
virtual void DidAbortSwapBuffers() override;

int last_started_frame() { return last_started_frame_; }
int last_ended_frame() { return last_ended_frame_; }

bool IsLocked() { return compositor_lock_ != NULL; }

const cc::LayerTreeDebugState& GetLayerTreeDebugState() const;
Expand All @@ -266,6 +274,9 @@ class COMPOSITOR_EXPORT Compositor
// Called to release any pending CompositorLock
void CancelCompositorLock();

// Notifies the compositor that compositing is complete.
void NotifyEnd();

gfx::Size size_;

ui::ContextFactory* context_factory_;
Expand Down Expand Up @@ -296,8 +307,19 @@ class COMPOSITOR_EXPORT Compositor

CompositorLock* compositor_lock_;

// Prevent more than one draw from being scheduled.
bool defer_draw_scheduling_;

// Used to prevent Draw()s while a composite is in progress.
bool waiting_on_compositing_end_;
bool draw_on_compositing_end_;
enum SwapState { SWAP_NONE, SWAP_POSTED, SWAP_COMPLETED };
SwapState swap_state_;

LayerAnimatorCollection layer_animator_collection_;

base::WeakPtrFactory<Compositor> schedule_draw_factory_;

DISALLOW_COPY_AND_ASSIGN(Compositor);
};

Expand Down
30 changes: 11 additions & 19 deletions ui/compositor/layer_unittest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -129,7 +129,7 @@ class LayerWithRealCompositorTest : public testing::Test {
void DrawTree(Layer* root) {
GetCompositor()->SetRootLayer(root);
GetCompositor()->ScheduleDraw();
WaitForSwap();
WaitForDraw();
}

void ReadPixels(SkBitmap* bitmap) {
Expand All @@ -149,7 +149,7 @@ class LayerWithRealCompositorTest : public testing::Test {
// be in the middle of a draw right now, and the commit with the
// copy output request may not be done on the first draw.
for (int i = 0; i < 2; i++) {
GetCompositor()->ScheduleFullRedraw();
GetCompositor()->ScheduleDraw();
WaitForDraw();
}

Expand All @@ -159,13 +159,7 @@ class LayerWithRealCompositorTest : public testing::Test {
*bitmap = holder->result();
}

void WaitForDraw() {
ui::DrawWaiterForTest::WaitForCompositingStarted(GetCompositor());
}

void WaitForSwap() {
DrawWaiterForTest::WaitForCompositingEnded(GetCompositor());
}
void WaitForDraw() { ui::DrawWaiterForTest::Wait(GetCompositor()); }

void WaitForCommit() {
ui::DrawWaiterForTest::WaitForCommit(GetCompositor());
Expand Down Expand Up @@ -457,9 +451,7 @@ class LayerWithDelegateTest : public testing::Test {
WaitForDraw();
}

void WaitForDraw() {
DrawWaiterForTest::WaitForCompositingStarted(compositor());
}
void WaitForDraw() { DrawWaiterForTest::Wait(compositor()); }

void WaitForCommit() {
DrawWaiterForTest::WaitForCommit(compositor());
Expand Down Expand Up @@ -1036,25 +1028,25 @@ TEST_F(LayerWithRealCompositorTest, CompositorObservers) {
// Moving, but not resizing, a layer should alert the observers.
observer.Reset();
l2->SetBounds(gfx::Rect(0, 0, 350, 350));
WaitForSwap();
WaitForDraw();
EXPECT_TRUE(observer.notified());

// So should resizing a layer.
observer.Reset();
l2->SetBounds(gfx::Rect(0, 0, 400, 400));
WaitForSwap();
WaitForDraw();
EXPECT_TRUE(observer.notified());

// Opacity changes should alert the observers.
observer.Reset();
l2->SetOpacity(0.5f);
WaitForSwap();
WaitForDraw();
EXPECT_TRUE(observer.notified());

// So should setting the opacity back.
observer.Reset();
l2->SetOpacity(1.0f);
WaitForSwap();
WaitForDraw();
EXPECT_TRUE(observer.notified());

// Setting the transform of a layer should alert the observers.
Expand All @@ -1064,15 +1056,15 @@ TEST_F(LayerWithRealCompositorTest, CompositorObservers) {
transform.Rotate(90.0);
transform.Translate(-200.0, -200.0);
l2->SetTransform(transform);
WaitForSwap();
WaitForDraw();
EXPECT_TRUE(observer.notified());

// A change resulting in an aborted swap buffer should alert the observer
// and also signal an abort.
observer.Reset();
l2->SetOpacity(0.1f);
GetCompositor()->DidAbortSwapBuffers();
WaitForSwap();
WaitForDraw();
EXPECT_TRUE(observer.notified());
EXPECT_TRUE(observer.aborted());

Expand All @@ -1081,7 +1073,7 @@ TEST_F(LayerWithRealCompositorTest, CompositorObservers) {
// Opacity changes should no longer alert the removed observer.
observer.Reset();
l2->SetOpacity(0.5f);
WaitForSwap();
WaitForDraw();

EXPECT_FALSE(observer.notified());
}
Expand Down
Loading

0 comments on commit 1f27b28

Please sign in to comment.