MediaLog: Replace task posting in AddEvent with lock

content/renderer/media/render_media_log.cc

 // Copyright (c) 2012 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 "content/renderer/media/render_media_log.h"
 
 #include "base/bind.h"
 #include "base/location.h"
 #include "base/logging.h"
 #include "base/single_thread_task_runner.h"
@@ skipping 17 matching lines @@
   }
 }
 
 }  // namespace
 
 namespace content {
 
 RenderMediaLog::RenderMediaLog()
     : task_runner_(base::ThreadTaskRunnerHandle::Get()),
       tick_clock_(new base::DefaultTickClock()),
-      last_ipc_send_time_(tick_clock_->NowTicks()) {
+      last_ipc_send_time_(tick_clock_->NowTicks()),
+      ipc_send_pending_(false) {
   DCHECK(RenderThread::Get())
       << "RenderMediaLog must be constructed on the render thread";
 }
 
 void RenderMediaLog::AddEvent(scoped_ptr<media::MediaLogEvent> event) {
-  // Always post to preserve the correct order of events.
-  // TODO(xhwang): Consider using sorted containers to keep the order and
-  // avoid extra posting.
-  task_runner_->PostTask(FROM_HERE,
-                         base::Bind(&RenderMediaLog::AddEventInternal, this,
-                                    base::Passed(&event)));
-}
+  {
+    base::AutoLock auto_lock(lock_);
 
-void RenderMediaLog::AddEventInternal(scoped_ptr<media::MediaLogEvent> event) {
-  DCHECK(task_runner_->BelongsToCurrentThread());
+    Log(event.get());

[xhwang, 2016/04/01 05:42:55]

nit: This can be outside of the lock.

[wolenetz, 2016/04/04 18:30:32]

Done.

 
-  Log(event.get());
+    // Keep track of the latest buffered extents properties to avoid sending
+    // thousands of events over IPC. See http://crbug.com/352585 for details.
+    if (event->type == media::MediaLogEvent::BUFFERED_EXTENTS_CHANGED)
+      last_buffered_extents_changed_event_.swap(event);
+    else
+      queued_media_events_.push_back(*event);
 
-  // If there is an event waiting to be sent, there must be a send task pending.
-  const bool delayed_ipc_send_pending =
-      !queued_media_events_.empty() || last_buffered_extents_changed_event_;
+    if (ipc_send_pending_)
+      return;
 
-  // Keep track of the latest buffered extents properties to avoid sending
-  // thousands of events over IPC. See http://crbug.com/352585 for details.
-  if (event->type == media::MediaLogEvent::BUFFERED_EXTENTS_CHANGED)
-    last_buffered_extents_changed_event_.swap(event);
-  else
-    queued_media_events_.push_back(*event);
+    ipc_send_pending_ = true;
 
-  if (delayed_ipc_send_pending)
-    return;
-
-  // Delay until it's been a second since the last ipc message was sent.
-  base::TimeDelta delay_for_next_ipc_send =
-      base::TimeDelta::FromSeconds(1) -
-      (tick_clock_->NowTicks() - last_ipc_send_time_);
-  if (delay_for_next_ipc_send > base::TimeDelta()) {
-    task_runner_->PostDelayedTask(
-        FROM_HERE, base::Bind(&RenderMediaLog::SendQueuedMediaEvents, this),
-        delay_for_next_ipc_send);
-    return;
+    // Delay until it's been a second since the last ipc message was sent.
+    base::TimeDelta delay_for_next_ipc_send =
+        base::TimeDelta::FromSeconds(1) -
+        (tick_clock_->NowTicks() - last_ipc_send_time_);
+    if (delay_for_next_ipc_send > base::TimeDelta()) {
+      task_runner_->PostDelayedTask(
+          FROM_HERE, base::Bind(&RenderMediaLog::SendQueuedMediaEvents, this),
+          delay_for_next_ipc_send);
+      return;
+    }
   }
 
-  // It's been more than a second so send now.
-  SendQueuedMediaEvents();
+  // It's been more than a second so send ASAP.
+  if (task_runner_->BelongsToCurrentThread())
+    SendQueuedMediaEvents();
+  else
+    task_runner_->PostTask(
+        FROM_HERE, base::Bind(&RenderMediaLog::SendQueuedMediaEvents, this));
 }
 
 void RenderMediaLog::SendQueuedMediaEvents() {
   DCHECK(task_runner_->BelongsToCurrentThread());
+  base::AutoLock auto_lock(lock_);
+
+  DCHECK(ipc_send_pending_);
+  ipc_send_pending_ = false;
 
   if (last_buffered_extents_changed_event_) {
     queued_media_events_.push_back(*last_buffered_extents_changed_event_);
     last_buffered_extents_changed_event_.reset();
   }
 
   RenderThread::Get()->Send(
       new ViewHostMsg_MediaLogEvents(queued_media_events_));
   queued_media_events_.clear();
   last_ipc_send_time_ = tick_clock_->NowTicks();

[xhwang, 2016/04/01 05:42:55]

nit: Send involves serialization and could be slow (depending how many messages
we have). Can we use vector::swap to swap the queued_media_events_ to a local
one, update the last_ipc_send_time_, and then release the lock. Then we can
Send() the local one outside the lock.

[wolenetz, 2016/04/04 18:30:32]

Good idea. Note that if Send() becomes severely slow, multiple Sends() of the
events can be in flight at the same time. That shouldn't violate expectations
though, since we'd want to catch up on sending events ASAP if we're behind so
much.
Done.

 }
 
 RenderMediaLog::~RenderMediaLog() {
 }
 
 void RenderMediaLog::SetTickClockForTesting(
     scoped_ptr<base::TickClock> tick_clock) {
+  base::AutoLock auto_lock(lock_);
   tick_clock_.swap(tick_clock);
   last_ipc_send_time_ = tick_clock_->NowTicks();
 }
 
 void RenderMediaLog::SetTaskRunnerForTesting(
     const scoped_refptr<base::SingleThreadTaskRunner>& task_runner) {
+  base::AutoLock auto_lock(lock_);

[xhwang, 2016/04/01 05:42:55]

It seems odd the protect the task runner especially since we are not always
protecting the use of task_runner under a lock.

[wolenetz, 2016/04/04 18:30:32]

Yeah. This does seem odd. I'll not lock in this method. Done.

   task_runner_ = task_runner;
 }
 
 }  // namespace content