fallback to null sink in WebAudioSourceProvider::Initialize() + UMA stats for device status

media/blink/webmediaplayer_impl.cc

 // Copyright 2013 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/blink/webmediaplayer_impl.h"
 
 #include <algorithm>
 #include <cmath>
 #include <limits>
 #include <string>
@@ skipping 247 matching lines @@
 
   media_log_->AddEvent(
       media_log_->CreateEvent(MediaLogEvent::WEBMEDIAPLAYER_CREATED));
 
   if (params.initial_cdm())
     SetCdm(params.initial_cdm());
 
   // TODO(xhwang): When we use an external Renderer, many methods won't work,
   // e.g. GetCurrentFrameFromCompositor(). See http://crbug.com/434861
 
+  if (params.audio_renderer_sink().get()) {

[Guido Urdaneta, 2016/11/24 17:30:09]

nit: Should a stat be collected if params.audio_renderer_sink() is null?

+    media::OutputDeviceStatus device_status =
+        params.audio_renderer_sink()->GetOutputDeviceInfo().device_status();

[DaleCurtis, 2016/11/28 18:58:17]

In retrospect we probably shouldn't do this since it blocks the main render
thread. Instead we shouldn't construct the audio_source_provider_ until we know
if the sink is any good and that determination should be made on another thread.

This is no worse than we've had in the past, but guidou@ did a lot of work to
avoid us having this code run on the main thread, it's a shame to put it back.

[o1ka, 2016/11/29 13:55:58]

It's not new in this CL, we have already been doing it at l.272 (It's a change
Guido introduced recently to fallback to null sink)

[DaleCurtis, 2016/11/29 20:32:02]

I know, sorry I wasn't clear, I was saying it's not great that we added it in
the previous CL.

[o1ka, 2016/11/30 16:13:47]

Ah I see. We can probably defer fallback until
WebAudioSourceProviderImpl::Initialize, but I'm not sure how it all will work if
WebAudioSourceProviderImpl::GetOutputDeviceInfo() is called prior to
initialization.

We can remove it now, or figure out how to make it happen on media thread and
address it in a separate CL. WDYT?

+    UMA_HISTOGRAM_ENUMERATION("Media.AudioSinkStatus", device_status,

[DaleCurtis, 2016/11/28 18:58:17]

Maybe Media.WebMediaPlayer.SinkStatus?

[o1ka, 2016/11/30 16:13:48]

Done.

+                              OUTPUT_DEVICE_STATUS_MAX + 1);
+
+    if (device_status == OUTPUT_DEVICE_STATUS_OK) {
+      audio_source_provider_ =
+          new WebAudioSourceProviderImpl(params.audio_renderer_sink());
+      return;
+    }
+    LOG(WARNING) << "Output device error, falling back to null sink";

[DaleCurtis, 2016/11/28 18:58:17]

This should be a MEDIA_LOG() so it shows up in chrome://media-internals

[o1ka, 2016/11/30 16:13:47]

Done.

+  }
   // Use the null sink if no valid sink was provided.
-  audio_source_provider_ = new WebAudioSourceProviderImpl(
-      params.audio_renderer_sink().get() &&
-              params.audio_renderer_sink()
-                      ->GetOutputDeviceInfo()
-                      .device_status() == OUTPUT_DEVICE_STATUS_OK
-          ? params.audio_renderer_sink()
-          : new NullAudioSink(media_task_runner_));
+  audio_source_provider_ =
+      new WebAudioSourceProviderImpl(new NullAudioSink(media_task_runner_));
 }
 
 WebMediaPlayerImpl::~WebMediaPlayerImpl() {
   DCHECK(main_task_runner_->BelongsToCurrentThread());
 
   suppress_destruction_errors_ = true;
   if (delegate_) {
     delegate_->PlayerGone(delegate_id_);
     delegate_->RemoveObserver(delegate_id_);
   }
@@ skipping 1695 matching lines @@
     watch_time_reporter_->OnShown();
 }
 
 bool WebMediaPlayerImpl::IsHidden() const {
   DCHECK(main_task_runner_->BelongsToCurrentThread());
 
   return delegate_ && delegate_->IsHidden();
 }
 
 }  // namespace media