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

 // 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/browser/renderer_host/media/audio_input_sync_writer.h"
 
 #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);
     media::AudioInputBuffer* buffer =
         reinterpret_cast<media::AudioInputBuffer*>(ptr);
     scoped_ptr<media::AudioBus> audio_bus =
         media::AudioBus::WrapMemory(params, buffer->audio);
     audio_buses_.push_back(audio_bus.release());
     ptr += shared_memory_segment_size_;
   }
 }
 
-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,
                                  bool key_pressed,
                                  uint32 hardware_delay_bytes) {
 #if !defined(OS_ANDROID)
   static const base::TimeDelta kLogDelayThreadhold =
       base::TimeDelta::FromMilliseconds(500);
 
   std::ostringstream oss;
   if (last_write_time_.is_null()) {
     // This is the first time Write is called.
     base::TimeDelta interval = base::Time::Now() - creation_time_;
     oss << "AISW::Write: audio input data received for the first time: delay "
            "= " << interval.InMilliseconds() << "ms";
 
   } else {
     base::TimeDelta interval = base::Time::Now() - last_write_time_;
     if (interval > kLogDelayThreadhold) {
       oss << "AISW::Write: audio input data delay unexpectedly long: delay = "
           << interval.InMilliseconds() << "ms";
     }
   }
   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);
   buffer->params.volume = volume;
   buffer->params.size = audio_bus_memory_size_;
   buffer->params.key_pressed = key_pressed;
   buffer->params.hardware_delay_bytes = hardware_delay_bytes;
   buffer->params.id = next_buffer_id_++;
 
   // Copy data from the native audio layer into shared memory using pre-
   // allocated audio buses.
   media::AudioBus* audio_bus = audio_buses_[current_segment_id_];
   data->CopyTo(audio_bus);
 
   socket_->Send(&current_segment_id_, sizeof(current_segment_id_));
 
   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() {
   socket_->Close();
 }
 
 bool AudioInputSyncWriter::Init() {
   socket_.reset(new base::CancelableSyncSocket());
   foreign_socket_.reset(new base::CancelableSyncSocket());
   return base::CancelableSyncSocket::CreatePair(socket_.get(),
                                                 foreign_socket_.get());
 }
 
 bool AudioInputSyncWriter::PrepareForeignSocket(
     base::ProcessHandle process_handle,
     base::SyncSocket::TransitDescriptor* descriptor) {
   return foreign_socket_->PrepareTransitDescriptor(process_handle, descriptor);
 }
 
+void AudioInputSyncWriter::AddToNativeLog(const std::string& message) {
+  MediaStreamManager::SendMessageToNativeLog(message);
+}
 
 }  // namespace content