viz: Refactor SharedBitmapManager::ChildAddedSharedBitmap

This CL implements step 2 in the refactoring described by
http://crbug.com/951391, which attemps to port the viz
component and service to use our new shared memory API
(base::SharedMemory is deprecated).

It modifies SharedBitmapManager::ChildAddedSharedBitmap
to accept WritableSharedMemoryMapping values directly.

This simplifies the implementation and gets rid of
Mojo handle usage to pass shared memory region, in
favor of the new base::UnsafeSharedMemoryRegion and
base::WritableSharedMemoryMapping types.

BUG=951391,795291
R=alexilin@chromium.org, enne@chromium.org, danakj@chromium.org, piman@chromium.org, samans@chromium.org, liberato@chromium.org

Change-Id: I0314f43f64fc4cd5591c65816b764adb97fe6b13
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1571648
Commit-Queue: David Turner <digit@chromium.org>
Reviewed-by: Saman Sami <samans@chromium.org>
Cr-Commit-Position: refs/heads/master@{#658520}

cc/test/pixel_test.cc

@@ -214,12 +214,11 @@ bool PixelTest::PixelsMatchReference(const base::FilePath& ref_file,
 base::WritableSharedMemoryMapping PixelTest::AllocateSharedBitmapMemory(
     const viz::SharedBitmapId& id,
     const gfx::Size& size) {
-  base::MappedReadOnlyRegion mapped_region =
+  base::MappedReadOnlyRegion shm =
       viz::bitmap_allocation::AllocateSharedBitmap(size, viz::RGBA_8888);
-  this->shared_bitmap_manager_->ChildAllocatedSharedBitmap(
-      viz::bitmap_allocation::ToMojoHandle(std::move(mapped_region.region)),
-      id);
-  return std::move(mapped_region.mapping);
+  this->shared_bitmap_manager_->ChildAllocatedSharedBitmap(shm.region.Map(),
+                                                           id);
+  return std::move(shm.mapping);
 }
 
 viz::ResourceId PixelTest::AllocateAndFillSoftwareResource(

components/viz/service/display/display_resource_provider_unittest.cc

@@ -73,11 +73,10 @@ static SharedBitmapId CreateAndFillSharedBitmap(SharedBitmapManager* manager,
 
   base::MappedReadOnlyRegion shm =
       bitmap_allocation::AllocateSharedBitmap(size, RGBA_8888);
-  manager->ChildAllocatedSharedBitmap(
-      bitmap_allocation::ToMojoHandle(std::move(shm.region)), shared_bitmap_id);
-
-  auto* memory = static_cast<uint32_t*>(shm.mapping.memory());
-  std::fill_n(memory, size.GetArea(), value);
+  manager->ChildAllocatedSharedBitmap(shm.region.Map(), shared_bitmap_id);
+  base::span<uint32_t> span =
+      shm.mapping.GetMemoryAsSpan<uint32_t>(size.GetArea());
+  std::fill(span.begin(), span.end(), value);
   return shared_bitmap_id;
 }
 

components/viz/service/display/renderer_pixeltest.cc

@@ -71,8 +71,7 @@ base::WritableSharedMemoryMapping AllocateAndRegisterSharedBitmapMemory(
     SharedBitmapManager* shared_bitmap_manager) {
   base::MappedReadOnlyRegion shm =
       bitmap_allocation::AllocateSharedBitmap(size, RGBA_8888);
-  shared_bitmap_manager->ChildAllocatedSharedBitmap(
-      bitmap_allocation::ToMojoHandle(std::move(shm.region)), id);
+  shared_bitmap_manager->ChildAllocatedSharedBitmap(shm.region.Map(), id);
   return std::move(shm.mapping);
 }
 

components/viz/service/display/shared_bitmap_manager.h

@@ -8,8 +8,8 @@
 #include <memory>
 
 #include "base/macros.h"
+#include "base/memory/shared_memory_mapping.h"
 #include "components/viz/common/resources/shared_bitmap.h"
-#include "mojo/public/cpp/system/buffer.h"
 
 namespace gfx {
 class Size;
@@ -29,9 +29,10 @@ class SharedBitmapManager {
       const SharedBitmapId& id) = 0;
   virtual base::UnguessableToken GetSharedBitmapTracingGUIDFromId(
       const SharedBitmapId& id) = 0;
-  // Used in the display compositor to associate an id to a shm handle.
-  virtual bool ChildAllocatedSharedBitmap(mojo::ScopedSharedBufferHandle buffer,
-                                          const SharedBitmapId& id) = 0;
+  // Used in the display compositor to associate an id to a shm mapping.
+  virtual bool ChildAllocatedSharedBitmap(
+      base::ReadOnlySharedMemoryMapping mapping,
+      const SharedBitmapId& id) = 0;
   // Used in the display compositor to break an association of an id to a shm
   // handle.
   virtual void ChildDeletedSharedBitmap(const SharedBitmapId& id) = 0;

components/viz/service/display/software_renderer_unittest.cc

@@ -85,9 +85,8 @@ class SoftwareRendererTest : public testing::Test {
 
     // Registers the SharedBitmapId in the display compositor.
     SharedBitmapId shared_bitmap_id = SharedBitmap::GenerateId();
-    shared_bitmap_manager_->ChildAllocatedSharedBitmap(
-        bitmap_allocation::ToMojoHandle(std::move(shm.region)),
-        shared_bitmap_id);
+    shared_bitmap_manager_->ChildAllocatedSharedBitmap(shm.region.Map(),
+                                                       shared_bitmap_id);
 
     // Makes a resource id that refers to the registered SharedBitmapId.
     return child_resource_provider_->ImportResource(

components/viz/service/display_embedder/server_shared_bitmap_manager.cc

@@ -29,9 +29,9 @@ class BitmapData : public base::RefCounted<BitmapData> {
   explicit BitmapData(base::ReadOnlySharedMemoryMapping mapping)
       : mapping_(std::move(mapping)) {}
 
+  const void* memory() const { return mapping_.memory(); }
   size_t size() const { return mapping_.size(); }
   const base::UnguessableToken& mapped_id() const { return mapping_.guid(); }
-  const void* memory() const { return mapping_.memory(); }
 
  private:
   friend class base::RefCounted<BitmapData>;
@@ -43,9 +43,9 @@ class BitmapData : public base::RefCounted<BitmapData> {
 
 namespace {
 
-// Holds a reference on the BitmapData so that the SharedMemory can outlive the
-// SharedBitmapId registration as long as this SharedBitmap object is held
-// alive.
+// Holds a reference on the BitmapData so that the WritableSharedMemoryMapping
+// can outlive the SharedBitmapId registration as long as this SharedBitmap
+// object is held alive.
 class ServerSharedBitmap : public SharedBitmap {
  public:
   // NOTE: bitmap_data->memory() is read-only but SharedBitmap expects a
@@ -79,15 +79,17 @@ std::unique_ptr<SharedBitmap> ServerSharedBitmapManager::GetSharedBitmapFromId(
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
 
   auto it = handle_map_.find(id);
-  if (it == handle_map_.end())
+  if (it == handle_map_.end()) {
     return nullptr;
+  }
 
   BitmapData* data = it->second.get();
 
   size_t bitmap_size;
   if (!ResourceSizes::MaybeSizeInBytes(size, format, &bitmap_size) ||
-      bitmap_size > data->size())
+      bitmap_size > data->size()) {
     return nullptr;
+  }
 
   if (!data->memory()) {
     return nullptr;
@@ -107,7 +109,7 @@ ServerSharedBitmapManager::GetSharedBitmapTracingGUIDFromId(
 }
 
 bool ServerSharedBitmapManager::ChildAllocatedSharedBitmap(
-    mojo::ScopedSharedBufferHandle buffer,
+    base::ReadOnlySharedMemoryMapping mapping,
     const SharedBitmapId& id) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
 
@@ -115,15 +117,8 @@ bool ServerSharedBitmapManager::ChildAllocatedSharedBitmap(
   if (base::ContainsKey(handle_map_, id))
     return false;
 
-  base::ReadOnlySharedMemoryRegion region =
-      bitmap_allocation::FromMojoHandle(std::move(buffer));
-
   // This function handles public API requests, so verify we unwrapped a shared
   // memory handle before trying to use the handle.
-  if (!region.IsValid())
-    return false;
-
-  base::ReadOnlySharedMemoryMapping mapping = region.Map();
   if (!mapping.IsValid())
     return false;
 

components/viz/service/display_embedder/server_shared_bitmap_manager.h

@@ -38,7 +38,7 @@ class VIZ_SERVICE_EXPORT ServerSharedBitmapManager
       const SharedBitmapId& id) override;
   base::UnguessableToken GetSharedBitmapTracingGUIDFromId(
       const SharedBitmapId& id) override;
-  bool ChildAllocatedSharedBitmap(mojo::ScopedSharedBufferHandle buffer,
+  bool ChildAllocatedSharedBitmap(base::ReadOnlySharedMemoryMapping mapping,
                                   const SharedBitmapId& id) override;
   void ChildDeletedSharedBitmap(const SharedBitmapId& id) override;
 

components/viz/service/display_embedder/server_shared_bitmap_manager_unittest.cc

@@ -36,8 +36,7 @@ TEST_F(ServerSharedBitmapManagerTest, TestCreate) {
   std::fill(span.begin(), span.end(), 0xff);
 
   SharedBitmapId id = SharedBitmap::GenerateId();
-  manager()->ChildAllocatedSharedBitmap(
-      bitmap_allocation::ToMojoHandle(std::move(shm.region)), id);
+  manager()->ChildAllocatedSharedBitmap(shm.region.Map(), id);
 
   std::unique_ptr<SharedBitmap> large_bitmap;
   large_bitmap =
@@ -95,8 +94,7 @@ TEST_F(ServerSharedBitmapManagerTest, AddDuplicate) {
   SharedBitmapId id = SharedBitmap::GenerateId();
 
   // NOTE: Duplicate the mapping to compare its content later.
-  manager()->ChildAllocatedSharedBitmap(
-      bitmap_allocation::ToMojoHandle(std::move(shm.region)), id);
+  manager()->ChildAllocatedSharedBitmap(shm.region.Map(), id);
 
   base::MappedReadOnlyRegion shm2 =
       bitmap_allocation::AllocateSharedBitmap(bitmap_size, RGBA_8888);
@@ -104,8 +102,7 @@ TEST_F(ServerSharedBitmapManagerTest, AddDuplicate) {
   base::span<uint8_t> span2 = shm.mapping.GetMemoryAsSpan<uint8_t>();
   std::fill(span2.begin(), span2.end(), 0x00);
 
-  manager()->ChildAllocatedSharedBitmap(
-      bitmap_allocation::ToMojoHandle(std::move(shm2.region)), id);
+  manager()->ChildAllocatedSharedBitmap(shm2.region.Map(), id);
 
   std::unique_ptr<SharedBitmap> shared_bitmap;
   shared_bitmap = manager()->GetSharedBitmapFromId(bitmap_size, RGBA_8888, id);
@@ -125,8 +122,7 @@ TEST_F(ServerSharedBitmapManagerTest, SharedMemoryHandle) {
   EXPECT_FALSE(shared_memory_guid.is_empty());
 
   SharedBitmapId id = SharedBitmap::GenerateId();
-  manager()->ChildAllocatedSharedBitmap(
-      bitmap_allocation::ToMojoHandle(std::move(shm.region)), id);
+  manager()->ChildAllocatedSharedBitmap(shm.region.Map(), id);
 
   base::UnguessableToken tracing_guid =
       manager()->GetSharedBitmapTracingGUIDFromId(id);
@@ -137,10 +133,10 @@ TEST_F(ServerSharedBitmapManagerTest, SharedMemoryHandle) {
 
 TEST_F(ServerSharedBitmapManagerTest, InvalidScopedSharedBufferHandle) {
   SharedBitmapId id = SharedBitmap::GenerateId();
-  mojo::ScopedSharedBufferHandle invalid_handle(
-      mojo::SharedBufferHandle(0x1234567));
+  base::ReadOnlySharedMemoryMapping invalid_mapping;
+  EXPECT_FALSE(invalid_mapping.IsValid());
   EXPECT_FALSE(
-      manager()->ChildAllocatedSharedBitmap(std::move(invalid_handle), id));
+      manager()->ChildAllocatedSharedBitmap(std::move(invalid_mapping), id));
 
   // The client could still send an IPC to say it deleted the shared bitmap,
   // even though it wasn't valid, which should be ignored.

components/viz/service/frame_sinks/compositor_frame_sink_support.cc

@@ -13,6 +13,7 @@
 #include "base/time/time.h"
 #include "components/viz/common/frame_sinks/begin_frame_source.h"
 #include "components/viz/common/quads/compositor_frame.h"
+#include "components/viz/common/resources/bitmap_allocation.h"
 #include "components/viz/common/surfaces/surface_info.h"
 #include "components/viz/service/display/display.h"
 #include "components/viz/service/display/shared_bitmap_manager.h"
@@ -324,8 +325,9 @@ bool CompositorFrameSinkSupport::DidAllocateSharedBitmap(
     mojo::ScopedSharedBufferHandle buffer,
     const SharedBitmapId& id) {
   if (!frame_sink_manager_->shared_bitmap_manager()->ChildAllocatedSharedBitmap(
-          std::move(buffer), id))
+          bitmap_allocation::FromMojoHandle(std::move(buffer)).Map(), id)) {
     return false;
+  }
 
   owned_bitmaps_.insert(id);
   return true;

components/viz/test/test_layer_tree_frame_sink.cc

@@ -13,6 +13,7 @@
 #include "base/single_thread_task_runner.h"
 #include "cc/trees/layer_tree_frame_sink_client.h"
 #include "components/viz/common/frame_sinks/begin_frame_args.h"
+#include "components/viz/common/resources/bitmap_allocation.h"
 #include "components/viz/service/display/direct_renderer.h"
 #include "components/viz/service/display/output_surface.h"
 #include "components/viz/service/display/skia_output_surface.h"
@@ -204,8 +205,8 @@ void TestLayerTreeFrameSink::DidNotProduceFrame(const BeginFrameAck& ack) {
 void TestLayerTreeFrameSink::DidAllocateSharedBitmap(
     mojo::ScopedSharedBufferHandle buffer,
     const SharedBitmapId& id) {
-  bool ok =
-      shared_bitmap_manager_->ChildAllocatedSharedBitmap(std::move(buffer), id);
+  bool ok = shared_bitmap_manager_->ChildAllocatedSharedBitmap(
+      bitmap_allocation::FromMojoHandle(std::move(buffer)).Map(), id);
   DCHECK(ok);
   owned_bitmaps_.insert(id);
 }

components/viz/test/test_shared_bitmap_manager.cc

@@ -30,8 +30,7 @@ std::unique_ptr<SharedBitmap> TestSharedBitmapManager::GetSharedBitmapFromId(
     return nullptr;
   // NOTE: pixels needs to be writable for legacy reasons, but SharedBitmap
   // instances returned by a SharedBitmapManager are always read-only.
-  uint8_t* pixels =
-      static_cast<uint8_t*>(const_cast<void*>(it->second.memory()));
+  auto* pixels = static_cast<uint8_t*>(const_cast<void*>(it->second.memory()));
   return std::make_unique<SharedBitmap>(pixels);
 }
 
@@ -45,7 +44,7 @@ TestSharedBitmapManager::GetSharedBitmapTracingGUIDFromId(
 }
 
 bool TestSharedBitmapManager::ChildAllocatedSharedBitmap(
-    mojo::ScopedSharedBufferHandle buffer,
+    base::ReadOnlySharedMemoryMapping mapping,
     const SharedBitmapId& id) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
 
@@ -53,11 +52,6 @@ bool TestSharedBitmapManager::ChildAllocatedSharedBitmap(
   // notification here should be about a bitmap that was previously allocated
   // with AllocateSharedBitmap().
   if (mapping_map_.find(id) == mapping_map_.end()) {
-    base::ReadOnlySharedMemoryRegion region =
-        bitmap_allocation::FromMojoHandle(std::move(buffer));
-    DCHECK(region.IsValid());
-    base::ReadOnlySharedMemoryMapping mapping = region.Map();
-    DCHECK(mapping.IsValid());
     mapping_map_.emplace(id, std::move(mapping));
   }
 

components/viz/test/test_shared_bitmap_manager.h

@@ -26,7 +26,7 @@ class TestSharedBitmapManager : public SharedBitmapManager {
       const SharedBitmapId& id) override;
   base::UnguessableToken GetSharedBitmapTracingGUIDFromId(
       const SharedBitmapId& id) override;
-  bool ChildAllocatedSharedBitmap(mojo::ScopedSharedBufferHandle buffer,
+  bool ChildAllocatedSharedBitmap(base::ReadOnlySharedMemoryMapping mapping,
                                   const SharedBitmapId& id) override;
   void ChildDeletedSharedBitmap(const SharedBitmapId& id) override;
 

content/browser/renderer_host/render_widget_host_impl.cc

@@ -21,7 +21,6 @@
 #include "base/lazy_instance.h"
 #include "base/location.h"
 #include "base/macros.h"
-#include "base/memory/shared_memory.h"
 #include "base/message_loop/message_loop.h"
 #include "base/metrics/field_trial.h"
 #include "base/metrics/histogram_macros.h"
@@ -37,6 +36,7 @@
 #include "cc/trees/render_frame_metadata.h"
 #include "components/viz/common/features.h"
 #include "components/viz/common/quads/compositor_frame.h"
+#include "components/viz/common/resources/bitmap_allocation.h"
 #include "components/viz/host/host_frame_sink_manager.h"
 #include "components/viz/service/display_embedder/server_shared_bitmap_manager.h"
 #include "content/browser/accessibility/browser_accessibility_state_impl.h"
@@ -2354,8 +2354,9 @@ void RenderWidgetHostImpl::DidNotProduceFrame(const viz::BeginFrameAck& ack) {
 void RenderWidgetHostImpl::DidAllocateSharedBitmap(
     mojo::ScopedSharedBufferHandle buffer,
     const viz::SharedBitmapId& id) {
-  if (!shared_bitmap_manager_->ChildAllocatedSharedBitmap(std::move(buffer),
-                                                          id)) {
+  if (!shared_bitmap_manager_->ChildAllocatedSharedBitmap(
+          viz::bitmap_allocation::FromMojoHandle(std::move(buffer)).Map(),
+          id)) {
     bad_message::ReceivedBadMessage(GetProcess(),
                                     bad_message::RWH_SHARED_BITMAP);
   }