From 97528dbe66cb1f1d215e49e5379bf411898bea26 Mon Sep 17 00:00:00 2001 From: Christian Fremerey Date: Wed, 13 Mar 2019 15:53:41 +0000 Subject: [PATCH] [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 Reviewed-by: Emircan Uysaler Commit-Queue: Christian Fremerey 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 Cr-Commit-Position: refs/branch-heads/3729@{#85} Cr-Branched-From: d4a8972e30b604f090aeda5dfff68386ae656267-refs/heads/master@{#638880} --- services/video_capture/BUILD.gn | 1 + .../video_capture/broadcasting_receiver.cc | 74 ++++++---- .../video_capture/broadcasting_receiver.h | 10 +- .../broadcasting_receiver_unittest.cc | 129 ++++++++++++++++++ .../video_capture/public/cpp/mock_receiver.cc | 17 ++- .../video_capture/public/cpp/mock_receiver.h | 5 + 6 files changed, 206 insertions(+), 30 deletions(-) create mode 100644 services/video_capture/broadcasting_receiver_unittest.cc diff --git a/services/video_capture/BUILD.gn b/services/video_capture/BUILD.gn index 60a49487a9cdb..0d429eefd5d76 100644 --- a/services/video_capture/BUILD.gn +++ b/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", diff --git a/services/video_capture/broadcasting_receiver.cc b/services/video_capture/broadcasting_receiver.cc index b6a7006b0816f..78fadc17d9d91 100644 --- a/services/video_capture/broadcasting_receiver.cc +++ b/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,10 +296,10 @@ void BroadcastingReceiver::OnFrameReadyInBuffer( mojo::MakeStrongBinding( std::make_unique(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(); } @@ -297,16 +307,18 @@ void BroadcastingReceiver::OnFrameReadyInBuffer( 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,10 +379,20 @@ 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) { @@ -378,15 +400,15 @@ void BroadcastingReceiver::OnClientDisconnected(int32_t client_id) { clients_.erase(client_id); } -BroadcastingReceiver::BufferContext& -BroadcastingReceiver::LookupBufferContextFromBufferId(int32_t buffer_id) { +std::vector::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 diff --git a/services/video_capture/broadcasting_receiver.h b/services/video_capture/broadcasting_receiver.h index faa5e8fd118f0..70849b4ad3fe6 100644 --- a/services/video_capture/broadcasting_receiver.h +++ b/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::iterator FindUnretiredBufferContextFromBufferId( + int32_t buffer_id); SEQUENCE_CHECKER(sequence_checker_); std::map clients_; diff --git a/services/video_capture/broadcasting_receiver_unittest.cc b/services/video_capture/broadcasting_receiver_unittest.cc new file mode 100644 index 0000000000000..452f805961943 --- /dev/null +++ b/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(mojo::MakeRequest(&receiver_1)); + mock_receiver_2_ = + std::make_unique(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 mock_receiver_1_; + std::unique_ptr 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(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 diff --git a/services/video_capture/public/cpp/mock_receiver.cc b/services/video_capture/public/cpp/mock_receiver.cc index 788ee07de6212..754ab4587d598 100644 --- a/services/video_capture/public/cpp/mock_receiver.cc +++ b/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) { diff --git a/services/video_capture/public/cpp/mock_receiver.h b/services/video_capture/public/cpp/mock_receiver.h index 6493b216ddc39..4212a749dce71 100644 --- a/services/video_capture/public/cpp/mock_receiver.h +++ b/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 binding_; std::vector known_buffer_ids_; + bool should_store_access_permissions_; + std::vector access_permissions_; }; } // namespace video_capture