Ensure that data is not overwritten in the audio input shared memory ring buffer that sits on the b…

content/browser/renderer_host/media/audio_input_sync_writer.cc

Index: content/browser/renderer_host/media/audio_input_sync_writer.cc
diff --git a/content/browser/renderer_host/media/audio_input_sync_writer.cc b/content/browser/renderer_host/media/audio_input_sync_writer.cc
index 600881af9a93799011f993ebaae27a766a2414c0..dfe23c1ccae4b3484bb92b4f80244df236e89183 100644
--- a/content/browser/renderer_host/media/audio_input_sync_writer.cc
+++ b/content/browser/renderer_host/media/audio_input_sync_writer.cc
@@ -6,33 +6,50 @@
 
 #include <algorithm>
 
-#include "base/memory/shared_memory.h"
+#include "base/metrics/histogram.h"
+#include "base/strings/stringprintf.h"
 #include "content/browser/renderer_host/media/media_stream_manager.h"
 
 using media::AudioBus;
 
 namespace content {
 
-AudioInputSyncWriter::AudioInputSyncWriter(base::SharedMemory* shared_memory,
+namespace {
+
+// Used to log if any audio glitches have been detected during an audio session.
+// Elements in this enum should not be added, deleted or rearranged.
+enum AudioGlitchResult {
+  AUDIO_CAPTURER_NO_AUDIO_GLITCHES = 0,
+  AUDIO_CAPTURER_AUDIO_GLITCHES = 1,
+  AUDIO_CAPTURER_AUDIO_GLITCHES_MAX = AUDIO_CAPTURER_AUDIO_GLITCHES
+};
+
+}  // namespace
+
+AudioInputSyncWriter::AudioInputSyncWriter(void* shared_memory,
+                                           size_t shared_memory_size,
                                            int shared_memory_segment_count,
                                            const media::AudioParameters& params)
-    : shared_memory_(shared_memory),
+    : shared_memory_(static_cast<uint8*>(shared_memory)),
       shared_memory_segment_count_(shared_memory_segment_count),
       current_segment_id_(0),
       creation_time_(base::Time::Now()),
       audio_bus_memory_size_(AudioBus::CalculateMemorySize(params)),
-      next_buffer_id_(0) {
+      next_buffer_id_(0),
+      next_read_buffer_index_(0),
+      number_of_filled_segments_(0),
+      write_count_(0),
+      write_error_count_(0) {
   DCHECK_GT(shared_memory_segment_count, 0);
-  DCHECK_EQ(shared_memory->requested_size() % shared_memory_segment_count, 0u);
+  DCHECK_EQ(shared_memory_size % shared_memory_segment_count, 0u);
   shared_memory_segment_size_ =
-      shared_memory->requested_size() / shared_memory_segment_count;
-  DVLOG(1) << "SharedMemory::requested_size: "
-           << shared_memory->requested_size();
+      shared_memory_size / shared_memory_segment_count;
+  DVLOG(1) << "shared_memory_size: " << shared_memory_size;
   DVLOG(1) << "shared_memory_segment_count: " << shared_memory_segment_count;
   DVLOG(1) << "audio_bus_memory_size: " << audio_bus_memory_size_;
 
   // Create vector of audio buses by wrapping existing blocks of memory.
-  uint8* ptr = static_cast<uint8*>(shared_memory_->memory());
+  uint8* ptr = shared_memory_;
   for (int i = 0; i < shared_memory_segment_count; ++i) {
     CHECK((reinterpret_cast<uintptr_t>(ptr) &
           (media::AudioBus::kChannelAlignment - 1)) == 0U);
@@ -45,7 +62,27 @@ AudioInputSyncWriter::AudioInputSyncWriter(base::SharedMemory* shared_memory,
   }
 }
 
-AudioInputSyncWriter::~AudioInputSyncWriter() {}
+AudioInputSyncWriter::~AudioInputSyncWriter() {
+  if (write_count_ == 0)
+    return;
+
+  UMA_HISTOGRAM_PERCENTAGE(
+      "Media.AudioCapturerMissedReadDeadline",
+      100.0 * write_error_count_ / write_count_);
+
+  UMA_HISTOGRAM_ENUMERATION("Media.AudioCapturerAudioGlitches",
+                            write_error_count_ == 0 ?
+                                AUDIO_CAPTURER_NO_AUDIO_GLITCHES :
+                                AUDIO_CAPTURER_AUDIO_GLITCHES,
+                            AUDIO_CAPTURER_AUDIO_GLITCHES_MAX + 1);
+
+  std::string log_string = base::StringPrintf(
+      "AISW: number of detected audio glitches: %lu out of %lu",
+      write_error_count_,
+      write_count_);
+  MediaStreamManager::SendMessageToNativeLog(log_string);
+  DVLOG(1) << log_string;
+}
 
 void AudioInputSyncWriter::Write(const media::AudioBus* data,
                                  double volume,
@@ -70,15 +107,54 @@ void AudioInputSyncWriter::Write(const media::AudioBus* data,
     }
   }
   if (!oss.str().empty()) {
-    MediaStreamManager::SendMessageToNativeLog(oss.str());
+    AddToNativeLog(oss.str());
     DVLOG(1) << oss.str();
   }
 
   last_write_time_ = base::Time::Now();
 #endif
 
+  // Check that the renderer side has read data so that we don't overwrite.
+  // The renderer side sends a signal over the socket each time it has read
+  // data. Here, we read those verifications before writing. We verify that each
+  // buffer index is in sequence.
+  ++write_count_;

[tommi (sloooow) - chröme, 2015/09/03 14:55:13]

here we increment write_count_ without having actually written anything.  should
this be done below?

[Henrik Grunell, 2015/09/03 15:29:51]

It actually counts Write() calls, attempted write. Moved up to top of function
to clarify that + clarified the comment in header.

+
+  // Read verifications from other side as long as there is any.
+  while (true) {
+    if (socket_->Peek() == 0)
+      break;
+
+    uint32 index_read = 0;
+    size_t bytes_received = socket_->ReceiveWithTimeout(

[tommi (sloooow) - chröme, 2015/09/03 14:55:13]

Is it possible to read everything that's in the buffer in one call?

scoped_ptr<int> indices(new int[<segment count>]);
size_t bytes_received = socket_->ReceiveWithTimeout(&indices[0], <count *
sizeof(indices[0])>, ...);
<loop through and check all received indices>

number_of_filled_segments_ -= bytes_received / sizeof(indices[0]);
etc.

[Henrik Grunell, 2015/09/03 15:29:51]

That would mean that we have to timeout every time we call ReceiveWithTimeout,
which is not good. And we probably don't want to rely on what Peek() tells us is
available based on the caveat in the comment for it.

[tommi (sloooow) - chröme, 2015/09/03 15:47:52]

We can rely on what Peek() tells us as long as we treat it as a bool (as you're
doing already).  I wasn't suggesting to skip the call to Peek() btw, so this
approach would be at worst equally costly as the current approach, and at best,
less so, since it involves fewer context switches or user/kernel mode switches.

[Henrik Grunell, 2015/09/04 11:52:52]

Tried this, but both Receive and ReceiveWTO blocks. So unfortunately it doesn't
work.

+        &index_read, sizeof(index_read), base::TimeDelta::FromMilliseconds(5));
+    if (bytes_received != sizeof(index_read)) {
+      // TODO(grunell): This could happen if this thread has bad luck and don't

[tommi (sloooow) - chröme, 2015/09/03 14:55:13]

nit: ...and don't -> ...and doesn't

[Henrik Grunell, 2015/09/03 15:29:51]

Done.

+      // get to run before the timeout dealine. Otherwise it should never

[tommi (sloooow) - chröme, 2015/09/03 14:55:13]

deadline

[Henrik Grunell, 2015/09/03 15:29:51]

Done.

+      // happen. Best would be to have a non-blocking Receive().
+      break;
+    }
+
+    ++next_read_buffer_index_;
+    CHECK_EQ(index_read, next_read_buffer_index_);
+    --number_of_filled_segments_;
+    CHECK_GE(number_of_filled_segments_, 0);
+  }
+
+  // Check that the ring buffer isn't full and if it is, log error and drop the
+  // buffer.
+  if (number_of_filled_segments_ ==
+      static_cast<int>(shared_memory_segment_count_)) {
+    const std::string error_message =
+        "No room in ring buffer to write data to. Dropping the data.";
+    LOG(ERROR) << error_message;
+    AddToNativeLog(error_message);
+    ++write_error_count_;
+    return;
+  }
+
   // Write audio parameters to shared memory.
-  uint8* ptr = static_cast<uint8*>(shared_memory_->memory());
+  uint8* ptr = shared_memory_;
   ptr += current_segment_id_ * shared_memory_segment_size_;
   media::AudioInputBuffer* buffer =
       reinterpret_cast<media::AudioInputBuffer*>(ptr);
@@ -97,6 +173,10 @@ void AudioInputSyncWriter::Write(const media::AudioBus* data,
 
   if (++current_segment_id_ >= shared_memory_segment_count_)
     current_segment_id_ = 0;
+
+  ++number_of_filled_segments_;
+  CHECK_LE(number_of_filled_segments_,
+           static_cast<int>(shared_memory_segment_count_));
 }
 
 void AudioInputSyncWriter::Close() {
@@ -116,5 +196,8 @@ bool AudioInputSyncWriter::PrepareForeignSocket(
   return foreign_socket_->PrepareTransitDescriptor(process_handle, descriptor);
 }
 
+void AudioInputSyncWriter::AddToNativeLog(const std::string& message) {
+  MediaStreamManager::SendMessageToNativeLog(message);
+}
 
 }  // namespace content