PPAPI: Upgrade video decoder host shared memory API.

Use an UnsafeSharedMemoryRegion instead of the existing handle. The
shared memory on the host side is used read-only, but because a
writable region needs to be shipped to the other side of the proxy, an
unsafe region needs to be used.

Host                         | Other side of proxy
-----------------------------+---------------------------------
                             |   Request SHM
 Create SHM <-----------------------/
 Reply with SHM Handle       |
   | \-------------------------> Receive SHM
 Save SHM by ID              |
                             |
                             |   Fill SHM with video to decode
                             |       (write to SHM)
                             |   Send SHM ID to Host
 Receive decode request <-----------/
 Look up buffer by ID        |
 Decode what's in the SHM    |
  (read-only SHM access)

The host-side could use a read-only region only by adding an additional
round-trip, with the other side of the proxy either converting to
read-only after mapping writable, and shipping back to the host,
or the other side of the proxy mapping, shipping back a writable
handle, and then the host converting to read-only. This has not been
done in this CL.

Bug: 849207
Change-Id: I3e50f9ff9c65e51c21c7e4d72b3aed2402c03196
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1615021
Commit-Queue: Matthew Cary (CET) <mattcary@chromium.org>
Reviewed-by: Chris Palmer <palmer@chromium.org>
Reviewed-by: Antoine Labour <piman@chromium.org>
Reviewed-by: Bill Budge <bbudge@chromium.org>
Cr-Commit-Position: refs/heads/master@{#666194}

content/renderer/pepper/pepper_video_decoder_host.cc

@@ -22,6 +22,7 @@
 #include "media/base/media_util.h"
 #include "media/gpu/ipc/client/gpu_video_decode_accelerator_host.h"
 #include "media/video/video_decode_accelerator.h"
+#include "mojo/public/cpp/base/shared_memory_utils.h"
 #include "ppapi/c/pp_completion_callback.h"
 #include "ppapi/c/pp_errors.h"
 #include "ppapi/host/dispatch_host_message.h"
@@ -87,6 +88,17 @@ PepperVideoDecoderHost::PendingDecode::PendingDecode(
 
 PepperVideoDecoderHost::PendingDecode::~PendingDecode() {}
 
+PepperVideoDecoderHost::MappedBuffer::MappedBuffer(
+    base::UnsafeSharedMemoryRegion region,
+    base::WritableSharedMemoryMapping mapping)
+    : region(std::move(region)), mapping(std::move(mapping)) {}
+
+PepperVideoDecoderHost::MappedBuffer::~MappedBuffer() {}
+
+PepperVideoDecoderHost::MappedBuffer::MappedBuffer(MappedBuffer&&) = default;
+PepperVideoDecoderHost::MappedBuffer& PepperVideoDecoderHost::MappedBuffer::
+operator=(MappedBuffer&&) = default;
+
 PepperVideoDecoderHost::PepperVideoDecoderHost(RendererPpapiHost* host,
                                                PP_Instance instance,
                                                PP_Resource resource)
@@ -194,26 +206,24 @@ int32_t PepperVideoDecoderHost::OnHostMsgGetShm(
   if (shm_id > shm_buffers_.size())
     return PP_ERROR_FAILED;
   // Reject an attempt to reallocate a busy shm buffer.
-  if (shm_id < shm_buffers_.size() && shm_buffer_busy_[shm_id])
+  if (shm_id < shm_buffers_.size() && shm_buffers_[shm_id].busy)
     return PP_ERROR_FAILED;
 
-  content::RenderThread* render_thread = content::RenderThread::Get();
-  std::unique_ptr<base::SharedMemory> shm(
-      render_thread->HostAllocateSharedMemoryBuffer(shm_size));
-  if (!shm || !shm->Map(shm_size))
+  auto shm = mojo::CreateUnsafeSharedMemoryRegion(shm_size);
+  auto mapping = shm.Map();
+  if (!shm.IsValid() || !mapping.IsValid())
     return PP_ERROR_FAILED;
 
-  base::SharedMemoryHandle shm_handle = shm->handle();
+  SerializedHandle handle(
+      base::UnsafeSharedMemoryRegion::TakeHandleForSerialization(
+          renderer_ppapi_host_->ShareUnsafeSharedMemoryRegionWithRemote(shm)));
   if (shm_id == shm_buffers_.size()) {
-    shm_buffers_.push_back(std::move(shm));
-    shm_buffer_busy_.push_back(false);
+    shm_buffers_.emplace_back(std::move(shm), std::move(mapping));
   } else {
-    shm_buffers_[shm_id] = std::move(shm);
+    // Note by the check above this buffer cannot be busy.
+    shm_buffers_[shm_id] = MappedBuffer(std::move(shm), std::move(mapping));
   }
 
-  SerializedHandle handle(
-      renderer_ppapi_host_->ShareSharedMemoryHandleWithRemote(shm_handle),
-      shm_size);
   ppapi::host::ReplyMessageContext reply_context =
       context->MakeReplyMessageContext();
   reply_context.params.AppendHandle(std::move(handle));
@@ -235,7 +245,7 @@ int32_t PepperVideoDecoderHost::OnHostMsgDecode(
   if (static_cast<size_t>(shm_id) >= shm_buffers_.size())
     return PP_ERROR_FAILED;
   // Reject an attempt to pass a busy buffer to the decoder again.
-  if (shm_buffer_busy_[shm_id])
+  if (shm_buffers_[shm_id].busy)
     return PP_ERROR_FAILED;
   // Reject non-unique decode_id values.
   if (GetPendingDecodeById(decode_id) != pending_decodes_.end())
@@ -247,12 +257,12 @@ int32_t PepperVideoDecoderHost::OnHostMsgDecode(
   pending_decodes_.push_back(PendingDecode(decode_id, shm_id, size,
                                            context->MakeReplyMessageContext()));
 
-  shm_buffer_busy_[shm_id] = true;
-  // TODO(crbug.com/844456): The decode buffer should probably be read-only, but
-  // then shm_buffers_ will need to be refactored to use a
-  // ReadOnlySharedMemoryRegion with an associated writable mapping.
+  shm_buffers_[shm_id].busy = true;
   decoder_->Decode(media::BitstreamBuffer(
-      decode_id, shm_buffers_[shm_id]->handle(), false /* read_only */, size));
+      decode_id,
+      base::UnsafeSharedMemoryRegion::TakeHandleForSerialization(
+          shm_buffers_[shm_id].region.Duplicate()),
+      size));
 
   return PP_OK_COMPLETIONPENDING;
 }
@@ -443,7 +453,7 @@ void PepperVideoDecoderHost::NotifyEndOfBitstreamBuffer(
   }
   host()->SendReply(it->reply_context,
                     PpapiPluginMsg_VideoDecoder_DecodeReply(it->shm_id));
-  shm_buffer_busy_[it->shm_id] = false;
+  shm_buffers_[it->shm_id].busy = false;
   pending_decodes_.erase(it);
 }
 
@@ -492,7 +502,7 @@ const uint8_t* PepperVideoDecoderHost::DecodeIdToAddress(uint32_t decode_id) {
   PendingDecodeList::const_iterator it = GetPendingDecodeById(decode_id);
   DCHECK(it != pending_decodes_.end());
   uint32_t shm_id = it->shm_id;
-  return static_cast<uint8_t*>(shm_buffers_[shm_id]->memory());
+  return static_cast<uint8_t*>(shm_buffers_[shm_id].mapping.memory());
 }
 
 bool PepperVideoDecoderHost::TryFallbackToSoftwareDecoder() {
@@ -538,8 +548,8 @@ bool PepperVideoDecoderHost::TryFallbackToSoftwareDecoder() {
       const PendingDecode& decode = pending_decodes_.front();
       host()->SendReply(decode.reply_context,
                         PpapiPluginMsg_VideoDecoder_DecodeReply(decode.shm_id));
-      DCHECK(shm_buffer_busy_[decode.shm_id]);
-      shm_buffer_busy_[decode.shm_id] = false;
+      DCHECK(shm_buffers_[decode.shm_id].busy);
+      shm_buffers_[decode.shm_id].busy = false;
       pending_decodes_.pop_front();
     }
     NotifyResetDone();
@@ -547,12 +557,12 @@ bool PepperVideoDecoderHost::TryFallbackToSoftwareDecoder() {
 
   // Resubmit all pending decodes.
   for (const PendingDecode& decode : pending_decodes_) {
-    DCHECK(shm_buffer_busy_[decode.shm_id]);
-    // TODO(crbug.com/844456): As with OnHostMsgDecode, the decode buffer should
-    // probably be read-only (see the todo there for more details).
+    DCHECK(shm_buffers_[decode.shm_id].busy);
     decoder_->Decode(media::BitstreamBuffer(
-        decode.decode_id, shm_buffers_[decode.shm_id]->handle(),
-        false /* read_only */, decode.size));
+        decode.decode_id,
+        base::UnsafeSharedMemoryRegion::TakeHandleForSerialization(
+            shm_buffers_[decode.shm_id].region.Duplicate()),
+        decode.size));
   }
 
   // Flush the new decoder if Flush() was pending.

content/renderer/pepper/pepper_video_decoder_host.h

@@ -22,10 +22,6 @@
 #include "ppapi/host/resource_host.h"
 #include "ppapi/proxy/resource_message_params.h"
 
-namespace base {
-class SharedMemory;
-}
-
 namespace content {
 
 class RendererPpapiHost;
@@ -61,6 +57,19 @@ class CONTENT_EXPORT PepperVideoDecoderHost
   };
   typedef std::list<PendingDecode> PendingDecodeList;
 
+  struct MappedBuffer {
+    MappedBuffer(base::UnsafeSharedMemoryRegion region,
+                 base::WritableSharedMemoryMapping mapping);
+    ~MappedBuffer();
+
+    MappedBuffer(MappedBuffer&&);
+    MappedBuffer& operator=(MappedBuffer&&);
+
+    base::UnsafeSharedMemoryRegion region;
+    base::WritableSharedMemoryMapping mapping;
+    bool busy = false;
+  };
+
   friend class VideoDecoderShim;
 
   // ResourceHost implementation.
@@ -137,10 +146,13 @@ class CONTENT_EXPORT PepperVideoDecoderHost
   // resource. We use a buffer's index in these vectors as its id on both sides
   // of the proxy. Only add buffers or update them in place so as not to
   // invalidate these ids.
-  std::vector<std::unique_ptr<base::SharedMemory>> shm_buffers_;
-  // A vector of true/false indicating if a shm buffer is in use by the decoder.
-  // This is parallel to |shm_buffers_|.
-  std::vector<uint8_t> shm_buffer_busy_;
+  //
+  // These regions are created here, in the host, and shared with the other side
+  // of the proxy who will write into them. While they are only used in a
+  // read-only way in the host, using a ReadOnlySharedMemoryRegion would involve
+  // an extra round-trip to allow the other side of the proxy to map the region
+  // writable before sending a read-only region back to the host.
+  std::vector<MappedBuffer> shm_buffers_;
 
   uint32_t min_picture_count_;
   typedef std::map<uint32_t, PictureBufferState> PictureBufferMap;

ppapi/proxy/resource_message_params.cc

@@ -113,6 +113,18 @@ bool ResourceMessageParams::TakeReadOnlySharedMemoryRegionAtIndex(
   return true;
 }
 
+bool ResourceMessageParams::TakeUnsafeSharedMemoryRegionAtIndex(
+    size_t index,
+    base::UnsafeSharedMemoryRegion* region) const {
+  SerializedHandle serialized =
+      TakeHandleOfTypeAtIndex(index, SerializedHandle::SHARED_MEMORY_REGION);
+  if (!serialized.is_shmem_region())
+    return false;
+  *region = base::UnsafeSharedMemoryRegion::Deserialize(
+      serialized.TakeSharedMemoryRegion());
+  return true;
+}
+
 bool ResourceMessageParams::TakeSocketHandleAtIndex(
     size_t index,
     IPC::PlatformFileForTransit* handle) const {

ppapi/proxy/resource_message_params.h

@@ -67,6 +67,9 @@ class PPAPI_PROXY_EXPORT ResourceMessageParams {
   bool TakeReadOnlySharedMemoryRegionAtIndex(
       size_t index,
       base::ReadOnlySharedMemoryRegion* region) const;
+  bool TakeUnsafeSharedMemoryRegionAtIndex(
+      size_t index,
+      base::UnsafeSharedMemoryRegion* region) const;
   bool TakeSocketHandleAtIndex(size_t index,
                                IPC::PlatformFileForTransit* handle) const;
   bool TakeFileHandleAtIndex(size_t index,

ppapi/proxy/serialized_handle.cc

@@ -66,6 +66,16 @@ SerializedHandle::SerializedHandle(const base::SharedMemoryHandle& handle,
       open_flags_(0),
       file_io_(0) {}
 
+SerializedHandle::SerializedHandle(base::ReadOnlySharedMemoryRegion region)
+    : SerializedHandle(
+          base::ReadOnlySharedMemoryRegion::TakeHandleForSerialization(
+              std::move(region))) {}
+
+SerializedHandle::SerializedHandle(base::UnsafeSharedMemoryRegion region)
+    : SerializedHandle(
+          base::UnsafeSharedMemoryRegion::TakeHandleForSerialization(
+              std::move(region))) {}
+
 SerializedHandle::SerializedHandle(
     base::subtle::PlatformSharedMemoryRegion region)
     : type_(SHARED_MEMORY_REGION),

ppapi/proxy/serialized_handle.h

@@ -13,8 +13,10 @@
 #include "base/atomicops.h"
 #include "base/logging.h"
 #include "base/memory/platform_shared_memory_region.h"
+#include "base/memory/read_only_shared_memory_region.h"
 #include "base/memory/ref_counted.h"
 #include "base/memory/shared_memory.h"
+#include "base/memory/unsafe_shared_memory_region.h"
 #include "build/build_config.h"
 #include "ipc/ipc_platform_file.h"
 #include "ppapi/c/pp_resource.h"
@@ -66,6 +68,8 @@ class PPAPI_PROXY_EXPORT SerializedHandle {
   SerializedHandle(const base::SharedMemoryHandle& handle, uint32_t size);
 
   // Create a shared memory region handle.
+  explicit SerializedHandle(base::ReadOnlySharedMemoryRegion region);
+  explicit SerializedHandle(base::UnsafeSharedMemoryRegion region);
   explicit SerializedHandle(base::subtle::PlatformSharedMemoryRegion region);
 
   // Create a socket or file handle.

ppapi/proxy/video_decoder_resource.cc

@@ -32,12 +32,12 @@ namespace ppapi {
 namespace proxy {
 
 VideoDecoderResource::ShmBuffer::ShmBuffer(
-    std::unique_ptr<base::SharedMemory> shm_ptr,
-    uint32_t size,
+    base::UnsafeSharedMemoryRegion region,
     uint32_t shm_id)
-    : shm(std::move(shm_ptr)), addr(NULL), shm_id(shm_id) {
-  if (shm->Map(size))
-    addr = shm->memory();
+    : region(std::move(region)), shm_id(shm_id) {
+  mapping = this->region.Map();
+  if (mapping.IsValid())
+    addr = mapping.memory();
 }
 
 VideoDecoderResource::ShmBuffer::~ShmBuffer() {
@@ -199,7 +199,7 @@ int32_t VideoDecoderResource::Decode(uint32_t decode_id,
   decode_ids_[uid % kMaximumPictureDelay] = decode_id;
 
   if (available_shm_buffers_.empty() ||
-      available_shm_buffers_.back()->shm->mapped_size() < size) {
+      available_shm_buffers_.back()->mapping.size() < size) {
     uint32_t shm_id;
     if (shm_buffers_.size() < kMaximumPendingDecodes) {
       // Signal the host to create a new shm buffer by passing an index outside
@@ -227,13 +227,12 @@ int32_t VideoDecoderResource::Decode(uint32_t decode_id,
     if (!UnpackMessage<PpapiPluginMsg_VideoDecoder_GetShmReply>(reply,
                                                                 &shm_size))
       return PP_ERROR_FAILED;
-    base::SharedMemoryHandle shm_handle;
-    if (!reply_params.TakeSharedMemoryHandleAtIndex(0, &shm_handle))
+    base::UnsafeSharedMemoryRegion shm_region;
+    if (!reply_params.TakeUnsafeSharedMemoryRegionAtIndex(0, &shm_region) ||
+        !shm_region.IsValid() || shm_region.GetSize() != shm_size)
       return PP_ERROR_NOMEMORY;
-    std::unique_ptr<base::SharedMemory> shm(
-        new base::SharedMemory(shm_handle, false /* read_only */));
     std::unique_ptr<ShmBuffer> shm_buffer(
-        new ShmBuffer(std::move(shm), shm_size, shm_id));
+        new ShmBuffer(std::move(shm_region), shm_id));
     if (!shm_buffer->addr)
       return PP_ERROR_NOMEMORY;
 
@@ -246,7 +245,7 @@ int32_t VideoDecoderResource::Decode(uint32_t decode_id,
 
   // At this point we should have shared memory to hold the plugin's buffer.
   DCHECK(!available_shm_buffers_.empty() &&
-         available_shm_buffers_.back()->shm->mapped_size() >= size);
+         available_shm_buffers_.back()->mapping.size() >= size);
 
   ShmBuffer* shm_buffer = available_shm_buffers_.back();
   available_shm_buffers_.pop_back();

ppapi/proxy/video_decoder_resource.h

@@ -83,13 +83,12 @@ class PPAPI_PROXY_EXPORT VideoDecoderResource
  private:
   // Struct to hold a shared memory buffer.
   struct ShmBuffer {
-    ShmBuffer(std::unique_ptr<base::SharedMemory> shm,
-              uint32_t size,
-              uint32_t shm_id);
+    ShmBuffer(base::UnsafeSharedMemoryRegion region, uint32_t shm_id);
     ~ShmBuffer();
 
-    const std::unique_ptr<base::SharedMemory> shm;
-    void* addr;
+    base::UnsafeSharedMemoryRegion region;
+    base::WritableSharedMemoryMapping mapping;
+    void* addr = nullptr;
     // Index into shm_buffers_ vector, used as an id. This should map 1:1 to
     // the index on the host side of the proxy.
     const uint32_t shm_id;

ppapi/proxy/video_decoder_resource_unittest.cc

@@ -6,7 +6,7 @@
 #include <stddef.h>
 #include <stdint.h>
 
-#include "base/memory/shared_memory.h"
+#include "base/memory/unsafe_shared_memory_region.h"
 #include "build/build_config.h"
 #include "ppapi/c/pp_errors.h"
 #include "ppapi/c/ppb_video_decoder.h"
@@ -137,11 +137,10 @@ class VideoDecoderResourceTest : public PluginProxyTest {
     sink().AddFilter(&shm_msg_handler);
 
     if (expected_shm_msg) {
-      std::unique_ptr<SerializedHandle> serialized_handle;
-      base::SharedMemory shm;
-      shm.CreateAnonymous(kShmSize);
-      base::SharedMemoryHandle shm_handle = shm.handle().Duplicate();
-      serialized_handle.reset(new SerializedHandle(shm_handle, kShmSize));
+      auto region = base::UnsafeSharedMemoryRegion::Create(kShmSize);
+      auto serialized_handle = std::make_unique<SerializedHandle>(
+          base::UnsafeSharedMemoryRegion::TakeHandleForSerialization(
+              std::move(region)));
       shm_msg_handler.set_serialized_handle(std::move(serialized_handle));
     }