Clarify lifetime relationship between GLES2Decoder and CommandBufferS…

…ervice

Currently the lifetime of both classes is confusing, because the
CommandBufferService needs the GLES2Decoder to be constructed, but it is
supposed to outlive the GLES2Decoder.

This CL clarifies the relationship: since the decoder is only used in
CommandBufferService::Flush, pass it explicitly there, and that allows making
the GLES2Decoder lifetime to be a strict subset of the CommandBufferService,
without invalid temporary states.

Bug: 723770
Cq-Include-Trybots: master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
Change-Id: I24c20da70510c2fbab73e69d4dc2a8a1a157366c
Reviewed-on: https://chromium-review.googlesource.com/523036
Commit-Queue: Antoine Labour <piman@chromium.org>
Reviewed-by: Dale Curtis <dalecurtis@chromium.org>
Reviewed-by: Zhenyao Mo <zmo@chromium.org>
Cr-Commit-Position: refs/heads/master@{#477484}

gpu/BUILD.gn

@@ -105,6 +105,8 @@ test("command_buffer_gles2_tests") {
 static_library("test_support") {
   testonly = true
   sources = [
+    "command_buffer/client/client_test_helper.cc",
+    "command_buffer/client/client_test_helper.h",
     "command_buffer/client/gles2_interface_stub.cc",
     "command_buffer/client/gles2_interface_stub.h",
     "command_buffer/service/error_state_mock.cc",
@@ -121,6 +123,7 @@ static_library("test_support") {
     "//testing/gmock",
     "//testing/gtest",
     "//ui/gl:gl_unittest_utils",
+    "//ui/latency:test_support",
   ]
 }
 
@@ -219,8 +222,6 @@ test("gpu_unittests") {
   sources = [
     "command_buffer/client/buffer_tracker_unittest.cc",
     "command_buffer/client/client_discardable_manager_unittest.cc",
-    "command_buffer/client/client_test_helper.cc",
-    "command_buffer/client/client_test_helper.h",
     "command_buffer/client/cmd_buffer_helper_test.cc",
     "command_buffer/client/fenced_allocator_test.cc",
     "command_buffer/client/gles2_implementation_unittest.cc",

gpu/command_buffer/client/client_test_helper.cc

@@ -35,7 +35,7 @@ void FakeCommandBufferServiceBase::SetReleaseCount(uint64_t release_count) {
 // Get's the Id of the next transfer buffer that will be returned
 // by CreateTransferBuffer. This is useful for testing expected ids.
 int32_t FakeCommandBufferServiceBase::GetNextFreeTransferBufferId() {
-  for (size_t ii = 0; ii < arraysize(transfer_buffer_buffers_); ++ii) {
+  for (int32_t ii = 0; ii < kMaxTransferBuffers; ++ii) {
     if (!transfer_buffer_buffers_[ii].get()) {
       return kTransferBufferBaseId + ii;
     }

gpu/command_buffer/client/cmd_buffer_helper_test.cc

@@ -42,9 +42,8 @@ const int32_t kUnusedCommandId = 5;  // we use 0 and 2 currently.
 class CommandBufferDirectLocked : public CommandBufferDirect {
  public:
   explicit CommandBufferDirectLocked(
-      TransferBufferManager* transfer_buffer_manager,
-      AsyncAPIInterface* handler)
-      : CommandBufferDirect(transfer_buffer_manager, handler),
+      TransferBufferManager* transfer_buffer_manager)
+      : CommandBufferDirect(transfer_buffer_manager),
         flush_locked_(false),
         last_flush_(-1),
         previous_put_offset_(0),
@@ -98,19 +97,17 @@ class CommandBufferDirectLocked : public CommandBufferDirect {
 class CommandBufferHelperTest : public testing::Test {
  protected:
   virtual void SetUp() {
-    api_mock_.reset(new AsyncAPIMock(true));
+    transfer_buffer_manager_ = base::MakeUnique<TransferBufferManager>(nullptr);
+    command_buffer_.reset(
+        new CommandBufferDirectLocked(transfer_buffer_manager_.get()));
+    api_mock_.reset(new AsyncAPIMock(true, command_buffer_->service()));
+    command_buffer_->set_handler(api_mock_.get());
 
     // ignore noops in the mock - we don't want to inspect the internals of the
     // helper.
     EXPECT_CALL(*api_mock_, DoCommand(cmd::kNoop, _, _))
         .WillRepeatedly(Return(error::kNoError));
 
-    transfer_buffer_manager_ = base::MakeUnique<TransferBufferManager>(nullptr);
-    command_buffer_.reset(new CommandBufferDirectLocked(
-        transfer_buffer_manager_.get(), api_mock_.get()));
-
-    api_mock_->set_command_buffer_service(command_buffer_->service());
-
     helper_.reset(new CommandBufferHelper(command_buffer_.get()));
     helper_->Initialize(kCommandBufferSizeBytes);
 
@@ -253,9 +250,9 @@ class CommandBufferHelperTest : public testing::Test {
 
   CommandBufferOffset get_helper_put() { return helper_->put_; }
 
-  std::unique_ptr<AsyncAPIMock> api_mock_;
   std::unique_ptr<TransferBufferManager> transfer_buffer_manager_;
   std::unique_ptr<CommandBufferDirectLocked> command_buffer_;
+  std::unique_ptr<AsyncAPIMock> api_mock_;
   std::unique_ptr<CommandBufferHelper> helper_;
   std::vector<std::unique_ptr<CommandBufferEntry[]>> test_command_args_;
   unsigned int test_command_next_id_;

gpu/command_buffer/client/fenced_allocator_test.cc

@@ -38,7 +38,12 @@ class BaseFencedAllocatorTest : public testing::Test {
   static const int kAllocAlignment = 16;
 
   void SetUp() override {
-    api_mock_.reset(new AsyncAPIMock(true));
+    transfer_buffer_manager_ = base::MakeUnique<TransferBufferManager>(nullptr);
+    command_buffer_.reset(
+        new CommandBufferDirect(transfer_buffer_manager_.get()));
+    api_mock_.reset(new AsyncAPIMock(true, command_buffer_->service()));
+    command_buffer_->set_handler(api_mock_.get());
+
     // ignore noops in the mock - we don't want to inspect the internals of the
     // helper.
     EXPECT_CALL(*api_mock_, DoCommand(cmd::kNoop, 0, _))
@@ -48,21 +53,15 @@ class BaseFencedAllocatorTest : public testing::Test {
         .WillRepeatedly(DoAll(Invoke(api_mock_.get(), &AsyncAPIMock::SetToken),
                               Return(error::kNoError)));
 
-    transfer_buffer_manager_ = base::MakeUnique<TransferBufferManager>(nullptr);
-    command_buffer_.reset(new CommandBufferDirect(
-        transfer_buffer_manager_.get(), api_mock_.get()));
-
-    api_mock_->set_command_buffer_service(command_buffer_->service());
-
     helper_.reset(new CommandBufferHelper(command_buffer_.get()));
     helper_->Initialize(kBufferSize);
   }
 
   int32_t GetToken() { return command_buffer_->GetLastState().token; }
 
-  std::unique_ptr<AsyncAPIMock> api_mock_;
   std::unique_ptr<TransferBufferManager> transfer_buffer_manager_;
   std::unique_ptr<CommandBufferDirect> command_buffer_;
+  std::unique_ptr<AsyncAPIMock> api_mock_;
   std::unique_ptr<CommandBufferHelper> helper_;
   base::MessageLoop message_loop_;
 };

gpu/command_buffer/client/mapped_memory_unittest.cc

@@ -35,7 +35,12 @@ class MappedMemoryTestBase : public testing::Test {
   static const unsigned int kBufferSize = 1024;
 
   void SetUp() override {
-    api_mock_.reset(new AsyncAPIMock(true));
+    transfer_buffer_manager_ = base::MakeUnique<TransferBufferManager>(nullptr);
+    command_buffer_.reset(
+        new CommandBufferDirect(transfer_buffer_manager_.get()));
+    api_mock_.reset(new AsyncAPIMock(true, command_buffer_->service()));
+    command_buffer_->set_handler(api_mock_.get());
+
     // ignore noops in the mock - we don't want to inspect the internals of the
     // helper.
     EXPECT_CALL(*api_mock_, DoCommand(cmd::kNoop, 0, _))
@@ -45,21 +50,15 @@ class MappedMemoryTestBase : public testing::Test {
         .WillRepeatedly(DoAll(Invoke(api_mock_.get(), &AsyncAPIMock::SetToken),
                               Return(error::kNoError)));
 
-    transfer_buffer_manager_ = base::MakeUnique<TransferBufferManager>(nullptr);
-    command_buffer_.reset(new CommandBufferDirect(
-        transfer_buffer_manager_.get(), api_mock_.get()));
-
-    api_mock_->set_command_buffer_service(command_buffer_->service());
-
     helper_.reset(new CommandBufferHelper(command_buffer_.get()));
     helper_->Initialize(kBufferSize);
   }
 
   int32_t GetToken() { return command_buffer_->GetLastState().token; }
 
-  std::unique_ptr<AsyncAPIMock> api_mock_;
   std::unique_ptr<TransferBufferManager> transfer_buffer_manager_;
   std::unique_ptr<CommandBufferDirect> command_buffer_;
+  std::unique_ptr<AsyncAPIMock> api_mock_;
   std::unique_ptr<CommandBufferHelper> helper_;
   base::MessageLoop message_loop_;
 };

gpu/command_buffer/client/ring_buffer_test.cc

@@ -60,7 +60,12 @@ class BaseRingBufferTest : public testing::Test {
 
   void SetUp() override {
     delay_set_token_ = false;
-    api_mock_.reset(new AsyncAPIMock(true));
+    transfer_buffer_manager_ = base::MakeUnique<TransferBufferManager>(nullptr);
+    command_buffer_.reset(
+        new CommandBufferDirect(transfer_buffer_manager_.get()));
+    api_mock_.reset(new AsyncAPIMock(true, command_buffer_->service()));
+    command_buffer_->set_handler(api_mock_.get());
+
     // ignore noops in the mock - we don't want to inspect the internals of the
     // helper.
     EXPECT_CALL(*api_mock_, DoCommand(cmd::kNoop, 0, _))
@@ -70,21 +75,15 @@ class BaseRingBufferTest : public testing::Test {
         .WillRepeatedly(DoAll(Invoke(this, &BaseRingBufferTest::SetToken),
                               Return(error::kNoError)));
 
-    transfer_buffer_manager_ = base::MakeUnique<TransferBufferManager>(nullptr);
-    command_buffer_.reset(new CommandBufferDirect(
-        transfer_buffer_manager_.get(), api_mock_.get()));
-
-    api_mock_->set_command_buffer_service(command_buffer_->service());
-
     helper_.reset(new CommandBufferHelper(command_buffer_.get()));
     helper_->Initialize(kBufferSize);
   }
 
   int32_t GetToken() { return command_buffer_->GetLastState().token; }
 
-  std::unique_ptr<AsyncAPIMock> api_mock_;
   std::unique_ptr<TransferBufferManager> transfer_buffer_manager_;
   std::unique_ptr<CommandBufferDirect> command_buffer_;
+  std::unique_ptr<AsyncAPIMock> api_mock_;
   std::unique_ptr<CommandBufferHelper> helper_;
   std::vector<const volatile void*> set_token_arguments_;
   bool delay_set_token_;

gpu/command_buffer/service/command_buffer_direct.cc

@@ -13,27 +13,17 @@ namespace gpu {
 namespace {
 
 uint64_t g_next_command_buffer_id = 1;
-bool AlwaysTrue() {
-  return true;
-}
 
 }  // anonymous namespace
 
 CommandBufferDirect::CommandBufferDirect(
-    TransferBufferManager* transfer_buffer_manager,
-    AsyncAPIInterface* handler)
-    : CommandBufferDirect(transfer_buffer_manager,
-                          handler,
-                          base::Bind(&AlwaysTrue),
-                          nullptr) {}
+    TransferBufferManager* transfer_buffer_manager)
+    : CommandBufferDirect(transfer_buffer_manager, nullptr) {}
 
 CommandBufferDirect::CommandBufferDirect(
     TransferBufferManager* transfer_buffer_manager,
-    AsyncAPIInterface* handler,
-    const MakeCurrentCallback& callback,
     SyncPointManager* sync_point_manager)
-    : service_(this, transfer_buffer_manager, handler),
-      make_current_callback_(callback),
+    : service_(this, transfer_buffer_manager),
       sync_point_manager_(sync_point_manager),
       command_buffer_id_(
           CommandBufferId::FromUnsafeValue(g_next_command_buffer_id++)) {
@@ -84,10 +74,7 @@ CommandBuffer::State CommandBufferDirect::WaitForGetOffsetInRange(
 }
 
 void CommandBufferDirect::Flush(int32_t put_offset) {
-  if (!make_current_callback_.Run()) {
-    service_.SetParseError(error::kLostContext);
-    return;
-  }
+  DCHECK(handler_);
   uint32_t order_num = 0;
   if (sync_point_manager_) {
     // If sync point manager is supported, assign order numbers to commands.
@@ -109,7 +96,7 @@ void CommandBufferDirect::Flush(int32_t put_offset) {
     return;
   }
 
-  service_.Flush(put_offset);
+  service_.Flush(put_offset, handler_);
   if (sync_point_manager_) {
     // Finish processing order number here.
     sync_point_order_data_->FinishProcessingOrderNumber(order_num);

gpu/command_buffer/service/command_buffer_direct.h

@@ -26,14 +26,12 @@ class GPU_EXPORT CommandBufferDirect : public CommandBuffer,
   using MakeCurrentCallback = base::Callback<bool()>;
 
   CommandBufferDirect(TransferBufferManager* transfer_buffer_manager,
-                      AsyncAPIInterface* handler,
-                      const MakeCurrentCallback& callback,
                       SyncPointManager* sync_point_manager);
-  CommandBufferDirect(TransferBufferManager* transfer_buffer_manager,
-                      AsyncAPIInterface* handler);
+  explicit CommandBufferDirect(TransferBufferManager* transfer_buffer_manager);
 
   ~CommandBufferDirect() override;
 
+  void set_handler(AsyncAPIInterface* handler) { handler_ = handler; }
   CommandBufferServiceBase* service() { return &service_; }
 
   // CommandBuffer implementation:
@@ -65,9 +63,9 @@ class GPU_EXPORT CommandBufferDirect : public CommandBuffer,
 
  private:
   CommandBufferService service_;
-  MakeCurrentCallback make_current_callback_;
   SyncPointManager* sync_point_manager_;
 
+  AsyncAPIInterface* handler_ = nullptr;
   scoped_refptr<SyncPointOrderData> sync_point_order_data_;
   scoped_refptr<SyncPointClientState> sync_point_client_state_;
   bool pause_commands_ = false;

gpu/command_buffer/service/command_buffer_service.cc

@@ -41,14 +41,10 @@ class MemoryBufferBacking : public BufferBacking {
 
 CommandBufferService::CommandBufferService(
     CommandBufferServiceClient* client,
-    TransferBufferManager* transfer_buffer_manager,
-    AsyncAPIInterface* handler)
-    : client_(client),
-      transfer_buffer_manager_(transfer_buffer_manager),
-      handler_(handler) {
+    TransferBufferManager* transfer_buffer_manager)
+    : client_(client), transfer_buffer_manager_(transfer_buffer_manager) {
   DCHECK(client_);
   DCHECK(transfer_buffer_manager_);
-  DCHECK(handler_);
   state_.token = 0;
 }
 
@@ -60,14 +56,16 @@ void CommandBufferService::UpdateState() {
     shared_state_->Write(state_);
 }
 
-void CommandBufferService::Flush(int32_t put_offset) {
+void CommandBufferService::Flush(int32_t put_offset,
+                                 AsyncAPIInterface* handler) {
+  DCHECK(handler);
   if (put_offset < 0 || put_offset >= num_entries_) {
     SetParseError(gpu::error::kOutOfBounds);
     return;
   }
 
   TRACE_EVENT1("gpu", "CommandBufferService:PutChanged", "handler",
-               handler_->GetLogPrefix().as_string());
+               handler->GetLogPrefix().as_string());
 
   put_offset_ = put_offset;
 
@@ -83,10 +81,21 @@ void CommandBufferService::Flush(int32_t put_offset) {
     TRACE_COUNTER_ID1("gpu", "CommandBufferService::Paused", this, paused_);
   }
 
-  error::Error error = error::kNoError;
-  handler_->BeginDecoding();
+  handler->BeginDecoding();
+  int end = put_offset_ < state_.get_offset ? num_entries_ : put_offset_;
   while (put_offset_ != state_.get_offset) {
-    error = ProcessCommands(kParseCommandsSlice);
+    int num_entries = end - state_.get_offset;
+    int entries_processed = 0;
+    error::Error error =
+        handler->DoCommands(kParseCommandsSlice, buffer_ + state_.get_offset,
+                            num_entries, &entries_processed);
+
+    state_.get_offset += entries_processed;
+    DCHECK_LE(state_.get_offset, num_entries_);
+    if (state_.get_offset == num_entries_) {
+      end = put_offset_;
+      state_.get_offset = 0;
+    }
 
     if (error::IsError(error)) {
       SetParseError(error);
@@ -104,7 +113,7 @@ void CommandBufferService::Flush(int32_t put_offset) {
       break;
   }
 
-  handler_->EndDecoding();
+  handler->EndDecoding();
 }
 
 void CommandBufferService::SetGetBuffer(int32_t transfer_buffer_id) {
@@ -225,21 +234,4 @@ void CommandBufferService::SetScheduled(bool scheduled) {
   scheduled_ = scheduled;
 }
 
-error::Error CommandBufferService::ProcessCommands(int num_commands) {
-  int num_entries = put_offset_ < state_.get_offset
-                        ? num_entries_ - state_.get_offset
-                        : put_offset_ - state_.get_offset;
-
-  int entries_processed = 0;
-  error::Error result =
-      handler_->DoCommands(num_commands, buffer_ + state_.get_offset,
-                           num_entries, &entries_processed);
-
-  state_.get_offset += entries_processed;
-  if (state_.get_offset == num_entries_)
-    state_.get_offset = 0;
-
-  return result;
-}
-
 }  // namespace gpu

gpu/command_buffer/service/command_buffer_service.h

@@ -73,8 +73,7 @@ class GPU_EXPORT CommandBufferService : public CommandBufferServiceBase {
   static const int kParseCommandsSlice = 20;
 
   CommandBufferService(CommandBufferServiceClient* client,
-                       TransferBufferManager* transfer_buffer_manager,
-                       AsyncAPIInterface* handler);
+                       TransferBufferManager* transfer_buffer_manager);
   ~CommandBufferService() override;
 
   // CommandBufferServiceBase implementation:
@@ -92,9 +91,8 @@ class GPU_EXPORT CommandBufferService : public CommandBufferServiceBase {
   // state transfer buffer.
   void UpdateState();
 
-  // Flushes up to the put_offset and calls the PutOffsetChangeCallback to
-  // execute commands.
-  void Flush(int32_t put_offset);
+  // Flushes up to the put_offset and execute commands with the handler.
+  void Flush(int32_t put_offset, AsyncAPIInterface* handler);
 
   // Sets the get buffer and call the GetBufferChangeCallback.
   void SetGetBuffer(int32_t transfer_buffer_id);
@@ -123,11 +121,8 @@ class GPU_EXPORT CommandBufferService : public CommandBufferServiceBase {
   int32_t put_offset() const { return put_offset_; }
 
  private:
-  error::Error ProcessCommands(int num_commands);
-
   CommandBufferServiceClient* client_;
   TransferBufferManager* transfer_buffer_manager_;
-  AsyncAPIInterface* handler_;
 
   CommandBuffer::State state_;
   int32_t put_offset_ = 0;