Unmute Tab Audio For Desktop Share

content/browser/media/capture/audio_mirroring_manager.cc

 // Copyright (c) 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 "content/browser/media/capture/audio_mirroring_manager.h"
 
 #include <algorithm>
 
 #include "base/bind.h"
 #include "base/bind_helpers.h"
@@ skipping 63 matching lines @@
   DCHECK(thread_checker_.CalledOnValidThread());
   DCHECK(destination);
 
   // Insert an entry into the set of active mirroring sessions, if this is a
   // previously-unknown destination.
   if (std::find(sessions_.begin(), sessions_.end(), destination) ==
           sessions_.end()) {
     sessions_.push_back(destination);
   }
 
+  std::set<SourceFrameRef> candidates;
+
   // Query the MirroringDestination to see which of the audio streams should be
   // diverted.
-  std::set<SourceFrameRef> candidates;
   for (StreamRoutes::const_iterator it = routes_.begin(); it != routes_.end();
        ++it) {
-    if (!it->destination || it->destination == destination)
       candidates.insert(it->source_render_frame);

[qiangchen, 2016/05/03 16:58:23]

Can you double check whether removing the "if" condition here is ok or not?

As I found in UpdateRoutesToDivertDestination, there is a similar check to not
divert an already-diverting stream, so I think my change is harmless here.

If you think this may cause issues, I can change it back. But in that case, we
must have the IsDuplication method in MirroringDestination. Because for
duplication case, there is no way we can do a pre-screening here. 

   }
   if (!candidates.empty()) {
     destination->QueryForMatches(
         candidates,
         base::Bind(&AudioMirroringManager::UpdateRoutesToDestination,
                    base::Unretained(this),
                    destination,
                    false));
   }
 }
 
 void AudioMirroringManager::StopMirroring(MirroringDestination* destination) {
   DCHECK(thread_checker_.CalledOnValidThread());
 
-  // Stop diverting each audio stream in the mirroring session being stopped.
-  // Each stopped stream becomes a candidate to be diverted to another
-  // destination.
-  std::set<SourceFrameRef> redivert_candidates;
-  for (StreamRoutes::iterator it = routes_.begin(); it != routes_.end(); ++it) {
-    if (it->destination == destination) {
-      ChangeRoute(&(*it), NULL);
-      redivert_candidates.insert(it->source_render_frame);
-    }
-  }
-  if (!redivert_candidates.empty())
-    InitiateQueriesToFindNewDestination(destination, redivert_candidates);
+  std::set<SourceFrameRef> matches;
+  destination->QueryForMatches(

[miu, 2016/05/02 20:06:12]

This won't work, since |destination| is allowed to be destroyed once this method
returns. |destination| may simply punt on the QueryForMatches() call if it has
to hop threads to execute it.

So, the code that was here was correct for "diverted" audio flows. We want them
re-diverted if other destinations want them. So, please keep that. For the
duplicated audio use case, you just want to stop a duplication (no need to
re-query). Therefore, you should modify the for-loop to:

  for (...same as before...) {
    if (it->destination == destination) {
      ...same as before...
    }
    auto dup_it = it->dup_destinations.find(destination);
    if (dup_it != it->dup_destinations.end()) {
      it->diverter->StopDuplicating((*dup_it)[destination]);
      it->dup_destinations.erase(dup_it);
    }
  }

[qiangchen, 2016/05/03 16:58:22]

Done.

+      matches, base::Bind(&AudioMirroringManager::UpdateRoutesToDestination,
+                          base::Unretained(this), destination, false));
 
   // Remove the entry from the set of active mirroring sessions.
   const Destinations::iterator dest_it =
       std::find(sessions_.begin(), sessions_.end(), destination);
   if (dest_it == sessions_.end()) {
     NOTREACHED();
     return;
   }
   sessions_.erase(dest_it);
 }
 
 void AudioMirroringManager::InitiateQueriesToFindNewDestination(
     MirroringDestination* old_destination,
     const std::set<SourceFrameRef>& candidates) {
   DCHECK(thread_checker_.CalledOnValidThread());
 
   for (Destinations::const_iterator it = sessions_.begin();
        it != sessions_.end(); ++it) {
-    if (*it != old_destination) {
-      (*it)->QueryForMatches(
-          candidates,
-          base::Bind(&AudioMirroringManager::UpdateRoutesToDestination,
-                     base::Unretained(this),
-                     *it,
-                     true));
-    }
+    if (*it == old_destination)
+      continue;
+
+    (*it)->QueryForMatches(
+        candidates,
+        base::Bind(&AudioMirroringManager::UpdateRoutesToDestination,
+                   base::Unretained(this), *it, true));
   }
 }
 
 void AudioMirroringManager::UpdateRoutesToDestination(
     MirroringDestination* destination,
     bool add_only,
+    const std::set<SourceFrameRef>& matches,
+    bool is_duplicate) {
+  if (is_duplicate)
+    UpdateRoutesToDuplicateDestination(destination, add_only, matches);
+  else
+    UpdateRoutesToDivertDestination(destination, add_only, matches);
+}
+
+void AudioMirroringManager::UpdateRoutesToDivertDestination(
+    MirroringDestination* destination,
+    bool add_only,
     const std::set<SourceFrameRef>& matches) {
   DCHECK(thread_checker_.CalledOnValidThread());
 
   if (std::find(sessions_.begin(), sessions_.end(), destination) ==
           sessions_.end()) {
     return;  // Query result callback invoked after StopMirroring().
   }
 
   DVLOG(1) << (add_only ? "Add " : "Replace with ") << matches.size()
            << " routes to MirroringDestination@" << destination;
@@ skipping 11 matching lines @@
       if (it->destination == destination) {
         ChangeRoute(&(*it), NULL);
         redivert_candidates.insert(it->source_render_frame);
       }
     }
   }
   if (!redivert_candidates.empty())
     InitiateQueriesToFindNewDestination(destination, redivert_candidates);
 }
 
+void AudioMirroringManager::UpdateRoutesToDuplicateDestination(
+    MirroringDestination* destination,
+    bool add_only,
+    const std::set<SourceFrameRef>& matches) {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  if (std::find(sessions_.begin(), sessions_.end(), destination) ==
+      sessions_.end()) {
+    return;  // Query result callback invoked after StopMirroring().
+  }
+
+  for (StreamRoutes::iterator it = routes_.begin(); it != routes_.end(); ++it) {
+    if (matches.find(it->source_render_frame) != matches.end()) {
+      if (it->dup_destinations.count(destination) == 0) {

[miu, 2016/05/02 20:06:12]

These five lines of code do a map look-up three times.  Why not just once? 
Suggested replacement (use a reference to the value in the map entry):

  media::AudioPushSink*& pusher = it->dup_destinations[destination];
  if (!pusher) {
    pusher = destination->AddPushInput(it->diverter->GetAudioParameters());
    DCHECK(pusher);
    it->diverter->StartDuplicating(pusher);
  }

[qiangchen, 2016/05/03 16:58:22]

Done.

+        it->dup_destinations[destination] =
+            destination->AddPushInput(it->diverter->GetAudioParameters());
+        it->diverter->StartDuplicating(it->dup_destinations[destination]);
+      }
+

[miu, 2016/05/02 20:06:12]

nit: remove extra line

[qiangchen, 2016/05/03 16:58:22]

Done.

+    } else if (!add_only) {
+      if (it->dup_destinations.count(destination) > 0) {

[miu, 2016/05/02 20:06:12]

Same problem here (with repeated map look-ups).  Since you might be
conditionally removing the entry in the map, you'll want to use an iterator.

[qiangchen, 2016/05/03 16:58:23]

Done.

+        it->diverter->StopDuplicating(it->dup_destinations[destination]);
+        it->dup_destinations.erase(destination);
+      }
+    }
+  }
+}
+
 // static
 void AudioMirroringManager::ChangeRoute(
     StreamRoutingState* route, MirroringDestination* new_destination) {
   if (route->destination == new_destination)
     return;  // No change.
 
   if (route->destination) {
     DVLOG(1) << "Stop diverting render_process_id:render_frame_id="
              << route->source_render_frame.first << ':'
              << route->source_render_frame.second
@@ skipping 18 matching lines @@
   : source_render_frame(source_frame),
     diverter(stream_diverter),
     destination(NULL) {}
 
 AudioMirroringManager::StreamRoutingState::StreamRoutingState(
     const StreamRoutingState& other) = default;
 
 AudioMirroringManager::StreamRoutingState::~StreamRoutingState() {}
 
 }  // namespace content