AudioRendererSinkCache: Do not use unhealthy sinks

content/renderer/media/audio_renderer_sink_cache_impl.cc

 // Copyright 2016 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 <algorithm>
 
 #include "base/bind.h"
 #include "base/location.h"
 #include "base/memory/ptr_util.h"
 #include "base/metrics/histogram_macros.h"
@@ skipping 19 matching lines @@
   // cache a new sink.
   SINK_CACHE_MISS_CANNOT_LOOKUP_BY_SESSION_ID = 1,
 
   // Output parmeters for an already-cached sink are requested.
   SINK_CACHE_HIT = 2,
 
   // For UMA.
   SINK_CACHE_LAST_ENTRY
 };
 
+bool SinkIsHealthy(media::AudioRendererSink* sink) {
+  return sink->GetOutputDeviceInfo().device_status() ==
+         media::OUTPUT_DEVICE_STATUS_OK;
+}
+
 }  // namespace
 
 // Cached sink data.
 struct AudioRendererSinkCacheImpl::CacheEntry {
   int source_render_frame_id;
   std::string device_id;
   url::Origin security_origin;
   scoped_refptr<media::AudioRendererSink> sink;  // Sink instance
   bool used;                                     // True if in use by a client.
 };
@@ skipping 24 matching lines @@
   // is being destroyed anyways.
   for (auto& entry : cache_)
     entry.sink->Stop();
 }
 
 media::OutputDeviceInfo AudioRendererSinkCacheImpl::GetSinkInfo(
     int source_render_frame_id,
     int session_id,
     const std::string& device_id,
     const url::Origin& security_origin) {
-  CacheEntry cache_entry = {source_render_frame_id,
-                            std::string() /* device_id */, security_origin,
-                            nullptr /* sink */, false /* not used */};
+  scoped_refptr<media::AudioRendererSink> sink(nullptr);

[Guido Urdaneta, 2016/11/25 22:13:17]

nit: use default constructor? (up to you)

[o1ka, 2016/11/30 15:15:03]

Done.

 
   if (media::AudioDeviceDescription::UseSessionIdToSelectDevice(session_id,
                                                                 device_id)) {
     // We are provided with session id instead of device id. Session id is
     // unique, so we can't find any matching sink. Creating a new one.
-    cache_entry.sink = create_sink_cb_.Run(source_render_frame_id, session_id,
-                                           device_id, security_origin);
-    cache_entry.device_id = cache_entry.sink->GetOutputDeviceInfo().device_id();
+    sink = create_sink_cb_.Run(source_render_frame_id, session_id, device_id,
+                               security_origin);
 
-    DVLOG(1) << "GetSinkInfo: address: " << cache_entry.sink.get()
-             << " - used session to create new sink.";
-
-    // Cache a newly-created sink.
-    base::AutoLock auto_lock(cache_lock_);
-    cache_.push_back(cache_entry);
+    CacheUnusedSinkIfHealthy(source_render_frame_id,
+                             sink->GetOutputDeviceInfo().device_id(),
+                             security_origin, sink);
     UMA_HISTOGRAM_ENUMERATION(
         "Media.Audio.Render.SinkCache.GetOutputDeviceInfoCacheUtilization",
         SINK_CACHE_MISS_CANNOT_LOOKUP_BY_SESSION_ID, SINK_CACHE_LAST_ENTRY);
 
   } else {
     // Ignore session id.
-    base::AutoLock auto_lock(cache_lock_);
-
-    auto cache_iter =
-        FindCacheEntry_Locked(source_render_frame_id, device_id,
-                              security_origin, false /* unused_only */);
-
-    if (cache_iter != cache_.end()) {
-      // A matching cached sink is found.
-      DVLOG(1) << "GetSinkInfo: address: " << cache_iter->sink.get()
-               << " - reused a cached sink.";
-
-      UMA_HISTOGRAM_ENUMERATION(
-          "Media.Audio.Render.SinkCache.GetOutputDeviceInfoCacheUtilization",
-          SINK_CACHE_HIT, SINK_CACHE_LAST_ENTRY);
-      return cache_iter->sink->GetOutputDeviceInfo();
+    {
+      base::AutoLock auto_lock(cache_lock_);
+      auto cache_iter = FindHealthyCacheEntry_Locked(source_render_frame_id,

[Guido Urdaneta, 2016/11/25 22:13:17]

nit: if only healthy entries are cached there is no need to change the name of
the function. Otherwise the name suggests that it might be possible to find
unhealthy entries.

[o1ka, 2016/11/30 15:15:04]

Changed so that the cached sinks are always healthy.

+                                                     device_id, security_origin,
+                                                     false /* unused_only */);
+      if (cache_iter != cache_.end()) {
+        // A matching cached sink is found.
+        UMA_HISTOGRAM_ENUMERATION(
+            "Media.Audio.Render.SinkCache.GetOutputDeviceInfoCacheUtilization",
+            SINK_CACHE_HIT, SINK_CACHE_LAST_ENTRY);
+        return cache_iter->sink->GetOutputDeviceInfo();
+      }
     }
 
     // No matching sink found, create a new one.
-    cache_entry.device_id = device_id;
-    cache_entry.sink = create_sink_cb_.Run(
-        source_render_frame_id, 0 /* session_id */, device_id, security_origin);
-
-    DVLOG(1) << "GetSinkInfo: address: " << cache_entry.sink.get()
-             << " - no matching cached sink found, created a new one.";
-
-    // Cache a newly-created sink.
-    cache_.push_back(cache_entry);
+    sink = create_sink_cb_.Run(source_render_frame_id, 0 /* session_id */,
+                               device_id, security_origin);
+    CacheUnusedSinkIfHealthy(source_render_frame_id, device_id, security_origin,
+                             sink);
     UMA_HISTOGRAM_ENUMERATION(
         "Media.Audio.Render.SinkCache.GetOutputDeviceInfoCacheUtilization",
         SINK_CACHE_MISS_NO_SINK, SINK_CACHE_LAST_ENTRY);
   }
 
-  // Schedule it for deletion.
-  DeleteLaterIfUnused(cache_entry.sink.get());
-
-  DVLOG(1) << "GetSinkInfo: address: " << cache_entry.sink.get()
-           << " created. source_render_frame_id: " << source_render_frame_id
-           << " session_id: " << session_id << " device_id: " << device_id
-           << " security_origin: " << security_origin;
-
-  return cache_entry.sink->GetOutputDeviceInfo();
+  //|sink| is ref-counted, so it's ok if it is grabage-collected before we get

[Guido Urdaneta, 2016/11/25 22:13:17]

typo: grabage -> garbage.
I think the comment is wrong. How can |sink| be garbage collected if |sink|
still holds a reference to it?
Perhaps you mean evicted from the cache?

[o1ka, 2016/11/30 15:15:04]

Done.

+  // here.
+  return sink->GetOutputDeviceInfo();
 }
 
 scoped_refptr<media::AudioRendererSink> AudioRendererSinkCacheImpl::GetSink(
     int source_render_frame_id,
     const std::string& device_id,
     const url::Origin& security_origin) {
   UMA_HISTOGRAM_BOOLEAN("Media.Audio.Render.SinkCache.UsedForSinkCreation",
                         true);
 
   base::AutoLock auto_lock(cache_lock_);
 
-  auto cache_iter =
-      FindCacheEntry_Locked(source_render_frame_id, device_id, security_origin,
-                            true /* unused sink only */);
+  auto cache_iter = FindHealthyCacheEntry_Locked(source_render_frame_id,
+                                                 device_id, security_origin,
+                                                 true /* unused sink only */);
 
   if (cache_iter != cache_.end()) {
     // Found unused sink; mark it as used and return.
-    DVLOG(1) << "GetSink: address: " << cache_iter->sink.get()
-             << " - found unused cached sink, reusing it.";
-
     cache_iter->used = true;
     UMA_HISTOGRAM_BOOLEAN(
         "Media.Audio.Render.SinkCache.InfoSinkReusedForOutput", true);
     return cache_iter->sink;
   }
 
   // No unused sink is found, create one, mark it used, cache it and return.
   CacheEntry cache_entry = {
       source_render_frame_id, device_id, security_origin,
       create_sink_cb_.Run(source_render_frame_id, 0 /* session_id */, device_id,
                           security_origin),
       true /* used */};
-
   cache_.push_back(cache_entry);
-
-  DVLOG(1) << "GetSink: address: " << cache_entry.sink.get()
-           << " - no unused cached sink found, created a new one."
-           << " source_render_frame_id: " << source_render_frame_id
-           << " device_id: " << device_id
-           << " security_origin: " << security_origin;
   return cache_entry.sink;
 }
 
 void AudioRendererSinkCacheImpl::ReleaseSink(
     const media::AudioRendererSink* sink_ptr) {
   // We don't know the sink state, so won't reuse it. Delete it immediately.
   DeleteSink(sink_ptr, true);
 }
 
 void AudioRendererSinkCacheImpl::DeleteLaterIfUnused(
@@ skipping 19 matching lines @@
                                    [sink_ptr](const CacheEntry& val) {
                                      return val.sink.get() == sink_ptr;
                                    });
 
     if (cache_iter == cache_.end()) {
       // If |force_delete_used| is not set it means the sink scheduled for
       // deletion got aquired and released before scheduled deletion - it's ok.
       DCHECK(!force_delete_used)
           << "DeleteSink: address: " << sink_ptr
           << " could not find a sink which is supposed to be in use";
-
-      DVLOG(1) << "DeleteSink: address: " << sink_ptr
-               << " force_delete_used = false - already deleted.";
       return;
     }
 
     // When |force_delete_used| is set, it's expected that we are deleting a
     // used sink.
     DCHECK((!force_delete_used) || (force_delete_used && cache_iter->used))
         << "Attempt to delete a non-aquired sink.";
 
-    if (!force_delete_used && cache_iter->used) {
-      DVLOG(1) << "DeleteSink: address: " << sink_ptr
-               << " sink in use, skipping deletion.";
+    if (!force_delete_used && cache_iter->used)
       return;
-    }
 
     // To stop the sink before deletion if it's not used, we need to hold
     // a ref to it.
     if (!cache_iter->used) {
       sink_to_stop = cache_iter->sink;
       UMA_HISTOGRAM_BOOLEAN(
           "Media.Audio.Render.SinkCache.InfoSinkReusedForOutput", false);
     }
 
     cache_.erase(cache_iter);
-    DVLOG(1) << "DeleteSink: address: " << sink_ptr;
   }  // Lock scope;
 
   // Stop the sink out of the lock scope.
   if (sink_to_stop.get()) {
     DCHECK_EQ(sink_ptr, sink_to_stop.get());
     sink_to_stop->Stop();
-    DVLOG(1) << "DeleteSink: address: " << sink_ptr << " stopped.";
   }
 }
 
 AudioRendererSinkCacheImpl::CacheContainer::iterator
-AudioRendererSinkCacheImpl::FindCacheEntry_Locked(
+AudioRendererSinkCacheImpl::FindHealthyCacheEntry_Locked(
     int source_render_frame_id,
     const std::string& device_id,
     const url::Origin& security_origin,
     bool unused_only) {
   return std::find_if(
       cache_.begin(), cache_.end(),
       [source_render_frame_id, &device_id, &security_origin,
        unused_only](const CacheEntry& val) {
         if (val.used && unused_only)
           return false;
         if (val.source_render_frame_id != source_render_frame_id)
           return false;
         if (media::AudioDeviceDescription::IsDefaultDevice(device_id) &&
             media::AudioDeviceDescription::IsDefaultDevice(val.device_id)) {
           // Both device IDs represent the same default device => do not compare
           // them; the default device is always authorized => ignore security
           // origin.
-          return true;
+          return SinkIsHealthy(val.sink.get());

[Guido Urdaneta, 2016/11/25 22:13:17]

Can a healthy sink become unhealthy? If not, why check?

[DaleCurtis, 2016/11/28 19:00:37]

Yes, OnError() may be fired by the browser side.

[o1ka, 2016/11/30 15:15:03]

It does not change device status though. We don't do anything about a sink on
error, just call an error callback:
https://cs.chromium.org/chromium/src/media/audio/audio_output_device.cc?q=Aud....

         }
         return val.device_id == device_id &&
-               val.security_origin == security_origin;
+               val.security_origin == security_origin &&
+               SinkIsHealthy(val.sink.get());

[Guido Urdaneta, 2016/11/25 22:13:17]

ditto

[o1ka, 2016/11/30 15:15:04]

Done.

       });
 };
 
+void AudioRendererSinkCacheImpl::CacheUnusedSinkIfHealthy(
+    int source_render_frame_id,
+    const std::string& device_id,
+    const url::Origin& security_origin,
+    scoped_refptr<media::AudioRendererSink> sink) {
+  if (!SinkIsHealthy(sink.get())) {
+    UMA_HISTOGRAM_BOOLEAN("Media.Audio.Render.SinkCache.HealthySinkCached",
+                          false);
+    return;
+  }
+
+  CacheEntry cache_entry = {source_render_frame_id, device_id, security_origin,
+                            sink, false /* not used */};
+
+  {
+    base::AutoLock auto_lock(cache_lock_);
+    cache_.push_back(cache_entry);
+  }
+
+  UMA_HISTOGRAM_BOOLEAN("Media.Audio.Render.SinkCache.HealthySinkCached", true);
+  DeleteLaterIfUnused(cache_entry.sink.get());
+}
+
 int AudioRendererSinkCacheImpl::GetCacheSizeForTesting() {
   return cache_.size();
 }
 
 }  // namespace content