Audio input debug recording refactoring to reduce thread hops and simplify object ownership

content/browser/renderer_host/media/audio_input_debug_writer.cc

Index: content/browser/renderer_host/media/audio_input_debug_writer.cc
diff --git a/content/browser/renderer_host/media/audio_input_debug_writer.cc b/content/browser/renderer_host/media/audio_input_debug_writer.cc
index 7866c651a34288ab83a26094cdf3d334cf2ed549..3d0121a64d355ef58d9e0d71c4f20a74780ede0e 100644
--- a/content/browser/renderer_host/media/audio_input_debug_writer.cc
+++ b/content/browser/renderer_host/media/audio_input_debug_writer.cc
@@ -7,6 +7,7 @@
 #include <array>
 #include <utility>
 #include "base/logging.h"
+#include "base/memory/ptr_util.h"
 #include "base/sys_byteorder.h"
 #include "content/public/browser/browser_thread.h"
 #include "media/base/audio_bus.h"
@@ -127,36 +128,64 @@ void WriteWavHeader(WavHeaderBuffer* buf,
 
 }  // namespace
 
-AudioInputDebugWriter::AudioInputDebugWriter(
-    base::File file,
+class AudioInputDebugWriter::AudioFileWriter {
+ public:
+  AudioFileWriter(const base::FilePath& file_name,
+                  const media::AudioParameters& params);
+
+  // Must be called on FILE thread.
+  ~AudioFileWriter();
+
+  // Write data from |data| to file. Called on the FILE thread.
+  void Write(const media::AudioBus* data);
+
+  // Must be called on FILE thread.
+  void Close();
+
+ private:
+  // Write wave header to file. Called on the FILE thread twice: on construction
+  // of AudioFileWriter size of the wave data is unknown, so the header is
+  // written with zero sizes; then on destruction it is re-written with the
+  // actual size info accumulated throughout the object lifetime.
+  void WriteHeader();
+
+  void CreateRecordingFile(const base::FilePath& file_name);
+
+  // The file to write to.
+  base::File file_;
+
+  // Number of written samples.
+  uint64_t samples_;
+
+  // Input audio parameters required to build wave header.
+  const media::AudioParameters params_;
+
+  // Intermediate buffer to be written to file. Interleaved 16 bit audio data.
+  std::unique_ptr<int16_t[]> interleaved_data_;
+  int interleaved_data_size_;

[Guido Urdaneta, 2016/10/10 10:09:24]

nit: should this be size_t?

[o1ka, 2016/10/10 13:48:01]

We get the value by multiplying two ints (AudioBus::frames() *
AudioBus::channels()). Don't think we want type conversion. WDYT?

[Guido Urdaneta, 2016/10/10 14:53:22]

leave as int then

+};
+
+AudioInputDebugWriter::AudioFileWriter::AudioFileWriter(
+    const base::FilePath& file_name,
     const media::AudioParameters& params)
-    : file_(std::move(file)),
-      samples_(0),
-      params_(params),
-      interleaved_data_size_(0),
-      weak_factory_(this) {
+    : samples_(0), params_(params), interleaved_data_size_(0) {
   DCHECK_EQ(params.bits_per_sample(), kBytesPerSample * 8);
+  // base::Unretained is safe, because destructor is called on FILE thread.

[Guido Urdaneta, 2016/10/10 10:09:24]

nit: It would be good to clarify that all other operations also run on the FILE
thread, so that no operations will work with a partially constructed AFW.

[o1ka, 2016/10/10 13:48:01]

Well... Actually it will :) Fixing it. 

   BrowserThread::PostTask(BrowserThread::FILE, FROM_HERE,
-                          base::Bind(&AudioInputDebugWriter::WriteHeader,
-                                     weak_factory_.GetWeakPtr()));
+                          base::Bind(&AudioFileWriter::CreateRecordingFile,
+                                     base::Unretained(this), file_name));
 }
 
-AudioInputDebugWriter::~AudioInputDebugWriter() {
+AudioInputDebugWriter::AudioFileWriter::~AudioFileWriter() {
   DCHECK_CURRENTLY_ON(BrowserThread::FILE);
-  WriteHeader();
 }
 
-void AudioInputDebugWriter::Write(std::unique_ptr<media::AudioBus> data) {
-  BrowserThread::PostTask(
-      BrowserThread::FILE,
-      FROM_HERE,
-      base::Bind(&AudioInputDebugWriter::DoWrite,
-                 weak_factory_.GetWeakPtr(),
-                 base::Passed(&data)));
-}
-
-void AudioInputDebugWriter::DoWrite(std::unique_ptr<media::AudioBus> data) {
+void AudioInputDebugWriter::AudioFileWriter::Write(
+    const media::AudioBus* data) {
   DCHECK_CURRENTLY_ON(BrowserThread::FILE);
+  if (!file_.IsValid())
+    return;
+
   // Convert to 16 bit audio and write to file.
   int data_size = data->frames() * data->channels();
   if (!interleaved_data_ || interleaved_data_size_ < data_size) {
@@ -178,12 +207,15 @@ void AudioInputDebugWriter::DoWrite(std::unique_ptr<media::AudioBus> data) {
                           data_size * sizeof(interleaved_data_[0]));
 }
 
-// This method is called twice: on construction of AudioInputDebugWriter size of
-// the data is unknown, so the header is written with zero sizes; then on
-// destruction it is re-written with the actual size info accumulated throughout
-// its lifetime.
-void AudioInputDebugWriter::WriteHeader() {
+void AudioInputDebugWriter::AudioFileWriter::Close() {
   DCHECK_CURRENTLY_ON(BrowserThread::FILE);
+  if (file_.IsValid())
+    WriteHeader();
+}
+
+void AudioInputDebugWriter::AudioFileWriter::WriteHeader() {
+  DCHECK_CURRENTLY_ON(BrowserThread::FILE);
+  DCHECK(file_.IsValid());
 
   WavHeaderBuffer buf;
   WriteWavHeader(&buf, params_.channels(), params_.sample_rate(), samples_);
@@ -194,4 +226,93 @@ void AudioInputDebugWriter::WriteHeader() {
   file_.Seek(base::File::FROM_BEGIN, kWavHeaderSize);
 }
 
+void AudioInputDebugWriter::AudioFileWriter::CreateRecordingFile(
+    const base::FilePath& file_name) {
+  DCHECK_CURRENTLY_ON(BrowserThread::FILE);
+
+  file_ = base::File(file_name,
+                     base::File::FLAG_CREATE_ALWAYS | base::File::FLAG_WRITE);
+
+  if (file_.IsValid()) {
+    WriteHeader();
+    return;
+  }
+
+  // Note that we do not inform AudioInputDebugWriter that the file creation
+  // fails, so it will continue to post data to be recorded, which won't
+  // be written to the file. This also won't be reflect in IsRecprding(). It's

[Guido Urdaneta, 2016/10/10 10:09:23]

typo: IsRecprding -> IsRecording

[o1ka, 2016/10/10 13:48:01]

Done.

+  // fine, because this situation is rare, and all the posting is expected to
+  // happen in case of success anyways. This allow us to save on thread hops for
+  // error reporting and to avoid dealing with lifetime issues.
+  PLOG(ERROR) << "Could not open debug recording file, error="
+              << file_.error_details();
+}
+
+AudioInputDebugWriter::AudioInputDebugWriter(
+    const media::AudioParameters& params)
+    : params_(params) {
+  client_sequence_checker_.DetachFromSequence();
+}
+
+AudioInputDebugWriter::~AudioInputDebugWriter() {
+  // Callback takes ownership of |file_writer_|, so it will be deleted after

[Guido Urdaneta, 2016/10/10 10:09:23]

It would be good to make it clearer what "Callback" is.

[o1ka, 2016/10/10 13:48:01]

Done.

+  // Close() is executed or when FILE thread message loop is destroyed. Posting

[Henrik Grunell, 2016/10/10 10:56:59]

This could mean that the header isn't updated at a browser shutdown. This is
likely not going to happen often; I think we can live with that.

+  // non-nestable to make sure it is executed after all the writes are

[o1ka, 2016/10/10 08:44:58]

Actually, I'm not quite sure I interpreted this correctly. Should it be
non-nestable?

[Guido Urdaneta, 2016/10/10 10:09:23]

It's also not clear to me. Why would it be incorrect if you use PostTask?
Same below.

[Henrik Grunell, 2016/10/10 10:56:59]

It should only be needed if the writes call any task-running method. But it
should also be OK to do this as a guarantee, to increase readability and avoid
regression.

[o1ka, 2016/10/10 13:48:01]

I don't really understand what is behind this. But I see that Sequenced task
runner posts non-nestable task for destructor.
https://cs.chromium.org/chromium/src/base/sequenced_task_runner.h?q=DeleteSoo...
Should we post all the writes as non-nestable to avoid races in file access?

+  // completed.
+  if (file_writer_) {
+    BrowserThread::PostNonNestableTask(
+        BrowserThread::FILE, FROM_HERE,
+        base::Bind(&AudioFileWriter::Close,
+                   base::Owned(file_writer_.release())));
+  }
+}
+
+void AudioInputDebugWriter::Start(const base::FilePath& file_name) {
+  DCHECK(client_sequence_checker_.CalledOnValidSequence());
+  DCHECK(!file_writer_);
+  file_writer_.reset(new AudioFileWriter(file_name, params_));
+
+  base::AutoLock auto_lock(recording_lock_);
+  is_recording_ = true;
+}
+
+void AudioInputDebugWriter::Stop() {
+  DCHECK(client_sequence_checker_.CalledOnValidSequence());
+  {
+    base::AutoLock auto_lock(recording_lock_);
+    is_recording_ = false;
+  }
+
+  if (file_writer_) {
+    // Callback takes ownership of |file_writer_|, so it will be deleted after
+    // Close() is executed or when FILE thread message loop is destroyed.
+    // Posting

[Guido Urdaneta, 2016/10/10 10:09:24]

fix comment flow (A whole line just for Posting).

[o1ka, 2016/10/11 12:11:24]

Done.

+    // non-nestable to make sure it is executed after all the writes are
+    // completed.
+    BrowserThread::PostNonNestableTask(
+        BrowserThread::FILE, FROM_HERE,
+        base::Bind(&AudioFileWriter::Close,
+                   base::Owned(file_writer_.release())));
+  }
+
+  client_sequence_checker_.DetachFromSequence();
+}
+
+void AudioInputDebugWriter::Write(std::unique_ptr<media::AudioBus> data) {
+  DCHECK(client_sequence_checker_.CalledOnValidSequence());
+  DCHECK(file_writer_);
+
+  // base::Unretained for |file_writer_| is safe, see the destructor.
+  // Callback takes ownership of |data|.
+  BrowserThread::PostTask(
+      BrowserThread::FILE, FROM_HERE,
+      base::Bind(&AudioFileWriter::Write, base::Unretained(file_writer_.get()),
+                 base::Owned(data.release())));
+}
+
+bool AudioInputDebugWriter::IsRecording() {
+  base::AutoLock auto_lock(recording_lock_);
+  return is_recording_;
+}
+
 }  // namspace content