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)));
-}
-
-void RenderMediaLog::AddEventInternal(scoped_ptr<media::MediaLogEvent> event) {
-  DCHECK(task_runner_->BelongsToCurrentThread());
-
   Log(event.get());
 
-  // 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_;
+  {
+    base::AutoLock auto_lock(lock_);
 
-  // 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);
+    // 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 (delayed_ipc_send_pending)
-    return;
+    if (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;
+    ipc_send_pending_ = true;
+
+    // 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(

[watk, 2016/04/04 20:32:21]

Looks like this doesn't necessarily have to be done while holding the lock. I
don't think it's a big deal to leave it though.

[wolenetz, 2016/04/04 20:47:55]

Moved outside the lock.

+          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));

[watk, 2016/04/04 20:32:21]

braces for multi-line statement

[wolenetz, 2016/04/04 20:47:55]

Good catch. git cl format split the line but didn't add {}.

 }
 
 void RenderMediaLog::SendQueuedMediaEvents() {
   DCHECK(task_runner_->BelongsToCurrentThread());
 
-  if (last_buffered_extents_changed_event_) {
-    queued_media_events_.push_back(*last_buffered_extents_changed_event_);
-    last_buffered_extents_changed_event_.reset();
+  std::vector<media::MediaLogEvent> events_to_send;
+  {
+    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();
+    }
+
+    queued_media_events_.swap(events_to_send);
+    last_ipc_send_time_ = tick_clock_->NowTicks();
   }
 
-  RenderThread::Get()->Send(
-      new ViewHostMsg_MediaLogEvents(queued_media_events_));
-  queued_media_events_.clear();
-  last_ipc_send_time_ = tick_clock_->NowTicks();
+  RenderThread::Get()->Send(new ViewHostMsg_MediaLogEvents(events_to_send));
 }
 
 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) {
   task_runner_ = task_runner;
 }
 
 }  // namespace content