Refactor AudioInputController and related interfaces.

media/audio/audio_input_controller.h

Index: media/audio/audio_input_controller.h
diff --git a/media/audio/audio_input_controller.h b/media/audio/audio_input_controller.h
index 5d4a94aeab57dd8f462124436675cf11808fabbf..05d064be75c79eb8c30c37344d7a1ce12f033fc5 100644
--- a/media/audio/audio_input_controller.h
+++ b/media/audio/audio_input_controller.h
@@ -11,15 +11,7 @@
 #include <memory>
 #include <string>
 
-#include "base/atomicops.h"
-#include "base/callback.h"
 #include "base/files/file.h"
-#include "base/macros.h"
-#include "base/memory/ref_counted.h"
-#include "base/synchronization/lock.h"
-#include "base/synchronization/waitable_event.h"
-#include "base/threading/thread.h"
-#include "build/build_config.h"
 #include "media/audio/audio_io.h"
 #include "media/audio/audio_manager_base.h"
 #include "media/base/audio_bus.h"
@@ -55,14 +47,11 @@
 //                        AudioInputStream::Open()
 //                                  .- - - - - - - - - - - - ->   OnError()
 //                                  .------------------------->  OnCreated()
-//                               kCreated
 // - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -
 // Record() ==>                 DoRecord()
 //                      AudioInputStream::Start()
-//                              kRecording
 // - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -
 // Close() ==>                  DoClose()
-//                           state_ = kClosed
 //                        AudioInputStream::Stop()
 //                        AudioInputStream::Close()
 //                          SyncWriter::Close()
@@ -85,8 +74,7 @@ class AudioInputWriter;
 class UserInputMonitor;
 
 class MEDIA_EXPORT AudioInputController
-    : public base::RefCountedThreadSafe<AudioInputController>,
-      public AudioInputStream::AudioInputCallback {
+    : public base::RefCountedThreadSafe<AudioInputController> {
  public:
   // Error codes to make native logging more clear. These error codes are added
   // to generic error strings to provide a higher degree of details.
@@ -114,8 +102,6 @@ class MEDIA_EXPORT AudioInputController
     virtual void OnCreated(AudioInputController* controller) = 0;
     virtual void OnError(AudioInputController* controller,
                          ErrorCode error_code) = 0;
-    virtual void OnData(AudioInputController* controller,
-                        const AudioBus* data) = 0;
     virtual void OnLog(AudioInputController* controller,
                        const std::string& message) = 0;
 
@@ -164,6 +150,7 @@ class MEDIA_EXPORT AudioInputController
   static scoped_refptr<AudioInputController> Create(
       AudioManager* audio_manager,
       EventHandler* event_handler,
+      SyncWriter* sync_writer,
       const AudioParameters& params,
       const std::string& device_id,
       UserInputMonitor* user_input_monitor);
@@ -221,16 +208,6 @@ class MEDIA_EXPORT AudioInputController
   // to muted and 1.0 to maximum volume.
   virtual void SetVolume(double volume);
 
-  // AudioInputCallback implementation. Threading details depends on the
-  // device-specific implementation.
-  void OnData(AudioInputStream* stream,
-              const AudioBus* source,
-              uint32_t hardware_delay_bytes,
-              double volume) override;
-  void OnError(AudioInputStream* stream) override;
-
-  bool SharedMemoryAndSyncSocketMode() const { return sync_writer_ != NULL; }
-
   // Enable debug recording of audio input.
   virtual void EnableDebugRecording(const base::FilePath& file_name);
 
@@ -260,13 +237,6 @@ class MEDIA_EXPORT AudioInputController
     CAPTURE_STARTUP_RESULT_MAX = CAPTURE_STARTUP_NEVER_GOT_DATA
   };
 
-  // Internal state of the source.
-  enum State {
-    CREATED,
-    RECORDING,
-    CLOSED
-  };
-
 #if defined(AUDIO_POWER_MONITORING)
   // Used to log a silence report (see OnData).
   // Elements in this enum should not be deleted or rearranged; the only
@@ -287,13 +257,22 @@ class MEDIA_EXPORT AudioInputController
   };
 #endif
 
-  AudioInputController(EventHandler* handler,
+  AudioInputController(scoped_refptr<base::SingleThreadTaskRunner> task_runner,
+                       EventHandler* handler,
                        SyncWriter* sync_writer,
                        std::unique_ptr<AudioInputWriter> debug_writer,
                        UserInputMonitor* user_input_monitor,
                        const bool agc_is_enabled);
-  ~AudioInputController() override;
+  virtual ~AudioInputController();
+
+  const scoped_refptr<base::SingleThreadTaskRunner>& GetTaskRunnerForTesting()
+      const {
+    return task_runner_;
+  }
 
+  EventHandler* GetHandlerForTesting() const { return handler_; }
+
+ private:
   // Methods called on the audio thread (owned by the AudioManager).
   void DoCreate(AudioManager* audio_manager,
                 const AudioParameters& params,
@@ -306,12 +285,8 @@ class MEDIA_EXPORT AudioInputController
   void DoClose();
   void DoReportError();
   void DoSetVolume(double volume);
-  void DoOnData(std::unique_ptr<AudioBus> data);
   void DoLogAudioLevels(float level_dbfs, int microphone_volume_percent);
 
-  // Helper method that stops, closes, and NULL:s |*stream_|.
-  void DoStopCloseAndClearStream();
-
 #if defined(AUDIO_POWER_MONITORING)
   // Updates the silence state, see enum SilenceState above for state
   // transitions.
@@ -336,39 +311,36 @@ class MEDIA_EXPORT AudioInputController
   // Called by the stream with log messages.
   void LogMessage(const std::string& message);
 
+  // Called on the hw callback thread. Checks for keyboard input if
+  // user_input_monitor_ is set otherwise returns false.
+  bool CheckForKeyboardInput();
+
+  // Does power monitoring on supported platforms.
+  // Called on the hw callback thread.
+  void CheckAudioPower(const AudioBus* source, double volume);
+
   // Gives access to the task runner of the creating thread.
-  scoped_refptr<base::SingleThreadTaskRunner> creator_task_runner_;
+  scoped_refptr<base::SingleThreadTaskRunner> const creator_task_runner_;
 
   // The task runner of audio-manager thread that this object runs on.
-  scoped_refptr<base::SingleThreadTaskRunner> task_runner_;
+  scoped_refptr<base::SingleThreadTaskRunner> const task_runner_;
 
   // Contains the AudioInputController::EventHandler which receives state
   // notifications from this class.
-  EventHandler* handler_;
+  EventHandler* const handler_;
 
   // Pointer to the audio input stream object.
+  // Only used on the audio thread.
   AudioInputStream* stream_;
 
-  // Flag for whether CaptureStartupResults shall be reported.
-  // A value of 1 means that stats shall be reported,
-  // any other value means that stats have already been reported.
-  base::AtomicRefCount should_report_stats;
-
-  // |state_| is written on the audio thread and is read on the hardware audio
-  // thread. These operations need to be locked. But lock is not required for
-  // reading on the audio input controller thread.
-  State state_;
-
-  base::Lock lock_;
-
   // SyncWriter is used only in low-latency mode for synchronous writing.
-  SyncWriter* sync_writer_;
+  SyncWriter* const sync_writer_;
 
   static Factory* factory_;
 
   double max_volume_;
 
-  UserInputMonitor* user_input_monitor_;
+  UserInputMonitor* const user_input_monitor_;
 
   const bool agc_is_enabled_;
 
@@ -394,6 +366,12 @@ class MEDIA_EXPORT AudioInputController
   // Used for audio debug recordings. Accessed on audio thread.
   const std::unique_ptr<AudioInputWriter> debug_writer_;
 
+  class AudioCallback;
+  // Holds a pointer to the callback object that receives audio data from
+  // the lower audio layer. Valid only while 'recording' (aka capturing).
+  // The value of this pointer is only set and read on the audio thread.

[o1ka, 2017/01/12 12:07:39]

It is also accessed on audio callback thread. Doesn't it mean that we have a
data race if callback execution is in progress on callback thread while
|audio_callback_| is reset on audio thread in AudioInputController::DoClose()?
See coment to AudioInputStream::Stop()
(https://cs.chromium.org/chromium/src/media/audio/audio_io.h?sq=package:chromi...):
"// Stops recording audio. Effect might not be instantaneous as there could be
  // pending audio callbacks in the queue which will be issued first before
  // recording stops."

[tommi (sloooow) - chröme, 2017/01/12 14:21:01]

Thanks for pointing out that documentation.  It's wrong :)

It was also written 8 years ago it seems.  It was correct for a long while but
we had a lot of issues (crashes) with that behavior and made all implementations
of Stop() complete synchronously.  I've updated the comment in audio_io.h.

[o1ka, 2017/01/12 17:27:15]

It's not obvious from platform-specific implementations at all, and there is no
single comment on it in that code. So I blindly trust you :)
Anyways, if such race is possible the current version seems to have it, too.

Would be nice to mention that it is accessed on hw callback thread and must be
valid between stream_::Start() and stream_::Stop().

[tommi (sloooow) - chröme, 2017/01/12 18:58:20]

I was trying to convey that with "Valid only while 'recording' (aka capturing).
The value of this pointer is only set and read on the audio thread." but since
that wasn't obvious, I guess I failed :)  Updated now.

+  std::unique_ptr<AudioCallback> audio_callback_;
+
  private:
   DISALLOW_COPY_AND_ASSIGN(AudioInputController);
 };