From 78f60ef1ef5a8f4af3317383985955da4f1d4fdb Mon Sep 17 00:00:00 2001 From: rlp Date: Thu, 23 Oct 2014 16:00:31 -0700 Subject: [PATCH] [Hotword] Delay checking for audio devices until needed. Checking for audio devices right away is causing performance regressions on Mac. This CL delays checking until needed, but does not include the availability of an audio capture device in the error reporting until after it's known the check has been completed. Otherwise, the settings page immediately displays an error upon opening. In general, this should not be a problem as typically the audio device check will have completed by the time the user goes to the settings page. There is a rare use case, where the user installs Chrome and immediately goes to settings and in that case, we won't be able to accurately report if there is an audio device preventing the user from starting hotwording. However, by the time they reload, the error should be present which should be sufficient. BUG=385514,413104 Review URL: https://codereview.chromium.org/654613004 Cr-Commit-Position: refs/heads/master@{#300965} --- chrome/browser/search/hotword_service.cc | 41 ++++++++---------------- chrome/browser/search/hotword_service.h | 8 +---- chrome/browser/search/hotword_service_factory.cc | 18 +++++------ chrome/browser/search/hotword_service_factory.h | 12 +++++++ 4 files changed, 36 insertions(+), 43 deletions(-) diff --git a/chrome/browser/search/hotword_service.cc b/chrome/browser/search/hotword_service.cc index 4a6f2c90d88f..1e9c86d5b813 100644 --- a/chrome/browser/search/hotword_service.cc +++ b/chrome/browser/search/hotword_service.cc @@ -215,10 +215,6 @@ HotwordService::HotwordService(Profile* profile) base::Bind(&HotwordService::OnHotwordSearchEnabledChanged, base::Unretained(this))); - registrar_.Add(this, - chrome::NOTIFICATION_BROWSER_WINDOW_READY, - content::NotificationService::AllSources()); - extensions::ExtensionSystem::Get(profile_)->ready().Post( FROM_HERE, base::Bind(base::IgnoreResult( @@ -236,25 +232,6 @@ HotwordService::HotwordService(Profile* profile) HotwordService::~HotwordService() { } -void HotwordService::Observe(int type, - const content::NotificationSource& source, - const content::NotificationDetails& details) { - if (type == chrome::NOTIFICATION_BROWSER_WINDOW_READY) { - // The microphone monitor must be initialized as the page is loading - // so that the state of the microphone is available when the page - // loads. The Ok Google Hotword setting will display an error if there - // is no microphone but this information will not be up-to-date unless - // the monitor had already been started. Furthermore, the pop up to - // opt in to hotwording won't be available if it thinks there is no - // microphone. There is no hard guarantee that the monitor will actually - // be up by the time it's needed, but this is the best we can do without - // starting it at start up which slows down start up too much. - // The content/media for microphone uses the same observer design and - // makes use of the same audio device monitor. - HotwordServiceFactory::GetInstance()->UpdateMicrophoneState(); - } -} - void HotwordService::OnExtensionUninstalled( content::BrowserContext* browser_context, const extensions::Extension* extension, @@ -417,10 +394,20 @@ bool HotwordService::IsServiceAvailable() { RecordErrorMetrics(error_message_); // Determine if the proper audio capabilities exist. - bool audio_capture_allowed = - profile_->GetPrefs()->GetBoolean(prefs::kAudioCaptureAllowed); - if (!audio_capture_allowed || !HotwordServiceFactory::IsMicrophoneAvailable()) - error_message_ = IDS_HOTWORD_MICROPHONE_ERROR_MESSAGE; + // The first time this is called, it probably won't return in time, but that's + // why it won't be included in the error calculation (i.e., the call to + // IsAudioDeviceStateUpdated()). However, this use case is rare and typically + // the devices will be initialized by the time a user goes to settings. + bool audio_device_state_updated = + HotwordServiceFactory::IsAudioDeviceStateUpdated(); + HotwordServiceFactory::GetInstance()->UpdateMicrophoneState(); + if (audio_device_state_updated) { + bool audio_capture_allowed = + profile_->GetPrefs()->GetBoolean(prefs::kAudioCaptureAllowed); + if (!audio_capture_allowed || + !HotwordServiceFactory::IsMicrophoneAvailable()) + error_message_ = IDS_HOTWORD_MICROPHONE_ERROR_MESSAGE; + } return (error_message_ == 0) && IsHotwordAllowed(); } diff --git a/chrome/browser/search/hotword_service.h b/chrome/browser/search/hotword_service.h index 3a70003bdbfb..79e22d968d35 100644 --- a/chrome/browser/search/hotword_service.h +++ b/chrome/browser/search/hotword_service.h @@ -32,8 +32,7 @@ extern const char kHotwordFieldTrialDisabledGroupName[]; // Provides an interface for the Hotword component that does voice triggered // search. -class HotwordService : public content::NotificationObserver, - public extensions::ExtensionRegistryObserver, +class HotwordService : public extensions::ExtensionRegistryObserver, public KeyedService { public: // Returns true if the hotword supports the current system language. @@ -45,11 +44,6 @@ class HotwordService : public content::NotificationObserver, explicit HotwordService(Profile* profile); ~HotwordService() override; - // Overridden from content::NotificationObserver: - void Observe(int type, - const content::NotificationSource& source, - const content::NotificationDetails& details) override; - // Overridden from ExtensionRegisterObserver: void OnExtensionInstalled(content::BrowserContext* browser_context, const extensions::Extension* extension, diff --git a/chrome/browser/search/hotword_service_factory.cc b/chrome/browser/search/hotword_service_factory.cc index d8a061965324..7d05057c3743 100644 --- a/chrome/browser/search/hotword_service_factory.cc +++ b/chrome/browser/search/hotword_service_factory.cc @@ -52,11 +52,17 @@ bool HotwordServiceFactory::IsMicrophoneAvailable() { return GetInstance()->microphone_available(); } +// static +bool HotwordServiceFactory::IsAudioDeviceStateUpdated() { + return GetInstance()->audio_device_state_updated(); +} + HotwordServiceFactory::HotwordServiceFactory() : BrowserContextKeyedServiceFactory( "HotwordService", BrowserContextDependencyManager::GetInstance()), - microphone_available_(false) { + microphone_available_(false), + audio_device_state_updated_(false) { // No dependencies. // Register with the device observer list to update the microphone @@ -77,19 +83,13 @@ void HotwordServiceFactory::InitializeMicrophoneObserver() { void HotwordServiceFactory::OnUpdateAudioDevices( const content::MediaStreamDevices& devices) { microphone_available_ = !devices.empty(); + audio_device_state_updated_ = true; } void HotwordServiceFactory::UpdateMicrophoneState() { // In order to trigger the monitor, just call getAudioCaptureDevices. content::MediaStreamDevices devices = - MediaCaptureDevicesDispatcher::GetInstance()->GetAudioCaptureDevices(); - - // If the monitor had not previously been started, there may be 0 devices - // even if that is not accurate. However, we can update the microphone - // availability state now. Either the number of devices will be correct or - // we know that the call above will start the monitor and the microphone - // state will be updated very soon and call OnUpdateAudioDevices. - OnUpdateAudioDevices(devices); + MediaCaptureDevicesDispatcher::GetInstance()->GetAudioCaptureDevices(); } void HotwordServiceFactory::RegisterProfilePrefs( diff --git a/chrome/browser/search/hotword_service_factory.h b/chrome/browser/search/hotword_service_factory.h index 44b655d26a87..74665ec7ea15 100644 --- a/chrome/browser/search/hotword_service_factory.h +++ b/chrome/browser/search/hotword_service_factory.h @@ -35,6 +35,11 @@ class HotwordServiceFactory : public MediaCaptureDevicesDispatcher::Observer, // is browser (not profile) specific, it resides in the factory. static bool IsMicrophoneAvailable(); + // Returns whether the state of the audio devices has been updated. + // Essentially it indicates the validity of the return value from + // IsMicrophoneAvailable(). + static bool IsAudioDeviceStateUpdated(); + // Overridden from MediaCaptureDevicesDispatcher::Observer void OnUpdateAudioDevices( const content::MediaStreamDevices& devices) override; @@ -67,6 +72,13 @@ class HotwordServiceFactory : public MediaCaptureDevicesDispatcher::Observer, bool microphone_available_; + // Indicates if the check for audio devices has been run such that it can be + // included in the error checking. Audio checking is not done immediately + // upon start up because of the negative impact on performance. + bool audio_device_state_updated_; + + bool audio_device_state_updated() { return audio_device_state_updated_; } + DISALLOW_COPY_AND_ASSIGN(HotwordServiceFactory); }; -- 2.11.4.GIT