Introduce audio debug recording session.

* Add AudioDebugRecordingSession factory
* Enable debug recording in AudioDebugRecordingSessionImpl constructor
* Disable debug recording in AudioDebugRecordingSessionImpl destructor

Bug: 88657
Cq-Include-Trybots: master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
Change-Id: I44383d0994b8eebfd36c368827973e614937d8cd
Reviewed-on: https://chromium-review.googlesource.com/883528Reviewed-by: Jochen Eisinger <jochen@chromium.org>
Reviewed-by: Guido Urdaneta <guidou@chromium.org>
Reviewed-by: Olga Sharonova <olka@chromium.org>
Commit-Queue: Marina Ciocea <marinaciocea@chromium.org>
Cr-Commit-Position: refs/heads/master@{#532211}

chrome/browser/chrome_content_browser_client.cc

@@ -1078,7 +1078,7 @@ void ChromeContentBrowserClient::RenderProcessWillLaunch(
   // The audio manager outlives the host, so it's safe to hand a raw pointer to
   // it to the AudioDebugRecordingsHandler, which is owned by the host.
   AudioDebugRecordingsHandler* audio_debug_recordings_handler =
-      new AudioDebugRecordingsHandler(profile, media::AudioManager::Get());
+      new AudioDebugRecordingsHandler(profile);
   host->SetUserData(
       AudioDebugRecordingsHandler::kAudioDebugRecordingsHandlerKey,
       base::MakeUnique<base::UserDataAdapter<AudioDebugRecordingsHandler>>(

chrome/browser/media/webrtc/audio_debug_recordings_handler.cc

@@ -17,7 +17,7 @@
 #include "content/public/browser/browser_context.h"
 #include "content/public/browser/browser_thread.h"
 #include "content/public/browser/render_process_host.h"
-#include "media/audio/audio_manager.h"
+#include "media/audio/audio_debug_recording_session.h"
 
 using content::BrowserThread;
 
@@ -52,15 +52,10 @@ base::FilePath GetLogDirectoryAndEnsureExists(
 }  // namespace
 
 AudioDebugRecordingsHandler::AudioDebugRecordingsHandler(
-    content::BrowserContext* browser_context,
-    media::AudioManager* audio_manager)
-    : browser_context_(browser_context),
-      is_audio_debug_recordings_in_progress_(false),
-      current_audio_debug_recordings_id_(0),
-      audio_manager_(audio_manager) {
+    content::BrowserContext* browser_context)
+    : browser_context_(browser_context), current_audio_debug_recordings_id_(0) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   DCHECK(browser_context_);
-  DCHECK(audio_manager_);
 }
 
 AudioDebugRecordingsHandler::~AudioDebugRecordingsHandler() {}
@@ -102,22 +97,17 @@ void AudioDebugRecordingsHandler::DoStartAudioDebugRecordings(
     const base::FilePath& log_directory) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
 
-  if (is_audio_debug_recordings_in_progress_) {
+  if (audio_debug_recording_session_) {
     error_callback.Run("Audio debug recordings already in progress");
     return;
   }
 
-  is_audio_debug_recordings_in_progress_ = true;
   base::FilePath prefix_path = GetAudioDebugRecordingsPrefixPath(
       log_directory, ++current_audio_debug_recordings_id_);
   host->EnableAudioDebugRecordings(prefix_path);
 
-  // AudioManager is deleted on the audio thread, and the AudioManager outlives
-  // this object, which is owned by content::RenderProcessHost, so it's safe to
-  // post unretained.
-  audio_manager_->GetTaskRunner()->PostTask(
-      FROM_HERE, base::BindOnce(&media::AudioManager::EnableDebugRecording,
-                                base::Unretained(audio_manager_), prefix_path));
+  audio_debug_recording_session_ =
+      media::AudioDebugRecordingSession::Create(prefix_path);
 
   if (delay.is_zero()) {
     const bool is_stopped = false, is_manual_stop = false;
@@ -158,21 +148,15 @@ void AudioDebugRecordingsHandler::DoStopAudioDebugRecordings(
     return;
   }
 
-  if (!is_audio_debug_recordings_in_progress_) {
+  if (!audio_debug_recording_session_) {
     error_callback.Run("No audio debug recording in progress");
     return;
   }
 
-  // AudioManager is deleted on the audio thread, and the AudioManager outlives
-  // this object, which is owned by content::RenderProcessHost, so it's safe to
-  // post unretained.
-  audio_manager_->GetTaskRunner()->PostTask(
-      FROM_HERE, base::BindOnce(&media::AudioManager::DisableDebugRecording,
-                                base::Unretained(audio_manager_)));
+  audio_debug_recording_session_.reset();
 
   host->DisableAudioDebugRecordings();
 
-  is_audio_debug_recordings_in_progress_ = false;
   const bool is_stopped = true;
   callback.Run(prefix_path.AsUTF8Unsafe(), is_stopped, is_manual_stop);
 }

chrome/browser/media/webrtc/audio_debug_recordings_handler.h

@@ -8,6 +8,7 @@
 #include <stddef.h>
 #include <stdint.h>
 
+#include <memory>
 #include <string>
 
 #include "base/callback.h"
@@ -21,7 +22,7 @@ class RenderProcessHost;
 }
 
 namespace media {
-class AudioManager;
+class AudioDebugRecordingSession;
 }
 
 // AudioDebugRecordingsHandler provides an interface to start and stop
@@ -37,8 +38,8 @@ class AudioDebugRecordingsHandler
   // Key used to attach the handler to the RenderProcessHost
   static const char kAudioDebugRecordingsHandlerKey[];
 
-  AudioDebugRecordingsHandler(content::BrowserContext* browser_context,
-                              media::AudioManager* audio_manager);
+  explicit AudioDebugRecordingsHandler(
+      content::BrowserContext* browser_context);
 
   // Starts an audio debug recording. The recording lasts the given |delay|,
   // unless |delay| is zero, in which case recording will continue until
@@ -82,14 +83,12 @@ class AudioDebugRecordingsHandler
   // The browser context associated with our renderer process.
   content::BrowserContext* const browser_context_;
 
-  // Set if recordings are in progress.
-  bool is_audio_debug_recordings_in_progress_;
-
   // This counter allows saving each debug recording in separate files.
   uint64_t current_audio_debug_recordings_id_;
 
-  // Audio manager, used for enabling output recordings.
-  media::AudioManager* const audio_manager_;
+  // Used for controlling debug recordings.
+  std::unique_ptr<media::AudioDebugRecordingSession>
+      audio_debug_recording_session_;
 
   DISALLOW_COPY_AND_ASSIGN(AudioDebugRecordingsHandler);
 };

content/browser/webrtc/webrtc_internals.cc

@@ -20,6 +20,7 @@
 #include "content/public/browser/web_contents.h"
 #include "content/public/common/service_manager_connection.h"
 #include "ipc/ipc_platform_file.h"
+#include "media/audio/audio_debug_recording_session.h"
 #include "media/audio/audio_manager.h"
 #include "media/media_features.h"
 #include "services/device/public/interfaces/constants.mojom.h"
@@ -87,7 +88,6 @@ WebRTCInternals::WebRTCInternals() : WebRTCInternals(500, true) {}
 WebRTCInternals::WebRTCInternals(int aggregate_updates_ms,
                                  bool should_block_power_saving)
     : selection_type_(SelectionType::kAudioDebugRecordings),
-      audio_debug_recordings_(false),
       event_log_recordings_(false),
       num_open_connections_(0),
       should_block_power_saving_(should_block_power_saving),
@@ -318,10 +318,9 @@ void WebRTCInternals::EnableAudioDebugRecordings(
 void WebRTCInternals::DisableAudioDebugRecordings() {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
 #if BUILDFLAG(ENABLE_WEBRTC)
-  if (!audio_debug_recordings_)
+  if (!audio_debug_recording_session_)
     return;
-
-  audio_debug_recordings_ = false;
+  audio_debug_recording_session_.reset();
 
   // Tear down the dialog since the user has unchecked the audio debug
   // recordings box.
@@ -332,21 +331,12 @@ void WebRTCInternals::DisableAudioDebugRecordings() {
        !i.IsAtEnd(); i.Advance()) {
     i.GetCurrentValue()->DisableAudioDebugRecordings();
   }
-
-  // It's safe to get the AudioManager pointer here. That pointer is invalidated
-  // on the UI thread, which we're on.
-  // AudioManager is deleted on the audio thread, and the AudioManager outlives
-  // this object, so it's safe to post unretained to the audio thread.
-  media::AudioManager* audio_manager = media::AudioManager::Get();
-  audio_manager->GetTaskRunner()->PostTask(
-      FROM_HERE, base::BindOnce(&media::AudioManager::DisableDebugRecording,
-                                base::Unretained(audio_manager)));
 #endif
 }
 
 bool WebRTCInternals::IsAudioDebugRecordingsEnabled() const {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
-  return audio_debug_recordings_;
+  return !!audio_debug_recording_session_;
 }
 
 const base::FilePath& WebRTCInternals::GetAudioDebugRecordingsFilePath() const {
@@ -517,8 +507,9 @@ void WebRTCInternals::OnRendererExit(int render_process_id) {
 #if BUILDFLAG(ENABLE_WEBRTC)
 void WebRTCInternals::EnableAudioDebugRecordingsOnAllRenderProcessHosts() {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
-
-  audio_debug_recordings_ = true;
+  DCHECK(!audio_debug_recording_session_);
+  audio_debug_recording_session_ = media::AudioDebugRecordingSession::Create(
+      audio_debug_recordings_file_path_);
 
   for (RenderProcessHost::iterator i(
            content::RenderProcessHost::AllHostsIterator());
@@ -526,16 +517,6 @@ void WebRTCInternals::EnableAudioDebugRecordingsOnAllRenderProcessHosts() {
     i.GetCurrentValue()->EnableAudioDebugRecordings(
         audio_debug_recordings_file_path_);
   }
-
-  // It's safe to get the AudioManager pointer here. That pointer is invalidated
-  // on the UI thread, which we're on.
-  // AudioManager is deleted on the audio thread, and the AudioManager outlives
-  // this object, so it's safe to post unretained to the audio thread.
-  media::AudioManager* audio_manager = media::AudioManager::Get();
-  audio_manager->GetTaskRunner()->PostTask(
-      FROM_HERE, base::BindOnce(&media::AudioManager::EnableDebugRecording,
-                                base::Unretained(audio_manager),
-                                audio_debug_recordings_file_path_));
 }
 #endif
 

content/browser/webrtc/webrtc_internals.h

@@ -23,6 +23,10 @@
 #include "services/device/public/interfaces/wake_lock.mojom.h"
 #include "ui/shell_dialogs/select_file_dialog.h"
 
+namespace media {
+class AudioDebugRecordingSession;
+}
+
 namespace content {
 
 class WebContents;
@@ -218,8 +222,9 @@ class CONTENT_EXPORT WebRTCInternals : public RenderProcessHostObserver,
   } selection_type_;
 
   // Diagnostic audio recording state.
-  bool audio_debug_recordings_;
   base::FilePath audio_debug_recordings_file_path_;
+  std::unique_ptr<media::AudioDebugRecordingSession>
+      audio_debug_recording_session_;
 
   // Diagnostic event log recording state.
   bool event_log_recordings_;

media/audio/BUILD.gn

@@ -73,6 +73,10 @@ source_set("audio") {
     "audio_debug_recording_helper.h",
     "audio_debug_recording_manager.cc",
     "audio_debug_recording_manager.h",
+    "audio_debug_recording_session.cc",
+    "audio_debug_recording_session.h",
+    "audio_debug_recording_session_impl.cc",
+    "audio_debug_recording_session_impl.h",
     "audio_device_description.cc",
     "audio_device_description.h",
     "audio_device_name.cc",
@@ -369,6 +373,7 @@ source_set("unit_tests") {
     "audio_debug_file_writer_unittest.cc",
     "audio_debug_recording_helper_unittest.cc",
     "audio_debug_recording_manager_unittest.cc",
+    "audio_debug_recording_session_impl_unittest.cc",
     "audio_input_controller_unittest.cc",
     "audio_input_device_unittest.cc",
     "audio_input_unittest.cc",

media/audio/audio_debug_recording_session.cc

+// Copyright 2018 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 "media/audio/audio_debug_recording_session.h"
+
+#include "base/files/file_path.h"
+#include "media/audio/audio_debug_recording_session_impl.h"
+#include "media/audio/audio_manager.h"
+
+namespace media {
+
+AudioDebugRecordingSession::~AudioDebugRecordingSession() {}
+
+std::unique_ptr<AudioDebugRecordingSession> AudioDebugRecordingSession::Create(
+    const base::FilePath& debug_recording_file_path) {
+  return std::make_unique<AudioDebugRecordingSessionImpl>(
+      debug_recording_file_path);
+}
+
+}  // namespace media

media/audio/audio_debug_recording_session.h

+// Copyright 2018 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.
+
+#ifndef MEDIA_AUDIO_AUDIO_DEBUG_RECORDING_SESSION_H_
+#define MEDIA_AUDIO_AUDIO_DEBUG_RECORDING_SESSION_H_
+
+#include <memory>
+
+#include "base/macros.h"
+#include "media/base/media_export.h"
+
+namespace base {
+class FilePath;
+}
+
+namespace media {
+
+// Creating/Destroying an AudioDebugRecordingSession object enables/disables
+// audio debug recording.
+class MEDIA_EXPORT AudioDebugRecordingSession {
+ public:
+  virtual ~AudioDebugRecordingSession();
+  static std::unique_ptr<AudioDebugRecordingSession> Create(
+      const base::FilePath& debug_recording_file_path);
+
+ protected:
+  AudioDebugRecordingSession() = default;
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(AudioDebugRecordingSession);
+};
+
+}  // namespace media
+
+#endif  // MEDIA_AUDIO_AUDIO_DEBUG_RECORDING_SESSION_H_

media/audio/audio_debug_recording_session_impl.cc

+// Copyright 2018 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 "media/audio/audio_debug_recording_session_impl.h"
+
+#include "base/bind.h"
+#include "base/files/file_path.h"
+#include "base/single_thread_task_runner.h"
+#include "media/audio/audio_manager.h"
+
+namespace media {
+
+// Posting AudioManager::Get() as unretained is safe because
+// AudioManager::Shutdown() (called before AudioManager destruction) shuts down
+// AudioManager thread.
+
+AudioDebugRecordingSessionImpl::AudioDebugRecordingSessionImpl(
+    const base::FilePath& debug_recording_file_path) {
+  AudioManager* audio_manager = AudioManager::Get();
+  if (audio_manager == nullptr)
+    return;
+
+  audio_manager->GetTaskRunner()->PostTask(
+      FROM_HERE, base::BindOnce(&AudioManager::EnableDebugRecording,
+                                base::Unretained(audio_manager),
+                                debug_recording_file_path));
+}
+
+AudioDebugRecordingSessionImpl::~AudioDebugRecordingSessionImpl() {
+  AudioManager* audio_manager = AudioManager::Get();
+  if (audio_manager == nullptr)
+    return;
+
+  audio_manager->GetTaskRunner()->PostTask(
+      FROM_HERE, base::BindOnce(&AudioManager::DisableDebugRecording,
+                                base::Unretained(audio_manager)));
+}
+
+}  // namespace media

media/audio/audio_debug_recording_session_impl.h

+// Copyright 2018 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.
+
+#ifndef MEDIA_AUDIO_AUDIO_DEBUG_RECORDING_SESSION_IMPL_H_
+#define MEDIA_AUDIO_AUDIO_DEBUG_RECORDING_SESSION_IMPL_H_
+
+#include "media/audio/audio_debug_recording_session.h"
+#include "media/base/media_export.h"
+
+namespace base {
+class FilePath;
+}
+
+namespace media {
+
+class MEDIA_EXPORT AudioDebugRecordingSessionImpl
+    : public AudioDebugRecordingSession {
+ public:
+  explicit AudioDebugRecordingSessionImpl(
+      const base::FilePath& debug_recording_file_path);
+  ~AudioDebugRecordingSessionImpl() override;
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(AudioDebugRecordingSessionImpl);
+};
+
+}  // namespace media
+
+#endif  // MEDIA_AUDIO_AUDIO_DEBUG_RECORDING_SESSION_IMPL_H_

media/audio/audio_debug_recording_session_impl_unittest.cc

+// Copyright 2018 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 "media/audio/audio_debug_recording_session_impl.h"
+
+#include "base/test/scoped_task_environment.h"
+#include "media/audio/mock_audio_manager.h"
+#include "media/audio/test_audio_thread.h"
+#include "testing/gmock/include/gmock/gmock.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace media {
+
+namespace {
+const base::FilePath::CharType kFilePath[] = FILE_PATH_LITERAL("file_path");
+}
+
+class AudioDebugRecordingSessionImplTest : public testing::Test {
+ public:
+  void CreateAudioManager() {
+    mock_audio_manager_ =
+        std::make_unique<MockAudioManager>(std::make_unique<TestAudioThread>());
+    ASSERT_NE(nullptr, AudioManager::Get());
+    ASSERT_EQ(static_cast<AudioManager*>(mock_audio_manager_.get()),
+              AudioManager::Get());
+  }
+
+  void ShutdownAudioManager() { ASSERT_TRUE(mock_audio_manager_->Shutdown()); }
+
+  void CreateDebugRecordingSession() {
+    audio_debug_recording_session_impl_ =
+        std::make_unique<media::AudioDebugRecordingSessionImpl>(file_path_);
+  }
+
+  void DestroyDebugRecordingSession() {
+    audio_debug_recording_session_impl_.reset();
+  }
+
+ protected:
+  base::test::ScopedTaskEnvironment scoped_task_environment_;
+  std::unique_ptr<MockAudioManager> mock_audio_manager_;
+  std::unique_ptr<AudioDebugRecordingSessionImpl>
+      audio_debug_recording_session_impl_;
+  base::FilePath file_path_ = base::FilePath(kFilePath);
+};
+
+TEST_F(AudioDebugRecordingSessionImplTest,
+       ConstructorEnablesAndDestructorDisablesDebugRecordingOnAudioManager) {
+  ::testing::InSequence seq;
+
+  CreateAudioManager();
+  EXPECT_CALL(*mock_audio_manager_, EnableDebugRecording(file_path_));
+  CreateDebugRecordingSession();
+
+  EXPECT_CALL(*mock_audio_manager_, DisableDebugRecording());
+  DestroyDebugRecordingSession();
+
+  ShutdownAudioManager();
+}
+
+TEST_F(AudioDebugRecordingSessionImplTest,
+       CreateDestroySessionDontCrashWithNoAudioManager) {
+  ASSERT_EQ(nullptr, AudioManager::Get());
+  CreateDebugRecordingSession();
+  DestroyDebugRecordingSession();
+}
+
+}  // namespace media

media/audio/mock_audio_manager.cc

@@ -126,11 +126,6 @@ std::unique_ptr<AudioLog> MockAudioManager::CreateAudioLog(
 
 void MockAudioManager::InitializeDebugRecording() {}
 
-void MockAudioManager::EnableDebugRecording(
-    const base::FilePath& base_file_name) {}
-
-void MockAudioManager::DisableDebugRecording() {}
-
 const char* MockAudioManager::GetName() {
   return nullptr;
 }

media/audio/mock_audio_manager.h

@@ -9,10 +9,12 @@
 #include <string>
 
 #include "base/callback_forward.h"
+#include "base/files/file_path.h"
 #include "base/macros.h"
 #include "base/sequenced_task_runner_helpers.h"
 #include "base/single_thread_task_runner.h"
 #include "media/audio/audio_manager.h"
+#include "testing/gmock/include/gmock/gmock.h"
 
 namespace media {
 
@@ -59,8 +61,9 @@ class MockAudioManager : public AudioManager {
       AudioLogFactory::AudioComponent component) override;
 
   void InitializeDebugRecording() override;
-  void EnableDebugRecording(const base::FilePath& base_file_name) override;
-  void DisableDebugRecording() override;
+  MOCK_METHOD1(EnableDebugRecording,
+               void(const base::FilePath& base_file_name));
+  MOCK_METHOD0(DisableDebugRecording, void());
 
   const char* GetName() override;