Fixing issue with default stream upon setting 2nd remote description.

talk/app/webrtc/peerconnection.cc

 /*
  * libjingle
  * Copyright 2012 Google Inc.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions are met:
  *
  *  1. Redistributions of source code must retain the above copyright notice,
  *     this list of conditions and the following disclaimer.
  *  2. Redistributions in binary form must reproduce the above copyright notice,
@@ skipping 1069 matching lines @@
 
   // If setting the description decided our SSL role, allocate any necessary
   // SCTP sids.
   rtc::SSLRole role;
   if (session_->data_channel_type() == cricket::DCT_SCTP &&
       session_->GetSslRole(&role)) {
     AllocateSctpSids(role);
   }
 
   const cricket::SessionDescription* remote_desc = desc->description();
+  const cricket::ContentInfo* audio_content = GetFirstAudioContent(remote_desc);
+  const cricket::ContentInfo* video_content = GetFirstVideoContent(remote_desc);

[pthatcher1, 2015/12/03 20:07:39]

I think you can use 

const cricket::AudioContentDescription* audio =
GetFirstAudioContentDescriptionr(remote_desc);

To save a few casts and lines.

[Taylor Brandstetter, 2015/12/04 22:04:54]

I still need the ContentInfo pointers to tell if they're rejected later. But I
can do something similar.

+
+  remote_peer_msid_supported_ |= remote_desc->msid_supported();
+  // Just in case the peer supports MSID, but doesn't indicate so with
+  // "a=msid-semantic"...
+  if (audio_content &&
+      !static_cast<const cricket::AudioContentDescription*>(
+           audio_content->description)
+           ->streams()
+           .empty()) {
+    remote_peer_msid_supported_ = true;
+  }
+  if (video_content &&
+      !static_cast<const cricket::VideoContentDescription*>(
+           video_content->description)
+           ->streams()
+           .empty()) {
+    remote_peer_msid_supported_ = true;
+  }

[pthatcher1, 2015/12/03 20:07:40]

I think it could be:

auto audio = GetFirstAudioContentDescription(remote_desc);
auto video = GetVideoAudioContentDescriptionr(remote_desc);
if (remote_desc->msid_supported() ||
    (audio && !audio->streams.empty() ||
    (video && !video->streams.empty()) {
  remote_peer_supports_msid_ = true;
}

[Taylor Brandstetter, 2015/12/04 22:04:54]

Done.

 
   // We wait to signal new streams until we finish processing the description,
   // since only at that point will new streams have all their tracks.
   rtc::scoped_refptr<StreamCollection> new_streams(StreamCollection::Create());
 
   // Find all audio rtp streams and create corresponding remote AudioTracks
   // and MediaStreams.
-  const cricket::ContentInfo* audio_content = GetFirstAudioContent(remote_desc);
   if (audio_content) {
     if (audio_content->rejected) {
       RemoveTracks(cricket::MEDIA_TYPE_AUDIO);
     } else {
       const cricket::AudioContentDescription* desc =
           static_cast<const cricket::AudioContentDescription*>(
               audio_content->description);

[pthatcher1, 2015/12/03 20:07:40]

This line (or 3) could go away if you use the function I mentioned before.

[Taylor Brandstetter, 2015/12/04 22:04:54]

Done.

-      UpdateRemoteStreamsList(GetActiveStreams(desc), desc->type(),
+      bool default_audio_track_needed =
+          !remote_peer_msid_supported_ && desc->streams().empty() &&
+          MediaContentDirectionHasSend(desc->direction());
+      UpdateRemoteStreamsList(GetActiveStreams(desc),
+                              default_audio_track_needed, desc->type(),
                               new_streams);
-      remote_info_.default_audio_track_needed =
-          !remote_desc->msid_supported() && desc->streams().empty() &&
-          MediaContentDirectionHasSend(desc->direction());
     }
   }
 
   // Find all video rtp streams and create corresponding remote VideoTracks
   // and MediaStreams.
-  const cricket::ContentInfo* video_content = GetFirstVideoContent(remote_desc);
   if (video_content) {
     if (video_content->rejected) {
       RemoveTracks(cricket::MEDIA_TYPE_VIDEO);
     } else {
       const cricket::VideoContentDescription* desc =
           static_cast<const cricket::VideoContentDescription*>(
               video_content->description);
-      UpdateRemoteStreamsList(GetActiveStreams(desc), desc->type(),
+      bool default_video_track_needed =
+          !remote_peer_msid_supported_ && desc->streams().empty() &&
+          MediaContentDirectionHasSend(desc->direction());
+      UpdateRemoteStreamsList(GetActiveStreams(desc),
+                              default_video_track_needed, desc->type(),
                               new_streams);
-      remote_info_.default_video_track_needed =
-          !remote_desc->msid_supported() && desc->streams().empty() &&
-          MediaContentDirectionHasSend(desc->direction());
     }
   }
 
   // Update the DataChannels with the information from the remote peer.
   const cricket::ContentInfo* data_content = GetFirstDataContent(remote_desc);
   if (data_content) {
     const cricket::DataContentDescription* desc =
         static_cast<const cricket::DataContentDescription*>(
             data_content->description);
     if (rtc::starts_with(desc->protocol().data(),
                          cricket::kMediaProtocolRtpPrefix)) {
       UpdateRemoteRtpDataChannels(GetActiveStreams(desc));
     }
   }
 
   // Iterate new_streams and notify the observer about new MediaStreams.
   for (size_t i = 0; i < new_streams->count(); ++i) {
     MediaStreamInterface* new_stream = new_streams->at(i);
     stats_->AddStream(new_stream);
     observer_->OnAddStream(new_stream);
   }
 
-  // Find removed MediaStreams.
-  if (remote_info_.IsDefaultMediaStreamNeeded() &&
-      remote_streams_->find(kDefaultStreamLabel) != nullptr) {
-    // The default media stream already exists. No need to do anything.
-  } else {
-    UpdateEndedRemoteMediaStreams();
-    remote_info_.msid_supported |= remote_streams_->count() > 0;
-  }
-  MaybeCreateDefaultStream();
+  UpdateEndedRemoteMediaStreams();
 
   SetSessionDescriptionMsg* msg = new SetSessionDescriptionMsg(observer);
   signaling_thread()->Post(this, MSG_SET_SESSIONDESCRIPTION_SUCCESS, msg);
 }
 
 bool PeerConnection::SetConfiguration(const RTCConfiguration& config) {
   if (port_allocator_) {
     std::vector<PortAllocatorFactoryInterface::StunConfiguration> stuns;
     std::vector<PortAllocatorFactoryInterface::TurnConfiguration> turns;
     if (!ParseIceServers(config.servers, &stuns, &turns)) {
@@ skipping 316 matching lines @@
   // are not signaled in the SDP so does not go through that path and must be
   // handled here.
   if (session_->data_channel_type() == cricket::DCT_SCTP) {
     session_options->data_channel_type = cricket::DCT_SCTP;
   }
   return true;
 }
 
 void PeerConnection::RemoveTracks(cricket::MediaType media_type) {
   UpdateLocalTracks(std::vector<cricket::StreamParams>(), media_type);
-  UpdateRemoteStreamsList(std::vector<cricket::StreamParams>(), media_type,
-                          nullptr);
+  UpdateRemoteStreamsList(std::vector<cricket::StreamParams>(), false,
+                          media_type, nullptr);
 }
 
 void PeerConnection::UpdateRemoteStreamsList(
     const cricket::StreamParamsVec& streams,
+    bool default_track_needed,
     cricket::MediaType media_type,
     StreamCollection* new_streams) {
   TrackInfos* current_tracks = GetRemoteTracks(media_type);
 
   // Find removed tracks. I.e., tracks where the track id or ssrc don't match
-  // the
-  // new StreamParam.
+  // the new StreamParam.
   auto track_it = current_tracks->begin();
   while (track_it != current_tracks->end()) {
     const TrackInfo& info = *track_it;
     const cricket::StreamParams* params =
         cricket::GetStreamBySsrc(streams, info.ssrc);
     if (!params || params->id != info.track_id) {
+      // If this is a default track, and we still need it, don't remove it.
+      if (info.stream_label == kDefaultStreamLabel && default_track_needed) {
+        ++track_it;
+        continue;
+      }
       OnRemoteTrackRemoved(info.stream_label, info.track_id, media_type);
       track_it = current_tracks->erase(track_it);
     } else {
       ++track_it;
     }

[pthatcher1, 2015/12/03 20:07:40]

Would it be more clear as this?

if (info.stream_label == kDefaultStreamLabel && default_track_needed) {
  // If this is a default track, and we still need it, don't remove
} else if (!params || params->id != info.track_id)
  // This is not a default track, but it's gone, so remote it.
  OnRemoteTrackRemoved(info.stream_label, info.track_id, media_type);
  track_it = current_tracks->erase(track_it);
  continue;
}
++track_it;

[Taylor Brandstetter, 2015/12/04 22:04:54]

I have another idea. Take a look.

[pthatcher1, 2015/12/05 00:31:41]

I like it.

   }
 
   // Find new and active tracks.
   for (const cricket::StreamParams& params : streams) {
     // The sync_label is the MediaStream label and the |stream.id| is the
     // track id.
     const std::string& stream_label = params.sync_label;
     const std::string& track_id = params.id;
     uint32_t ssrc = params.first_ssrc();
 
     rtc::scoped_refptr<MediaStreamInterface> stream =
         remote_streams_->find(stream_label);
     if (!stream) {
       // This is a new MediaStream. Create a new remote MediaStream.
       stream = remote_stream_factory_->CreateMediaStream(stream_label);
       remote_streams_->AddStream(stream);
       new_streams->AddStream(stream);
     }
 
     const TrackInfo* track_info =
         FindTrackInfo(*current_tracks, stream_label, track_id);
     if (!track_info) {
       current_tracks->push_back(TrackInfo(stream_label, track_id, ssrc));
       OnRemoteTrackSeen(stream_label, track_id, ssrc, media_type);
     }
   }
+
+  // Add default track if necessary.
+  if (default_track_needed) {
+    rtc::scoped_refptr<MediaStreamInterface> stream =
+        remote_streams_->find(kDefaultStreamLabel);

[pthatcher1, 2015/12/03 20:07:40]

A better name for that variable might be "default_stream"

[Taylor Brandstetter, 2015/12/04 22:04:54]

Done.

+    if (!stream) {
+      // Create the new default MediaStream.
+      stream = remote_stream_factory_->CreateMediaStream(kDefaultStreamLabel);
+      remote_streams_->AddStream(stream);
+      new_streams->AddStream(stream);
+    }
+    std::string track_id = (media_type == cricket::MEDIA_TYPE_AUDIO)
+                               ? kDefaultAudioTrackLabel
+                               : kDefaultVideoTrackLabel;

[pthatcher1, 2015/12/03 20:07:40]

And that default_track_id

[Taylor Brandstetter, 2015/12/04 22:04:54]

Done.

+    const TrackInfo* track_info =
+        FindTrackInfo(*current_tracks, kDefaultStreamLabel, track_id);

[pthatcher1, 2015/12/03 20:07:40]

And that default_track

[Taylor Brandstetter, 2015/12/04 22:04:54]

Done.

+    if (!track_info) {
+      current_tracks->push_back(TrackInfo(kDefaultStreamLabel, track_id, 0));
+      OnRemoteTrackSeen(kDefaultStreamLabel, track_id, 0, media_type);
+    }
+  }
 }
 
 void PeerConnection::OnRemoteTrackSeen(const std::string& stream_label,
                                        const std::string& track_id,
                                        uint32_t ssrc,
                                        cricket::MediaType media_type) {
   MediaStreamInterface* stream = remote_streams_->find(stream_label);
 
   if (media_type == cricket::MEDIA_TYPE_AUDIO) {
     AudioTrackInterface* audio_track =
@@ skipping 42 matching lines @@
       streams_to_remove.push_back(stream);
     }
   }
 
   for (const auto& stream : streams_to_remove) {
     remote_streams_->RemoveStream(stream);
     observer_->OnRemoveStream(stream);
   }
 }
 
-void PeerConnection::MaybeCreateDefaultStream() {
-  if (!remote_info_.IsDefaultMediaStreamNeeded()) {
-    return;
-  }
-
-  bool default_created = false;
-
-  rtc::scoped_refptr<MediaStreamInterface> default_remote_stream =
-      remote_streams_->find(kDefaultStreamLabel);
-  if (default_remote_stream == nullptr) {
-    default_created = true;
-    default_remote_stream =
-        remote_stream_factory_->CreateMediaStream(kDefaultStreamLabel);
-    remote_streams_->AddStream(default_remote_stream);
-  }
-  if (remote_info_.default_audio_track_needed &&
-      default_remote_stream->GetAudioTracks().size() == 0) {
-    remote_audio_tracks_.push_back(
-        TrackInfo(kDefaultStreamLabel, kDefaultAudioTrackLabel, 0));
-    OnRemoteTrackSeen(kDefaultStreamLabel, kDefaultAudioTrackLabel, 0,
-                      cricket::MEDIA_TYPE_AUDIO);
-  }
-  if (remote_info_.default_video_track_needed &&
-      default_remote_stream->GetVideoTracks().size() == 0) {
-    remote_video_tracks_.push_back(
-        TrackInfo(kDefaultStreamLabel, kDefaultVideoTrackLabel, 0));
-    OnRemoteTrackSeen(kDefaultStreamLabel, kDefaultVideoTrackLabel, 0,
-                      cricket::MEDIA_TYPE_VIDEO);
-  }
-  if (default_created) {
-    stats_->AddStream(default_remote_stream);
-    observer_->OnAddStream(default_remote_stream);
-  }
-}
-
 void PeerConnection::EndRemoteTracks(cricket::MediaType media_type) {
   TrackInfos* current_tracks = GetRemoteTracks(media_type);
   for (TrackInfos::iterator track_it = current_tracks->begin();
        track_it != current_tracks->end(); ++track_it) {
     const TrackInfo& info = *track_it;
     MediaStreamInterface* stream = remote_streams_->find(info.stream_label);
     if (media_type == cricket::MEDIA_TYPE_AUDIO) {
       AudioTrackInterface* track = stream->FindAudioTrack(info.track_id);
       // There's no guarantee the track is still available, e.g. the track may
       // have been removed from the stream by javascript.
@@ skipping 377 matching lines @@
 DataChannel* PeerConnection::FindDataChannelBySid(int sid) const {
   for (const auto& channel : sctp_data_channels_) {
     if (channel->id() == sid) {
       return channel;
     }
   }
   return nullptr;
 }
 
 }  // namespace webrtc