exo: Only create new SurfaceLayer if layer properties change.

components/exo/surface.cc

Index: components/exo/surface.cc
diff --git a/components/exo/surface.cc b/components/exo/surface.cc
index 7cf11498f137d0e6ad5096444bee4d22809718c3..893373aa0e02113c68821f5a93fecff67fccf4dd 100644
--- a/components/exo/surface.cc
+++ b/components/exo/surface.cc
@@ -265,6 +265,12 @@ void Surface::Attach(Buffer* buffer) {
 
   has_pending_contents_ = true;
   pending_buffer_ = buffer ? buffer->AsWeakPtr() : base::WeakPtr<Buffer>();
+
+  if ((pending_buffer_ &&
+       (current_resource_.size != pending_buffer_->GetSize())) ||
+      (!pending_buffer_ && !current_resource_.size.IsEmpty())) {
+    modified_layer_info_ = true;

[reveman, 2016/06/09 14:19:29]

Can we evaluate this as part of CommitSurfaceHierarchy() instead? Same goes for
the other functions that set pending state below. This way when more complicated
clients make a large number of changes before they commit and some changes that
cancel out other changes, tht will not result in unnecessary work at the time of
commit().

+  }
 }
 
 void Surface::Damage(const gfx::Rect& damage) {
@@ -283,7 +289,10 @@ void Surface::SetOpaqueRegion(const SkRegion& region) {
   TRACE_EVENT1("exo", "Surface::SetOpaqueRegion", "region",
                gfx::SkIRectToRect(region.getBounds()).ToString());
 
+  if (pending_opaque_region_ == region)
+    return;
   pending_opaque_region_ = region;
+  modified_layer_info_ = true;
 }
 
 void Surface::SetInputRegion(const SkRegion& region) {
@@ -291,12 +300,17 @@ void Surface::SetInputRegion(const SkRegion& region) {
                gfx::SkIRectToRect(region.getBounds()).ToString());
 
   pending_input_region_ = region;
+  modified_layer_info_ = true;
 }
 
 void Surface::SetBufferScale(float scale) {
   TRACE_EVENT1("exo", "Surface::SetBufferScale", "scale", scale);
+  if (pending_buffer_scale_ == scale)
+    return;
 
   pending_buffer_scale_ = scale;
+
+  modified_layer_info_ = true;
 }
 
 void Surface::AddSubSurface(Surface* sub_surface) {
@@ -309,6 +323,7 @@ void Surface::AddSubSurface(Surface* sub_surface) {
 
   DCHECK(!ListContainsEntry(pending_sub_surfaces_, sub_surface));
   pending_sub_surfaces_.push_back(std::make_pair(sub_surface, gfx::Point()));
+  modified_layer_info_ = true;
 }
 
 void Surface::RemoveSubSurface(Surface* sub_surface) {
@@ -322,6 +337,7 @@ void Surface::RemoveSubSurface(Surface* sub_surface) {
   DCHECK(ListContainsEntry(pending_sub_surfaces_, sub_surface));
   pending_sub_surfaces_.erase(
       FindListEntry(pending_sub_surfaces_, sub_surface));
+  modified_layer_info_ = true;
 }
 
 void Surface::SetSubSurfacePosition(Surface* sub_surface,
@@ -331,7 +347,10 @@ void Surface::SetSubSurfacePosition(Surface* sub_surface,
 
   auto it = FindListEntry(pending_sub_surfaces_, sub_surface);
   DCHECK(it != pending_sub_surfaces_.end());
+  if (it->second == position)
+    return;
   it->second = position;
+  modified_layer_info_ = true;
 }
 
 void Surface::PlaceSubSurfaceAbove(Surface* sub_surface, Surface* reference) {
@@ -359,9 +378,11 @@ void Surface::PlaceSubSurfaceAbove(Surface* sub_surface, Surface* reference) {
   }
 
   DCHECK(ListContainsEntry(pending_sub_surfaces_, sub_surface));
-  pending_sub_surfaces_.splice(
-      position_it, pending_sub_surfaces_,
-      FindListEntry(pending_sub_surfaces_, sub_surface));
+  auto it = FindListEntry(pending_sub_surfaces_, sub_surface);
+  if (it == position_it)
+    return;
+  pending_sub_surfaces_.splice(position_it, pending_sub_surfaces_, it);
+  modified_layer_info_ = true;
 }
 
 void Surface::PlaceSubSurfaceBelow(Surface* sub_surface, Surface* sibling) {
@@ -382,21 +403,29 @@ void Surface::PlaceSubSurfaceBelow(Surface* sub_surface, Surface* sibling) {
   }
 
   DCHECK(ListContainsEntry(pending_sub_surfaces_, sub_surface));
-  pending_sub_surfaces_.splice(
-      sibling_it, pending_sub_surfaces_,
-      FindListEntry(pending_sub_surfaces_, sub_surface));
+  auto it = FindListEntry(pending_sub_surfaces_, sub_surface);
+  if (it == sibling_it)
+    return;
+  pending_sub_surfaces_.splice(sibling_it, pending_sub_surfaces_, it);
+  modified_layer_info_ = true;
 }
 
 void Surface::SetViewport(const gfx::Size& viewport) {
   TRACE_EVENT1("exo", "Surface::SetViewport", "viewport", viewport.ToString());
 
+  if (pending_viewport_ == viewport)
+    return;
   pending_viewport_ = viewport;
+  modified_layer_info_ = true;
 }
 
 void Surface::SetCrop(const gfx::RectF& crop) {
   TRACE_EVENT1("exo", "Surface::SetCrop", "crop", crop.ToString());
 
+  if (pending_crop_ == crop)
+    return;
   pending_crop_ = crop;
+  modified_layer_info_ = true;
 }
 
 void Surface::SetOnlyVisibleOnSecureOutput(bool only_visible_on_secure_output) {
@@ -409,13 +438,19 @@ void Surface::SetOnlyVisibleOnSecureOutput(bool only_visible_on_secure_output) {
 void Surface::SetBlendMode(SkXfermode::Mode blend_mode) {
   TRACE_EVENT1("exo", "Surface::SetBlendMode", "blend_mode", blend_mode);
 
+  if (pending_blend_mode_ == blend_mode)
+    return;
   pending_blend_mode_ = blend_mode;
+  modified_layer_info_ = true;
 }
 
 void Surface::SetAlpha(float alpha) {
   TRACE_EVENT1("exo", "Surface::SetAlpha", "alpha", alpha);
 
+  if (pending_alpha_ == alpha)
+    return;
   pending_alpha_ = alpha;
+  modified_layer_info_ = true;
 }
 
 void Surface::Commit() {
@@ -423,15 +458,18 @@ void Surface::Commit() {
 
   needs_commit_surface_hierarchy_ = true;
 
-  if (delegate_)
+  if (delegate_) {
     delegate_->OnSurfaceCommit();
-  else
+  } else {
+    CheckForCommittingToNewSurface();

[reveman, 2016/06/09 14:19:29]

Instead of adding this function, can the requirement to use a new surface be a
return value of CommitSurfaceHierarchy?

     CommitSurfaceHierarchy();
+  }
 }
 
 void Surface::CommitSurfaceHierarchy() {
   DCHECK(needs_commit_surface_hierarchy_);
   needs_commit_surface_hierarchy_ = false;
+  modified_layer_info_ = false;
 
   // TODO(dcastagna): Make secure_output_only a layer property instead of a
   // texture mailbox flag so this can be changed without have to provide
@@ -639,6 +677,29 @@ void Surface::WillDraw(cc::SurfaceId id) {
   }
 }
 
+void Surface::CheckForCommittingToNewSurface() {
+  if (HaveChildLayersChanged())
+    SetChildrenCommitToNewSurface();
+}
+
+bool Surface::HaveChildLayersChanged() {
+  if (needs_commit_surface_hierarchy_ && modified_layer_info_)
+    return true;
+
+  for (auto& surf : pending_sub_surfaces_) {
+    if (surf.first->HaveChildLayersChanged())
+      return true;
+  }
+  return false;
+}
+
+void Surface::SetChildrenCommitToNewSurface() {
+  commit_to_new_surface_ = true;
+  for (auto& surf : pending_sub_surfaces_) {
+    surf.first->SetChildrenCommitToNewSurface();
+  }
+}
+
 void Surface::CommitTextureContents() {
   // We update contents if Attach() has been called since last commit.
   if (has_pending_contents_) {
@@ -749,9 +810,15 @@ void Surface::CommitSurfaceContents() {
     }
   }
 
-  cc::SurfaceId old_surface_id = surface_id_;
-  surface_id_ = factory_owner_->id_allocator_->GenerateId();
-  factory_owner_->surface_factory_->Create(surface_id_);
+  cc::SurfaceId old_surface_id;
+  bool new_surface = false;

[reveman, 2016/06/09 14:19:29]

is this variable needed? would "surface_id_ != old_surface_id" not be the same?

+  if (commit_to_new_surface_ || surface_id_.is_null()) {
+    commit_to_new_surface_ = false;
+    new_surface = true;
+    old_surface_id = surface_id_;
+    surface_id_ = factory_owner_->id_allocator_->GenerateId();
+    factory_owner_->surface_factory_->Create(surface_id_);
+  }
 
   gfx::Size buffer_size = current_resource_.size;
   gfx::SizeF scaled_buffer_size(
@@ -844,13 +911,15 @@ void Surface::CommitSurfaceContents() {
     factory_owner_->surface_factory_->Destroy(old_surface_id);
   }
 
-  layer()->SetShowSurface(
-      surface_id_,
-      base::Bind(&SatisfyCallback, base::Unretained(surface_manager_)),
-      base::Bind(&RequireCallback, base::Unretained(surface_manager_)),
-      contents_surface_size, contents_surface_to_layer_scale, layer_size);
-  layer()->SetBounds(gfx::Rect(layer()->bounds().origin(), layer_size));
-  layer()->SetFillsBoundsOpaquely(alpha_ == 1.0f && frame_is_opaque);
+  if (new_surface) {
+    layer()->SetShowSurface(
+        surface_id_,
+        base::Bind(&SatisfyCallback, base::Unretained(surface_manager_)),
+        base::Bind(&RequireCallback, base::Unretained(surface_manager_)),
+        contents_surface_size, contents_surface_to_layer_scale, layer_size);
+    layer()->SetBounds(gfx::Rect(layer()->bounds().origin(), layer_size));
+    layer()->SetFillsBoundsOpaquely(alpha_ == 1.0f && frame_is_opaque);
+  }
 
   // Reset damage.
   pending_damage_.setEmpty();