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..8162e57015966581029eb1c0025fe78cd1916de6 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,55 @@
 
 #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 {
+
+// TODO BEFORE COMMIT: Which value makes sense? I.e. how long can we accept
+// blocking the soundcard thread and wait before dropping data.
+const base::TimeDelta kReadCheckTimeout =

[DaleCurtis, 2015/09/02 16:18:50]

This is a static initializer, you'll need to keep this as a primitive type and
convert to a timedelta inside a function.

As for the timeout itself, can you just use the same one we use in the reader?
duration / 2?

[Henrik Grunell, 2015/09/03 14:32:33]

I've removed this and changed the verification algorithm.

+    base::TimeDelta::FromMilliseconds(10);
+
+// 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),
+      expected_buffer_reads_(0),
+      read_verification_count_(0),
+      read_verification_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 +67,27 @@ AudioInputSyncWriter::AudioInputSyncWriter(base::SharedMemory* shared_memory,
   }
 }
 
-AudioInputSyncWriter::~AudioInputSyncWriter() {}
+AudioInputSyncWriter::~AudioInputSyncWriter() {
+  if (read_verification_count_ == 0)
+    return;
+
+  UMA_HISTOGRAM_PERCENTAGE(
+      "Media.AudioCapturerMissedReadDeadline",
+      100.0 * read_verification_error_count_ / read_verification_count_);
+
+  UMA_HISTOGRAM_ENUMERATION("Media.AudioCapturerAudioGlitches",
+                            read_verification_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",
+      read_verification_error_count_,
+      read_verification_count_);
+  MediaStreamManager::SendMessageToNativeLog(log_string);
+  DVLOG(1) << log_string;
+}
 
 void AudioInputSyncWriter::Write(const media::AudioBus* data,
                                  double volume,
@@ -70,15 +112,49 @@ 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 count the number of reads we expect to have been done. When
+  // we reach |shared_memory_segment_count_|, we do a socket receive, which
+  // normally is in queue. If we timeout, there's a problem with being able to
+  // read at the high enough pace, and we throw away the audio buffer.
+  if (expected_buffer_reads_ ==
+      static_cast<int>(shared_memory_segment_count_)) {
+    ++read_verification_count_;
+    ++next_read_buffer_index_;
+    size_t bytes_received = 0;
+    uint32 index_read = 0;
+    bytes_received = socket_->ReceiveWithTimeout(

[DaleCurtis, 2015/09/02 16:18:50]

This will work, but I suspect during periods of high load it'll take longer for
the user to get back to a glitch free experience. With this if 10 slots get
filled, you'll only be working with 9 for the rest of the session -- so you have
no buffer for future periods of load.

I think it's still worth having the while loop to speed up recovery, you just
want to make sure that you decrease timeout according to the time taken by each
receive (just like is done in the AUdioSyncReader).

[Henrik Grunell, 2015/09/03 14:32:33]

It can't really work like on the output side since both are driven form the
browser process side. (I.e. consumer for output and producer for input.) I did
rewrite to instead receive as much data as possible, but peek first to avoid
timeout altogether. Then simply check if the ring buffer is full or not. Since
we have a buffer already, of 100 ms as of now, having a timeout is redundant
really. If the ring buffer is full we have already "waited" the last 9 writes,
no point waiting any more if the read side hasn't been able to read.

+        &index_read, sizeof(index_read), kReadCheckTimeout);
+    if (bytes_received != sizeof(index_read)) {
+      const std::string error_message =
+          "Verifying shared memory read timed out. Dropping audio data.";
+      LOG(ERROR) << error_message;
+      AddToNativeLog(error_message);
+      ++read_verification_error_count_;
+      return;
+    } else if (index_read != next_read_buffer_index_) {

[DaleCurtis, 2015/09/02 16:18:50]

Hmm, this should never happen right? I think this should be a DCHECK/CHECK
instead then.

[Henrik Grunell, 2015/09/03 14:32:33]

Right, done.

+      const std::string error_message = base::StringPrintf(
+          "Shared memory read index mismatch. Expected = %u. Actual = %u",
+          next_read_buffer_index_, index_read);
+      LOG(ERROR) << error_message;
+      AddToNativeLog(error_message);
+      ++read_verification_error_count_;
+      next_read_buffer_index_ = index_read + 1;
+    }
+    --expected_buffer_reads_;
+    DCHECK_GE(expected_buffer_reads_, 0);
+  }
+
   // 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;
+
+  ++expected_buffer_reads_;
+  DCHECK_LE(expected_buffer_reads_,
+            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