exo: Only create new SurfaceLayer if layer properties change.

components/exo/surface.cc

 // Copyright 2015 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "components/exo/surface.h"
 
 #include <utility>
 
 #include "base/callback_helpers.h"
 #include "base/logging.h"
@@ skipping 247 matching lines @@
 void Surface::SetUseSurfaceLayer(bool use_surface_layer) {
   use_surface_layer_ = use_surface_layer;
 }
 
 void Surface::Attach(Buffer* buffer) {
   TRACE_EVENT1("exo", "Surface::Attach", "buffer",
                buffer ? buffer->GetSize().ToString() : "null");
 
   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) {
   TRACE_EVENT1("exo", "Surface::Damage", "damage", damage.ToString());
 
   pending_damage_.op(gfx::RectToSkIRect(damage), SkRegion::kUnion_Op);
 }
 
 void Surface::RequestFrameCallback(const FrameCallback& callback) {
   TRACE_EVENT0("exo", "Surface::RequestFrameCallback");
 
   pending_frame_callbacks_.push_back(callback);
 }
 
 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) {
   TRACE_EVENT1("exo", "Surface::SetInputRegion", "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) {
   TRACE_EVENT1("exo", "Surface::AddSubSurface", "sub_surface",
                sub_surface->AsTracedValue());
 
   DCHECK(!sub_surface->parent());
   DCHECK(!sub_surface->IsVisible());
   AddChild(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) {
   TRACE_EVENT1("exo", "Surface::AddSubSurface", "sub_surface",
                sub_surface->AsTracedValue());
 
   RemoveChild(sub_surface);
   if (sub_surface->IsVisible())
     sub_surface->Hide();
 
   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,
                                     const gfx::Point& position) {
   TRACE_EVENT2("exo", "Surface::SetSubSurfacePosition", "sub_surface",
                sub_surface->AsTracedValue(), "position", position.ToString());
 
   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) {
   TRACE_EVENT2("exo", "Surface::PlaceSubSurfaceAbove", "sub_surface",
                sub_surface->AsTracedValue(), "reference",
                reference->AsTracedValue());
 
   if (sub_surface == reference) {
     DLOG(WARNING) << "Client tried to place sub-surface above itself";
     return;
   }
 
   auto position_it = pending_sub_surfaces_.begin();
   if (reference != this) {
     position_it = FindListEntry(pending_sub_surfaces_, reference);
     if (position_it == pending_sub_surfaces_.end()) {
       DLOG(WARNING) << "Client tried to place sub-surface above a reference "
                        "surface that is neither a parent nor a sibling";
       return;
     }
 
     // Advance iterator to have |position_it| point to the sibling surface
     // above |reference|.
     ++position_it;
   }
 
   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) {
   TRACE_EVENT2("exo", "Surface::PlaceSubSurfaceBelow", "sub_surface",
                sub_surface->AsTracedValue(), "sibling",
                sibling->AsTracedValue());
 
   if (sub_surface == sibling) {
     DLOG(WARNING) << "Client tried to place sub-surface below itself";
     return;
   }
 
   auto sibling_it = FindListEntry(pending_sub_surfaces_, sibling);
   if (sibling_it == pending_sub_surfaces_.end()) {
     DLOG(WARNING) << "Client tried to place sub-surface below a surface that "
                      "is not a sibling";
     return;
   }
 
   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) {
   TRACE_EVENT1("exo", "Surface::SetOnlyVisibleOnSecureOutput",
                "only_visible_on_secure_output", only_visible_on_secure_output);
 
   pending_only_visible_on_secure_output_ = 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() {
   TRACE_EVENT0("exo", "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
   // new contents.
   only_visible_on_secure_output_ = pending_only_visible_on_secure_output_;
   pending_only_visible_on_secure_output_ = false;
 
   // Update current alpha.
   alpha_ = pending_alpha_;
 
@@ skipping 187 matching lines @@
   compositor_ = nullptr;
 }
 
 void Surface::WillDraw(cc::SurfaceId id) {
   while (!active_frame_callbacks_.empty()) {
     active_frame_callbacks_.front().Run(base::TimeTicks::Now());
     active_frame_callbacks_.pop_front();
   }
 }
 
+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_) {
     has_pending_contents_ = false;
 
     current_buffer_ = pending_buffer_;
     pending_buffer_.reset();
 
     cc::TextureMailbox texture_mailbox;
     std::unique_ptr<cc::SingleReleaseCallback> texture_mailbox_release_callback;
@@ skipping 90 matching lines @@
 
       factory_owner_->release_callbacks_[resource.id] = std::make_pair(
           factory_owner_, std::move(texture_mailbox_release_callback));
       current_resource_ = resource;
     } else {
       current_resource_.id = 0;
       current_resource_.size = gfx::Size();
     }
   }
 
-  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(
       gfx::ScaleSize(gfx::SizeF(buffer_size), 1.0f / pending_buffer_scale_));
 
   gfx::Size layer_size;  // Size of the output layer, in DIP.
   if (!pending_viewport_.IsEmpty()) {
     layer_size = pending_viewport_;
   } else if (!crop_.IsEmpty()) {
     DLOG_IF(WARNING, !gfx::IsExpressibleAsInt(crop_.width()) ||
@@ skipping 72 matching lines @@
 
   factory_owner_->surface_factory_->SubmitCompositorFrame(
       surface_id_, std::move(frame), cc::SurfaceFactory::DrawCallback());
 
   if (!old_surface_id.is_null()) {
     factory_owner_->surface_factory_->SetPreviousFrameSurface(surface_id_,
                                                               old_surface_id);
     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();
 
   DCHECK(!current_resource_.id ||
          factory_owner_->release_callbacks_.count(current_resource_.id));
 
   // Move pending frame callbacks to the end of active_frame_callbacks_
   active_frame_callbacks_.splice(active_frame_callbacks_.end(),
                                  pending_frame_callbacks_);
@@ skipping 32 matching lines @@
   layer->SetShowSurface(
       surface_id_,
       base::Bind(&SatisfyCallback, base::Unretained(surface_manager_)),
       base::Bind(&RequireCallback, base::Unretained(surface_manager_)),
       layer_size, contents_surface_to_layer_scale, layer_size);
 }
 
 bool Surface::use_surface_layer_ = false;
 
 }  // namespace exo