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

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

If a description is set that requires making a default stream, and one
already exists, we'll now keep the existing default audio/video tracks,
rather than destroying them and recreating them. Destroying them caused
the blink MediaStream to go to an "ended" state, which is the root cause
of the bug.

BUG=webrtc:5250
Committed: https://crrev.com/bda7e0b932fc89598da95496efc8650bc0e2c86c
Cr-Commit-Position: refs/heads/master@{#10946}

[Taylor Brandstetter, 2015-11-25 00:34:53]

deadbeef@webrtc.org changed reviewers:
+ pthatcher@webrtc.org

[pthatcher1, 2015-12-02 22:05:53]

https://codereview.webrtc.org/1469833006/diff/1/talk/app/webrtc/peerconnectio...
File talk/app/webrtc/peerconnection.cc (right):

https://codereview.webrtc.org/1469833006/diff/1/talk/app/webrtc/peerconnectio...
talk/app/webrtc/peerconnection.cc:1510: if (info.stream_label !=
kDefaultStreamLabel &&
Would it be cleaner to put this inside of UpdateRemoteStreamList?

if (info.stream_label == kDefaultStreamLabel) {
  if (!remote_info_.IsDefaultMediaStreamNeeded() ||
      !remote_info_.default_audio_track_needed)) {	
    OnRemoteTrackRemoved(info.stream_label, info.track_id, media_type);
    track_it = current_tracks->erase(track_it);
   }
} else ... {
}

And then call UpdateRemoteStreamList inside of SetRemoteDescription in both the
rejected and non-rejected cases?

[Taylor Brandstetter, 2015-12-03 01:45:03]

I did what we discussed and moved the default track removal (and also creation)
to UpdateRemoteStreamsList.

I also removed remote_audio_track_needed_ and remote_video_track_needed_ from
RemotePeerInfo, and instead made "default_track_needed" a parameter passed into
UpdateRemoteStreamsList. It didn't make much sense that those variables were
only used in that function and immediately before calling it, so this makes
things cleaner, and means you can't introduce a bug by forgetting to set those
variables.

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

lgtm

That looks a lot better.  Thank you for changing it.  Now, just a few little
tweaks could make it more readable.  I'll leave it up to your judgement how many
of them to apply.

https://codereview.webrtc.org/1469833006/diff/20001/talk/app/webrtc/peerconne...
File talk/app/webrtc/peerconnection.cc (right):

https://codereview.webrtc.org/1469833006/diff/20001/talk/app/webrtc/peerconne...
talk/app/webrtc/peerconnection.cc:1091: const cricket::ContentInfo*
video_content = GetFirstVideoContent(remote_desc);
I think you can use 

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

To save a few casts and lines.

https://codereview.webrtc.org/1469833006/diff/20001/talk/app/webrtc/peerconne...
talk/app/webrtc/peerconnection.cc:1109: }
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;
}

https://codereview.webrtc.org/1469833006/diff/20001/talk/app/webrtc/peerconne...
talk/app/webrtc/peerconnection.cc:1123: audio_content->description);
This line (or 3) could go away if you use the function I mentioned before.

https://codereview.webrtc.org/1469833006/diff/20001/talk/app/webrtc/peerconne...
talk/app/webrtc/peerconnection.cc:1535: }
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;

https://codereview.webrtc.org/1469833006/diff/20001/talk/app/webrtc/peerconne...
talk/app/webrtc/peerconnection.cc:1566:
remote_streams_->find(kDefaultStreamLabel);
A better name for that variable might be "default_stream"

https://codereview.webrtc.org/1469833006/diff/20001/talk/app/webrtc/peerconne...
talk/app/webrtc/peerconnection.cc:1575: : kDefaultVideoTrackLabel;
And that default_track_id

https://codereview.webrtc.org/1469833006/diff/20001/talk/app/webrtc/peerconne...
talk/app/webrtc/peerconnection.cc:1577: FindTrackInfo(*current_tracks,
kDefaultStreamLabel, track_id);
And that default_track

https://codereview.webrtc.org/1469833006/diff/20001/talk/app/webrtc/peerconne...
File talk/app/webrtc/peerconnection.h (right):

https://codereview.webrtc.org/1469833006/diff/20001/talk/app/webrtc/peerconne...
talk/app/webrtc/peerconnection.h:239: // MediaStreamTrack. |media_type| is the
type of the |streams| and can be
Then perhaps a better name would be create_default_track.

https://codereview.webrtc.org/1469833006/diff/20001/talk/app/webrtc/peerconne...
talk/app/webrtc/peerconnection.h:379: bool remote_peer_msid_supported_ = false;
Perhaps a better name would be remote_peer_supports_msid_.  Then you probably
don't even need the comment.

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

Addressed all your comments.

https://codereview.webrtc.org/1469833006/diff/20001/talk/app/webrtc/peerconne...
File talk/app/webrtc/peerconnection.cc (right):

https://codereview.webrtc.org/1469833006/diff/20001/talk/app/webrtc/peerconne...
talk/app/webrtc/peerconnection.cc:1091: const cricket::ContentInfo*
video_content = GetFirstVideoContent(remote_desc);
On 2015/12/03 20:07:39, pthatcher1 wrote:
> I think you can use 
> 
> const cricket::AudioContentDescription* audio =
> GetFirstAudioContentDescriptionr(remote_desc);
> 
> To save a few casts and lines.

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

https://codereview.webrtc.org/1469833006/diff/20001/talk/app/webrtc/peerconne...
talk/app/webrtc/peerconnection.cc:1109: }
On 2015/12/03 20:07:40, pthatcher1 wrote:
> 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;
> }

Done.

https://codereview.webrtc.org/1469833006/diff/20001/talk/app/webrtc/peerconne...
talk/app/webrtc/peerconnection.cc:1123: audio_content->description);
On 2015/12/03 20:07:40, pthatcher1 wrote:
> This line (or 3) could go away if you use the function I mentioned before.

Done.

https://codereview.webrtc.org/1469833006/diff/20001/talk/app/webrtc/peerconne...
talk/app/webrtc/peerconnection.cc:1535: }
On 2015/12/03 20:07:40, pthatcher1 wrote:
> 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;

I have another idea. Take a look.

https://codereview.webrtc.org/1469833006/diff/20001/talk/app/webrtc/peerconne...
talk/app/webrtc/peerconnection.cc:1566:
remote_streams_->find(kDefaultStreamLabel);
On 2015/12/03 20:07:40, pthatcher1 wrote:
> A better name for that variable might be "default_stream"

Done.

https://codereview.webrtc.org/1469833006/diff/20001/talk/app/webrtc/peerconne...
talk/app/webrtc/peerconnection.cc:1575: : kDefaultVideoTrackLabel;
On 2015/12/03 20:07:40, pthatcher1 wrote:
> And that default_track_id

Done.

https://codereview.webrtc.org/1469833006/diff/20001/talk/app/webrtc/peerconne...
talk/app/webrtc/peerconnection.cc:1577: FindTrackInfo(*current_tracks,
kDefaultStreamLabel, track_id);
On 2015/12/03 20:07:40, pthatcher1 wrote:
> And that default_track

Done.

https://codereview.webrtc.org/1469833006/diff/20001/talk/app/webrtc/peerconne...
File talk/app/webrtc/peerconnection.h (right):

https://codereview.webrtc.org/1469833006/diff/20001/talk/app/webrtc/peerconne...
talk/app/webrtc/peerconnection.h:239: // MediaStreamTrack. |media_type| is the
type of the |streams| and can be
On 2015/12/03 20:07:40, pthatcher1 wrote:
> Then perhaps a better name would be create_default_track.

But an existing default track will be destroyed if it's "false". I guess I can
make the comment more explicit.

https://codereview.webrtc.org/1469833006/diff/20001/talk/app/webrtc/peerconne...
talk/app/webrtc/peerconnection.h:379: bool remote_peer_msid_supported_ = false;
On 2015/12/03 20:07:40, pthatcher1 wrote:
> Perhaps a better name would be remote_peer_supports_msid_.  Then you probably
> don't even need the comment.

Done.

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

lgtm

https://codereview.webrtc.org/1469833006/diff/20001/talk/app/webrtc/peerconne...
File talk/app/webrtc/peerconnection.cc (right):

https://codereview.webrtc.org/1469833006/diff/20001/talk/app/webrtc/peerconne...
talk/app/webrtc/peerconnection.cc:1535: }
On 2015/12/04 22:04:54, Taylor Brandstetter wrote:
> On 2015/12/03 20:07:40, pthatcher1 wrote:
> > 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;
> 
> I have another idea. Take a look.

I like it.

[Taylor Brandstetter, 2015-12-09 00:18:52]

The CQ bit was checked by deadbeef@webrtc.org

[commit-bot: I haz the power, 2015-12-09 00:19:06]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1469833006/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1469833006/40001

[commit-bot: I haz the power, 2015-12-09 01:13:46]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-12-09 01:13:58]

Description was changed from

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

If a description is set that requires making a default stream, and one
already exists, we'll now keep the existing default audio/video tracks,
rather than destroying them and recreating them. Destroying them caused
the blink MediaStream to go to an "ended" state, which is the root cause
of the bug.

BUG=webrtc:5250
==========

to

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

If a description is set that requires making a default stream, and one
already exists, we'll now keep the existing default audio/video tracks,
rather than destroying them and recreating them. Destroying them caused
the blink MediaStream to go to an "ended" state, which is the root cause
of the bug.

BUG=webrtc:5250

Committed: https://crrev.com/bda7e0b932fc89598da95496efc8650bc0e2c86c
Cr-Commit-Position: refs/heads/master@{#10946}
==========

[commit-bot: I haz the power, 2015-12-09 01:14:00]

Patchset 3 (id:??) landed as
https://crrev.com/bda7e0b932fc89598da95496efc8650bc0e2c86c
Cr-Commit-Position: refs/heads/master@{#10946}