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..3ddc01e60778c77fc70ad1dbfbba7504b8c81783 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,82 @@ void WriteWavHeader(WavHeaderBuffer* buf,
 
 }  // namespace
 
-AudioInputDebugWriter::AudioInputDebugWriter(
-    base::File file,
-    const media::AudioParameters& params)
-    : file_(std::move(file)),
-      samples_(0),
-      params_(params),
-      interleaved_data_size_(0),
-      weak_factory_(this) {
-  DCHECK_EQ(params.bits_per_sample(), kBytesPerSample * 8);
-  BrowserThread::PostTask(BrowserThread::FILE, FROM_HERE,
-                          base::Bind(&AudioInputDebugWriter::WriteHeader,
-                                     weak_factory_.GetWeakPtr()));
+class AudioInputDebugWriter::AudioFileWriter {
+ public:
+  static std::unique_ptr<AudioFileWriter> Create(
+      const base::FilePath& file_name,
+      const media::AudioParameters& params);
+
+  // Must be called on FILE thread or on FILE message loop destruction.
+  ~AudioFileWriter();

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

The question I have not found an answer for so far: if the message loop is being
destroyed, which thread unexecuted callbacks are delete on? I.e., if I have a
thread check in a destructor of an object, and this object is passed to a
callback as base::Owned() - will this thread check fire on callback deletion?

+
+  // Write data from |data| to file. Called on the FILE thread.
+  void Write(const media::AudioBus* data);
+
+  // Must be called on FILE thread.

[Henrik Grunell, 2016/10/11 13:07:24]

Nit: Does this class live only on the FILE thread? If so, you should have a
class comment for that and remove these.

[o1ka, 2016/10/11 14:04:36]

Done.

+  void Close();
+
+ private:
+  AudioFileWriter(const media::AudioParameters& params);
+
+  // 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_;
+};
+
+// static
+std::unique_ptr<AudioInputDebugWriter::AudioFileWriter>
+AudioInputDebugWriter::AudioFileWriter::Create(
+    const base::FilePath& file_name,
+    const media::AudioParameters& params) {
+  std::unique_ptr<AudioInputDebugWriter::AudioFileWriter> file_writer(
+      new AudioFileWriter(params));
+
+  // base::Unretained is safe, because destructor is called on FILE thread or on
+  // FILE message loop destruction.
+  BrowserThread::PostTask(
+      BrowserThread::FILE, FROM_HERE,
+      base::Bind(&AudioFileWriter::CreateRecordingFile,
+                 base::Unretained(file_writer.get()), file_name));
+  return file_writer;
 }
 
-AudioInputDebugWriter::~AudioInputDebugWriter() {
-  DCHECK_CURRENTLY_ON(BrowserThread::FILE);
-  WriteHeader();
+AudioInputDebugWriter::AudioFileWriter::AudioFileWriter(
+    const media::AudioParameters& params)
+    : samples_(0), params_(params), interleaved_data_size_(0) {
+  DCHECK_EQ(params.bits_per_sample(), kBytesPerSample * 8);
 }
 
-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)));
+AudioInputDebugWriter::AudioFileWriter::~AudioFileWriter() {

[Henrik Grunell, 2016/10/11 13:07:24]

Nit: DCHECK correct thread?

[o1ka, 2016/10/11 14:04:37]

Seem my question above :) But I changed the code a little bit now. From what I
see in other places now it's fine to have a DCHECK.

[Henrik Grunell, 2016/10/13 07:26:11]

Great!

+  // In case a scheduled Close() call has not been executed because message loop
+  // is destroyed.
+  if (file_.IsValid())
+    WriteHeader();
 }
 
-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,13 +225,17 @@ 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();
+  file_ = base::File();
+}
 
+void AudioInputDebugWriter::AudioFileWriter::WriteHeader() {
+  DCHECK_CURRENTLY_ON(BrowserThread::FILE);
+  if (!file_.IsValid())
+    return;
   WavHeaderBuffer buf;
   WriteWavHeader(&buf, params_.channels(), params_.sample_rate(), samples_);
   file_.Write(0, &buf[0], kWavHeaderSize);
@@ -194,4 +245,81 @@ void AudioInputDebugWriter::WriteHeader() {
   file_.Seek(base::File::FROM_BEGIN, kWavHeaderSize);
 }
 
+void AudioInputDebugWriter::AudioFileWriter::CreateRecordingFile(
+    const base::FilePath& file_name) {
+  DCHECK_CURRENTLY_ON(BrowserThread::FILE);
+  DCHECK(!file_.IsValid());
+
+  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 WillWrite(). It's

[Guido Urdaneta, 2016/10/11 12:39:25]

nit: reflect -> reflected

[o1ka, 2016/10/11 14:04:37]

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

[Guido Urdaneta, 2016/10/11 12:39:25]

nit: allow -> allows

[o1ka, 2016/10/11 14:04:36]

Done.

+  // error reporting and to avoid dealing with lifetime issues. It also means
+  // file_.IsValid() should always be checked before issuing writes to it.
+  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() {
+  Stop();
+}
+
+void AudioInputDebugWriter::Start(const base::FilePath& file_name) {
+  DCHECK(client_sequence_checker_.CalledOnValidSequence());
+  DCHECK(!file_writer_);
+  file_writer_ = AudioFileWriter::Create(file_name, params_);
+}
+
+void AudioInputDebugWriter::Stop() {
+  DCHECK(client_sequence_checker_.CalledOnValidSequence());
+  // Callback takes ownership of |file_writer_|, so it will be deleted after
+  // Close() is executed or when FILE thread message loop is destroyed. Posting
+  // non-nestable just like in SequencedTaskRunner::DeleteSoon().

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

I still do not quite understand this "nestability".

[Henrik Grunell, 2016/10/11 13:07:24]

Afaict, if you want to guarantee it's run after any currently running tasks, you
need that. And not have to make assumptions on what the currently running tasks
are doing.

[o1ka, 2016/10/11 14:04:36]

 Ok, looks like non-netsable post is needed if I want to serialize tasks which
run other tasks, which is not our case. Removing it.

+  if (file_writer_) {
+    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());
+  if (!file_writer_)
+    return;
+
+  // base::Unretained for |file_writer_| is safe, see the destructor.
+  BrowserThread::PostTask(
+      BrowserThread::FILE, FROM_HERE,
+      // Callback takes ownership of |data|:
+      base::Bind(&AudioFileWriter::Write, base::Unretained(file_writer_.get()),
+                 base::Owned(data.release())));
+}
+
+bool AudioInputDebugWriter::WillWrite() {
+  // Note that if this is called from any place other than
+  // |client_sequence_checker_|

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

will fix (git cl format did not do a perfect job here)

[Henrik Grunell, 2016/10/11 13:07:24]

git cl format doesn't format comments afaik.

[o1ka, 2016/10/11 14:04:36]

it wraps
lines.

+  // then there is a data race here, but it's fine, because Write() will check
+  // for |file_writer_|. So, we are not very precies here, but it's fine: we can

[Guido Urdaneta, 2016/10/11 12:39:25]

nit: precies -> precise

[o1ka, 2016/10/11 14:04:36]

Done.

+  // afford missing some data or scheduling some no-op writes.
+  return !!file_writer_;
+}
+
 }  // namspace content