Unmute Tab Audio For Desktop Share

content/browser/media/capture/audio_mirroring_manager.cc

Index: content/browser/media/capture/audio_mirroring_manager.cc
diff --git a/content/browser/media/capture/audio_mirroring_manager.cc b/content/browser/media/capture/audio_mirroring_manager.cc
index f8b8b3a5b183e9d1b1ff07dd6728279b368d0c88..8b091ec43599df2cff21db0c93a39087707ab56f 100644
--- a/content/browser/media/capture/audio_mirroring_manager.cc
+++ b/content/browser/media/capture/audio_mirroring_manager.cc
@@ -81,12 +81,12 @@ void AudioMirroringManager::StartMirroring(MirroringDestination* destination) {
     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()) {
@@ -102,18 +102,10 @@ void AudioMirroringManager::StartMirroring(MirroringDestination* destination) {
 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 =
@@ -132,20 +124,30 @@ void AudioMirroringManager::InitiateQueriesToFindNewDestination(
 
   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());
 
@@ -177,6 +179,33 @@ void AudioMirroringManager::UpdateRoutesToDestination(
     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) {