[Video Capture Service] Allow buffer retired while still in use in BroadcastingReceiver

In the recently added class BroadcastingReceiver for enabling multi-client
access to video capture devices, see design doc [1], an assumption was made
that producers never retire buffers while they are still in use. Even though
this assumption may have held at some point during development, a test
failure at a CL [2] that moves the service to the browser process on
ChromeOS revealed that it no longer holds.

This CL removes this assumption and adds correct handling of the case.

[1] https://docs.google.com/document/d/1mYnsZfLBRmbsDpUtfb6C7dzhfw2Kcxg_-uiG_6MnWVQ/edit?usp=sharing
[2] https://chromium-review.googlesource.com/c/chromium/src/+/1506604/1

test: services_unittests --gtest_filter=BroadcastingReceiverTest.*
test: services_unittests --gtest_filter=MockVideoCaptureDeviceSharedAccessTest.*
test: content_browsertests --gtest_filter=WebRtcVideoCaptureServiceBrowserTest.*

Bug: 783442, 939587
Change-Id: I9d27c69386e76c2ea472f8549721fa5cf890ae61
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1509902
Reviewed-by: Robert Sesek <[email protected]>
Reviewed-by: Emircan Uysaler <[email protected]>
Commit-Queue: Christian Fremerey <[email protected]>
Cr-Original-Commit-Position: refs/heads/master@{#639747}(cherry picked from commit ce0b05d2aa49a6b22c1033c4d1081a3d923248a7)
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1520348
Reviewed-by: Christian Fremerey <[email protected]>
Cr-Commit-Position: refs/branch-heads/3729@{#85}
Cr-Branched-From: d4a8972-refs/heads/master@{#638880}

Author: Christian Fremerey

services/video_capture/BUILD.gn

@@ -76,6 +76,7 @@ source_set("tests") {
   testonly = true
 
   sources = [
+    "broadcasting_receiver_unittest.cc",
     "device_media_to_mojo_adapter_unittest.cc",
     "test/device_factory_provider_connectortest.cc",
     "test/device_factory_provider_test.cc",

services/video_capture/broadcasting_receiver.cc

@@ -95,7 +95,11 @@ BroadcastingReceiver::BufferContext::BufferContext(
     media::mojom::VideoBufferHandlePtr buffer_handle)
     : buffer_id_(buffer_id),
       buffer_handle_(std::move(buffer_handle)),
-      consumer_hold_count_(0) {}
+      consumer_hold_count_(0),
+      is_retired_(false) {
+  static int next_buffer_context_id = 0;
+  buffer_context_id_ = next_buffer_context_id++;
+}
 
 BroadcastingReceiver::BufferContext::~BufferContext() = default;
 
@@ -233,8 +237,10 @@ int32_t BroadcastingReceiver::AddClient(
   }
 
   for (auto& buffer_context : buffer_contexts_) {
+    if (buffer_context.is_retired())
+      continue;
     added_client_context.client()->OnNewBuffer(
-        buffer_context.buffer_id(),
+        buffer_context.buffer_context_id(),
         buffer_context.CloneBufferHandle(
             added_client_context.target_buffer_type()));
   }
@@ -260,11 +266,13 @@ void BroadcastingReceiver::OnNewBuffer(
     int32_t buffer_id,
     media::mojom::VideoBufferHandlePtr buffer_handle) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+  CHECK(FindUnretiredBufferContextFromBufferId(buffer_id) ==
+        buffer_contexts_.end());
   buffer_contexts_.emplace_back(buffer_id, std::move(buffer_handle));
   auto& buffer_context = buffer_contexts_.back();
   for (auto& client : clients_) {
     client.second.client()->OnNewBuffer(
-        buffer_context.buffer_id(),
+        buffer_context.buffer_context_id(),
         buffer_context.CloneBufferHandle(client.second.target_buffer_type()));
   }
 }
@@ -277,7 +285,9 @@ void BroadcastingReceiver::OnFrameReadyInBuffer(
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   if (clients_.empty())
     return;
-  auto& buffer_context = LookupBufferContextFromBufferId(buffer_id);
+  auto buffer_context_iter = FindUnretiredBufferContextFromBufferId(buffer_id);
+  CHECK(buffer_context_iter != buffer_contexts_.end());
+  auto& buffer_context = *buffer_context_iter;
   buffer_context.set_access_permission(std::move(access_permission));
   for (auto& client : clients_) {
     if (client.second.is_suspended())
@@ -286,27 +296,29 @@ void BroadcastingReceiver::OnFrameReadyInBuffer(
     mojo::MakeStrongBinding(
         std::make_unique<ConsumerAccessPermission>(base::BindOnce(
             &BroadcastingReceiver::OnClientFinishedConsumingFrame,
-            weak_factory_.GetWeakPtr(), buffer_context.buffer_id())),
+            weak_factory_.GetWeakPtr(), buffer_context.buffer_context_id())),
         mojo::MakeRequest(&consumer_access_permission));
     client.second.client()->OnFrameReadyInBuffer(
-        buffer_context.buffer_id(), frame_feedback_id,
+        buffer_context.buffer_context_id(), frame_feedback_id,
         std::move(consumer_access_permission), frame_info.Clone());
     buffer_context.IncreaseConsumerCount();
   }
 }
 
 void BroadcastingReceiver::OnBufferRetired(int32_t buffer_id) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
-  auto context_iter =
-      std::find_if(buffer_contexts_.begin(), buffer_contexts_.end(),
-                   [buffer_id](const BufferContext& entry) {
-                     return entry.buffer_id() == buffer_id;
-                   });
-  auto& buffer_context = *context_iter;
-  CHECK(!buffer_context.IsStillBeingConsumed());
-  buffer_contexts_.erase(context_iter);
+  auto buffer_context_iter = FindUnretiredBufferContextFromBufferId(buffer_id);
+  CHECK(buffer_context_iter != buffer_contexts_.end());
+  const auto context_id = buffer_context_iter->buffer_context_id();
+  if (buffer_context_iter->IsStillBeingConsumed())
+    // Mark the buffer context as retired but keep holding on to it until the
+    // last client finished consuming it, because it contains the
+    // |access_permission| required during consumption.
+    buffer_context_iter->set_retired();
+  else
+    buffer_contexts_.erase(buffer_context_iter);
   for (auto& client : clients_) {
-    client.second.client()->OnBufferRetired(buffer_id);
+    client.second.client()->OnBufferRetired(context_id);
   }
 }
 
@@ -367,26 +379,36 @@ void BroadcastingReceiver::OnStopped() {
   }
 }
 
-void BroadcastingReceiver::OnClientFinishedConsumingFrame(int32_t buffer_id) {
+void BroadcastingReceiver::OnClientFinishedConsumingFrame(
+    int32_t buffer_context_id) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
-  auto& buffer_context = LookupBufferContextFromBufferId(buffer_id);
-  buffer_context.DecreaseConsumerCount();
+  auto buffer_context_iter =
+      std::find_if(buffer_contexts_.begin(), buffer_contexts_.end(),
+                   [buffer_context_id](const BufferContext& entry) {
+                     return entry.buffer_context_id() == buffer_context_id;
+                   });
+  CHECK(buffer_context_iter != buffer_contexts_.end());
+  buffer_context_iter->DecreaseConsumerCount();
+  if (buffer_context_iter->is_retired() &&
+      !buffer_context_iter->IsStillBeingConsumed()) {
+    buffer_contexts_.erase(buffer_context_iter);
+  }
 }
 
 void BroadcastingReceiver::OnClientDisconnected(int32_t client_id) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   clients_.erase(client_id);
 }
 
-BroadcastingReceiver::BufferContext&
-BroadcastingReceiver::LookupBufferContextFromBufferId(int32_t buffer_id) {
+std::vector<BroadcastingReceiver::BufferContext>::iterator
+BroadcastingReceiver::FindUnretiredBufferContextFromBufferId(
+    int32_t buffer_id) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
-  auto context_iter =
-      std::find_if(buffer_contexts_.begin(), buffer_contexts_.end(),
-                   [buffer_id](const BufferContext& entry) {
-                     return entry.buffer_id() == buffer_id;
-                   });
-  return *context_iter;
+  return std::find_if(buffer_contexts_.begin(), buffer_contexts_.end(),
+                      [buffer_id](const BufferContext& entry) {
+                        return !entry.is_retired() &&
+                               entry.buffer_id() == buffer_id;
+                      });
 }
 
 }  // namespace video_capture

services/video_capture/broadcasting_receiver.h

@@ -103,6 +103,7 @@ class BroadcastingReceiver : public mojom::Receiver {
     ~BufferContext();
     BufferContext(BufferContext&& other);
     BufferContext& operator=(BufferContext&& other);
+    int32_t buffer_context_id() const { return buffer_context_id_; }
     int32_t buffer_id() const { return buffer_id_; }
     void set_access_permission(
         mojom::ScopedAccessPermissionPtr access_permission) {
@@ -111,6 +112,8 @@ class BroadcastingReceiver : public mojom::Receiver {
     void IncreaseConsumerCount();
     void DecreaseConsumerCount();
     bool IsStillBeingConsumed() const;
+    bool is_retired() const { return is_retired_; }
+    void set_retired() { is_retired_ = true; }
     media::mojom::VideoBufferHandlePtr CloneBufferHandle(
         media::VideoCaptureBufferType target_buffer_type);
 
@@ -121,17 +124,20 @@ class BroadcastingReceiver : public mojom::Receiver {
     // a regular shared memory and keep it in this form.
     void ConvertRawFileDescriptorToSharedBuffer();
 
+    int32_t buffer_context_id_;
     int32_t buffer_id_;
     media::mojom::VideoBufferHandlePtr buffer_handle_;
     // Indicates how many consumers are currently relying on
     // |access_permission_|.
     int32_t consumer_hold_count_;
+    bool is_retired_;
     mojom::ScopedAccessPermissionPtr access_permission_;
   };
 
-  void OnClientFinishedConsumingFrame(int32_t buffer_id);
+  void OnClientFinishedConsumingFrame(int32_t buffer_context_id);
   void OnClientDisconnected(int32_t client_id);
-  BufferContext& LookupBufferContextFromBufferId(int32_t buffer_id);
+  std::vector<BufferContext>::iterator FindUnretiredBufferContextFromBufferId(
+      int32_t buffer_id);
 
   SEQUENCE_CHECKER(sequence_checker_);
   std::map<int32_t /*client_id*/, ClientContext> clients_;

services/video_capture/broadcasting_receiver_unittest.cc

@@ -0,0 +1,129 @@
+// Copyright 2019 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 "services/video_capture/broadcasting_receiver.h"
+
+#include "base/run_loop.h"
+#include "base/test/scoped_task_environment.h"
+#include "media/capture/video/shared_memory_handle_provider.h"
+#include "mojo/public/cpp/bindings/strong_binding.h"
+#include "services/video_capture/public/cpp/mock_receiver.h"
+#include "testing/gmock/include/gmock/gmock.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+using testing::_;
+using testing::InvokeWithoutArgs;
+
+namespace video_capture {
+
+class FakeAccessPermission : public mojom::ScopedAccessPermission {
+ public:
+  FakeAccessPermission(base::OnceClosure destruction_cb)
+      : destruction_cb_(std::move(destruction_cb)) {}
+  ~FakeAccessPermission() override { std::move(destruction_cb_).Run(); }
+
+ private:
+  base::OnceClosure destruction_cb_;
+};
+
+class BroadcastingReceiverTest : public ::testing::Test {
+ public:
+  void SetUp() override {
+    mojom::ReceiverPtr receiver_1;
+    mojom::ReceiverPtr receiver_2;
+    mock_receiver_1_ =
+        std::make_unique<MockReceiver>(mojo::MakeRequest(&receiver_1));
+    mock_receiver_2_ =
+        std::make_unique<MockReceiver>(mojo::MakeRequest(&receiver_2));
+    broadcaster_.AddClient(std::move(receiver_1),
+                           media::VideoCaptureBufferType::kSharedMemory);
+    broadcaster_.AddClient(std::move(receiver_2),
+                           media::VideoCaptureBufferType::kSharedMemory);
+  }
+
+ protected:
+  BroadcastingReceiver broadcaster_;
+  std::unique_ptr<MockReceiver> mock_receiver_1_;
+  std::unique_ptr<MockReceiver> mock_receiver_2_;
+  base::test::ScopedTaskEnvironment task_environment_;
+};
+
+TEST_F(
+    BroadcastingReceiverTest,
+    HoldsOnToAccessPermissionForRetiredBufferUntilLastClientFinishedConsuming) {
+  media::SharedMemoryHandleProvider shm_provider;
+  const size_t kArbitraryDummyBufferSize = 8u;
+  ASSERT_TRUE(shm_provider.InitForSize(kArbitraryDummyBufferSize));
+  media::mojom::VideoBufferHandlePtr buffer_handle =
+      media::mojom::VideoBufferHandle::New();
+  buffer_handle->set_shared_buffer_handle(
+      shm_provider.GetHandleForInterProcessTransit(true /*read_only*/));
+  static const int kArbiraryBufferId = 123;
+  static const int kArbiraryFrameFeedbackId = 456;
+  broadcaster_.OnNewBuffer(kArbiraryBufferId, std::move(buffer_handle));
+
+  base::RunLoop frame_arrived_at_receiver_1;
+  base::RunLoop frame_arrived_at_receiver_2;
+  EXPECT_CALL(*mock_receiver_1_, DoOnFrameReadyInBuffer(_, _, _, _))
+      .WillOnce(InvokeWithoutArgs([&frame_arrived_at_receiver_1]() {
+        frame_arrived_at_receiver_1.Quit();
+      }));
+  EXPECT_CALL(*mock_receiver_2_, DoOnFrameReadyInBuffer(_, _, _, _))
+      .WillOnce(InvokeWithoutArgs([&frame_arrived_at_receiver_2]() {
+        frame_arrived_at_receiver_2.Quit();
+      }));
+  mock_receiver_2_->HoldAccessPermissions();
+
+  mojom::ScopedAccessPermissionPtr access_permission;
+  bool access_permission_has_been_released = false;
+  mojo::MakeStrongBinding(std::make_unique<FakeAccessPermission>(base::BindOnce(
+                              [](bool* access_permission_has_been_released) {
+                                *access_permission_has_been_released = true;
+                              },
+                              &access_permission_has_been_released)),
+                          mojo::MakeRequest(&access_permission));
+  media::mojom::VideoFrameInfoPtr frame_info =
+      media::mojom::VideoFrameInfo::New();
+  media::VideoFrameMetadata frame_metadata;
+  frame_info->metadata = frame_metadata.GetInternalValues().Clone();
+  broadcaster_.OnFrameReadyInBuffer(kArbiraryBufferId, kArbiraryFrameFeedbackId,
+                                    std::move(access_permission),
+                                    std::move(frame_info));
+
+  // mock_receiver_1_ finishes consuming immediately.
+  // mock_receiver_2_ continues consuming.
+  frame_arrived_at_receiver_1.Run();
+  frame_arrived_at_receiver_2.Run();
+
+  base::RunLoop buffer_retired_arrived_at_receiver_1;
+  base::RunLoop buffer_retired_arrived_at_receiver_2;
+  EXPECT_CALL(*mock_receiver_1_, DoOnBufferRetired(_))
+      .WillOnce(InvokeWithoutArgs([&buffer_retired_arrived_at_receiver_1]() {
+        buffer_retired_arrived_at_receiver_1.Quit();
+      }));
+  EXPECT_CALL(*mock_receiver_2_, DoOnBufferRetired(_))
+      .WillOnce(InvokeWithoutArgs([&buffer_retired_arrived_at_receiver_2]() {
+        buffer_retired_arrived_at_receiver_2.Quit();
+      }));
+
+  // retire the buffer
+  broadcaster_.OnBufferRetired(kArbiraryBufferId);
+
+  // expect that both receivers get the retired event
+  buffer_retired_arrived_at_receiver_1.Run();
+  buffer_retired_arrived_at_receiver_2.Run();
+
+  // expect that |access_permission| is still being held
+  base::RunLoop().RunUntilIdle();
+  EXPECT_FALSE(access_permission_has_been_released);
+
+  // mock_receiver_2_ finishes consuming
+  mock_receiver_2_->ReleaseAccessPermissions();
+
+  // expect that |access_permission| is released
+  base::RunLoop().RunUntilIdle();
+  EXPECT_TRUE(access_permission_has_been_released);
+}
+
+}  // namespace video_capture

services/video_capture/public/cpp/mock_receiver.cc

@@ -9,13 +9,24 @@
 
 namespace video_capture {
 
-MockReceiver::MockReceiver() : binding_(this) {}
+MockReceiver::MockReceiver()
+    : binding_(this), should_store_access_permissions_(false) {}
 
 MockReceiver::MockReceiver(mojom::ReceiverRequest request)
-    : binding_(this, std::move(request)) {}
+    : binding_(this, std::move(request)),
+      should_store_access_permissions_(false) {}
 
 MockReceiver::~MockReceiver() = default;
 
+void MockReceiver::HoldAccessPermissions() {
+  should_store_access_permissions_ = true;
+}
+
+void MockReceiver::ReleaseAccessPermissions() {
+  should_store_access_permissions_ = false;
+  access_permissions_.clear();
+}
+
 void MockReceiver::OnNewBuffer(
     int32_t buffer_id,
     media::mojom::VideoBufferHandlePtr buffer_handle) {
@@ -31,6 +42,8 @@ void MockReceiver::OnFrameReadyInBuffer(
     media::mojom::VideoFrameInfoPtr frame_info) {
   DoOnFrameReadyInBuffer(buffer_id, frame_feedback_id, &access_permission,
                          &frame_info);
+  if (should_store_access_permissions_)
+    access_permissions_.emplace_back(std::move(access_permission));
 }
 
 void MockReceiver::OnBufferRetired(int32_t buffer_id) {

services/video_capture/public/cpp/mock_receiver.h

@@ -22,6 +22,9 @@ class MockReceiver : public mojom::Receiver {
   explicit MockReceiver(mojom::ReceiverRequest request);
   ~MockReceiver() override;
 
+  void HoldAccessPermissions();
+  void ReleaseAccessPermissions();
+
   // Use forwarding method to work around gmock not supporting move-only types.
   void OnNewBuffer(int32_t buffer_id,
                    media::mojom::VideoBufferHandlePtr buffer_handle) override;
@@ -50,6 +53,8 @@ class MockReceiver : public mojom::Receiver {
  private:
   const mojo::Binding<mojom::Receiver> binding_;
   std::vector<int32_t> known_buffer_ids_;
+  bool should_store_access_permissions_;
+  std::vector<mojom::ScopedAccessPermissionPtr> access_permissions_;
 };
 
 }  // namespace video_capture