RTCPeerConnection.addTrack and removeTrack added (behind flag)

chrome/browser/media/webrtc/webrtc_rtp_browsertest.cc

Index: chrome/browser/media/webrtc/webrtc_rtp_browsertest.cc
diff --git a/chrome/browser/media/webrtc/webrtc_rtp_browsertest.cc b/chrome/browser/media/webrtc/webrtc_rtp_browsertest.cc
index bd591a994707151759292c68c2c31ff09259b054..d73c5c3873df2a3f110eac6b484345467e3c1cf4 100644
--- a/chrome/browser/media/webrtc/webrtc_rtp_browsertest.cc
+++ b/chrome/browser/media/webrtc/webrtc_rtp_browsertest.cc
@@ -2,6 +2,9 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
+#include <string>
+#include <vector>
+
 #include "base/command_line.h"
 #include "chrome/browser/media/webrtc/webrtc_browsertest_base.h"
 #include "content/public/common/content_switches.h"
@@ -63,3 +66,224 @@ IN_PROC_BROWSER_TEST_F(WebRtcRtpBrowserTest, GetReceivers) {
   VerifyRtpReceivers(left_tab_, 2);
   VerifyRtpReceivers(right_tab_, 6);
 }
+
+IN_PROC_BROWSER_TEST_F(WebRtcRtpBrowserTest, AddAndRemoveTracksWithoutStream) {
+  StartServerAndOpenTabs();
+
+  SetupPeerconnectionWithoutLocalStream(left_tab_);
+  SetupPeerconnectionWithoutLocalStream(right_tab_);
+
+  // TODO(hbos): Here and in other "AddAndRemoveTracks" tests: when ontrack and
+  // ended events are supported, verify that these are fired on the remote side
+  // when tracks are added and removed. https://crbug.com/webrtc/7933
+
+  // Add two tracks.
+  StartCountingOnNegotiationNeeded(left_tab_);
+  std::vector<std::string> ids =
+      CreateAndAddAudioAndVideoTrack(left_tab_, StreamArgumentType::NO_STREAM);
+  WaitUntilOnNegotiationCountIs(left_tab_, 2);

[Taylor_Brandstetter, 2017/07/07 19:44:13]

Actually, if we followed the standard, negotiationneeded would only fire once.
It's only supposed to fire if the flag flips from "false" to "true", or if it's
still "true" after an offer/answer finishes. Should leave a TODO here.

[hbos_chromium, 2017/07/10 12:32:59]

Done, filed an issue, this is a bug that should already be affecting addStream
etc.

+  std::string audio_stream_id = ids[0];
+  std::string audio_track_id = ids[1];
+  std::string video_stream_id = ids[2];
+  std::string video_track_id = ids[3];
+  EXPECT_EQ("null", audio_stream_id);
+  EXPECT_NE("null", audio_track_id);
+  EXPECT_EQ("null", video_stream_id);
+  EXPECT_NE("null", video_track_id);
+  EXPECT_FALSE(
+      HasLocalStreamWithTrack(left_tab_, audio_stream_id, audio_track_id));
+  EXPECT_FALSE(
+      HasLocalStreamWithTrack(left_tab_, video_stream_id, video_track_id));
+  EXPECT_TRUE(HasSenderWithTrack(left_tab_, audio_track_id));
+  EXPECT_TRUE(HasSenderWithTrack(left_tab_, video_track_id));
+  VerifyRtpSenders(left_tab_, 2);
+  // Negotiate call, sets remote description.
+  NegotiateCall(left_tab_, right_tab_);
+  // TODO(hbos): When |addTrack| without streams results in SDP that does not
+  // signal a remote stream to be added this should be EXPECT_FALSE.
+  // https://crbug.com/webrtc/7933
+  EXPECT_TRUE(HasRemoteStreamWithTrack(right_tab_, kUndefined, audio_track_id));
+  EXPECT_TRUE(HasRemoteStreamWithTrack(right_tab_, kUndefined, video_track_id));
+  EXPECT_TRUE(HasReceiverWithTrack(right_tab_, audio_track_id));
+  EXPECT_TRUE(HasReceiverWithTrack(right_tab_, video_track_id));
+  VerifyRtpReceivers(right_tab_, 2);
+
+  // Remove first track.
+  RemoveTrack(left_tab_, audio_track_id);
+  WaitUntilOnNegotiationCountIs(left_tab_, 3);
+  EXPECT_FALSE(HasSenderWithTrack(left_tab_, audio_track_id));
+  EXPECT_TRUE(HasSenderWithTrack(left_tab_, video_track_id));
+  VerifyRtpSenders(left_tab_, 1);
+  // Re-negotiate call, sets remote description again.
+  NegotiateCall(left_tab_, right_tab_);
+  EXPECT_FALSE(
+      HasRemoteStreamWithTrack(right_tab_, kUndefined, audio_track_id));
+  EXPECT_TRUE(HasRemoteStreamWithTrack(right_tab_, kUndefined, video_track_id));
+  EXPECT_FALSE(HasReceiverWithTrack(right_tab_, audio_track_id));
+  EXPECT_TRUE(HasReceiverWithTrack(right_tab_, video_track_id));
+  VerifyRtpReceivers(right_tab_, 1);
+
+  // Remove second track.
+  RemoveTrack(left_tab_, video_track_id);
+  WaitUntilOnNegotiationCountIs(left_tab_, 4);
+  EXPECT_FALSE(HasSenderWithTrack(left_tab_, audio_track_id));
+  EXPECT_FALSE(HasSenderWithTrack(left_tab_, video_track_id));
+  VerifyRtpSenders(left_tab_, 0);
+  // Re-negotiate call, sets remote description again.
+  NegotiateCall(left_tab_, right_tab_);
+  EXPECT_FALSE(
+      HasRemoteStreamWithTrack(right_tab_, kUndefined, audio_track_id));
+  EXPECT_FALSE(
+      HasRemoteStreamWithTrack(right_tab_, kUndefined, video_track_id));
+  EXPECT_FALSE(HasReceiverWithTrack(right_tab_, audio_track_id));
+  EXPECT_FALSE(HasReceiverWithTrack(right_tab_, video_track_id));
+  VerifyRtpReceivers(right_tab_, 0);
+}
+
+IN_PROC_BROWSER_TEST_F(WebRtcRtpBrowserTest,
+                       AddAndRemoveTracksWithSharedStream) {
+  StartServerAndOpenTabs();
+
+  SetupPeerconnectionWithoutLocalStream(left_tab_);
+  SetupPeerconnectionWithoutLocalStream(right_tab_);
+
+  // Add two tracks.
+  StartCountingOnNegotiationNeeded(left_tab_);
+  std::vector<std::string> ids = CreateAndAddAudioAndVideoTrack(
+      left_tab_, StreamArgumentType::SHARED_STREAM);
+  WaitUntilOnNegotiationCountIs(left_tab_, 2);
+  std::string audio_stream_id = ids[0];
+  std::string audio_track_id = ids[1];
+  std::string video_stream_id = ids[2];
+  std::string video_track_id = ids[3];
+  EXPECT_NE("null", audio_stream_id);
+  EXPECT_NE("null", audio_track_id);
+  EXPECT_NE("null", video_stream_id);
+  EXPECT_NE("null", video_track_id);
+  EXPECT_EQ(audio_stream_id, video_stream_id);
+  // TODO(hbos): When |getLocalStreams| is updated to return the streams of all
+  // senders, not just |addStream|-streams, then this will be EXPECT_TRUE.
+  // https://crbug.com/738918
+  EXPECT_FALSE(
+      HasLocalStreamWithTrack(left_tab_, audio_stream_id, audio_track_id));
+  EXPECT_FALSE(
+      HasLocalStreamWithTrack(left_tab_, video_stream_id, video_track_id));
+  EXPECT_TRUE(HasSenderWithTrack(left_tab_, audio_track_id));
+  EXPECT_TRUE(HasSenderWithTrack(left_tab_, video_track_id));
+  VerifyRtpSenders(left_tab_, 2);
+  // Negotiate call, sets remote description.
+  NegotiateCall(left_tab_, right_tab_);
+  EXPECT_TRUE(
+      HasRemoteStreamWithTrack(right_tab_, audio_stream_id, audio_track_id));
+  EXPECT_TRUE(
+      HasRemoteStreamWithTrack(right_tab_, video_stream_id, video_track_id));
+  EXPECT_TRUE(HasReceiverWithTrack(right_tab_, audio_track_id));
+  EXPECT_TRUE(HasReceiverWithTrack(right_tab_, video_track_id));
+  VerifyRtpReceivers(right_tab_, 2);
+
+  // Remove first track.
+  RemoveTrack(left_tab_, audio_track_id);
+  WaitUntilOnNegotiationCountIs(left_tab_, 3);
+  EXPECT_FALSE(HasSenderWithTrack(left_tab_, audio_track_id));
+  EXPECT_TRUE(HasSenderWithTrack(left_tab_, video_track_id));
+  VerifyRtpSenders(left_tab_, 1);
+  // Re-negotiate call, sets remote description again.
+  NegotiateCall(left_tab_, right_tab_);
+  EXPECT_FALSE(
+      HasRemoteStreamWithTrack(right_tab_, audio_stream_id, audio_track_id));
+  EXPECT_TRUE(
+      HasRemoteStreamWithTrack(right_tab_, video_stream_id, video_track_id));
+  EXPECT_FALSE(HasReceiverWithTrack(right_tab_, audio_track_id));
+  EXPECT_TRUE(HasReceiverWithTrack(right_tab_, video_track_id));
+  VerifyRtpReceivers(right_tab_, 1);
+
+  // Remove second track.
+  RemoveTrack(left_tab_, video_track_id);
+  WaitUntilOnNegotiationCountIs(left_tab_, 4);
+  EXPECT_FALSE(HasSenderWithTrack(left_tab_, audio_track_id));
+  EXPECT_FALSE(HasSenderWithTrack(left_tab_, video_track_id));
+  VerifyRtpSenders(left_tab_, 0);
+  // Re-negotiate call, sets remote description again.
+  NegotiateCall(left_tab_, right_tab_);
+  EXPECT_FALSE(
+      HasRemoteStreamWithTrack(right_tab_, audio_stream_id, audio_track_id));
+  EXPECT_FALSE(
+      HasRemoteStreamWithTrack(right_tab_, video_stream_id, video_track_id));
+  EXPECT_FALSE(HasReceiverWithTrack(right_tab_, audio_track_id));
+  EXPECT_FALSE(HasReceiverWithTrack(right_tab_, video_track_id));
+  VerifyRtpReceivers(right_tab_, 0);
+}
+
+IN_PROC_BROWSER_TEST_F(WebRtcRtpBrowserTest,
+                       AddAndRemoveTracksWithIndividualStreams) {
+  StartServerAndOpenTabs();
+
+  SetupPeerconnectionWithoutLocalStream(left_tab_);
+  SetupPeerconnectionWithoutLocalStream(right_tab_);
+
+  // Add two tracks.
+  StartCountingOnNegotiationNeeded(left_tab_);
+  std::vector<std::string> ids = CreateAndAddAudioAndVideoTrack(
+      left_tab_, StreamArgumentType::INDIVIDUAL_STREAMS);
+  WaitUntilOnNegotiationCountIs(left_tab_, 2);
+  std::string audio_stream_id = ids[0];
+  std::string audio_track_id = ids[1];
+  std::string video_stream_id = ids[2];
+  std::string video_track_id = ids[3];
+  EXPECT_NE("null", audio_stream_id);
+  EXPECT_NE("null", audio_track_id);
+  EXPECT_NE("null", video_stream_id);
+  EXPECT_NE("null", video_track_id);
+  EXPECT_NE(audio_stream_id, video_stream_id);
+  // TODO(hbos): When |getLocalStreams| is updated to return the streams of all
+  // senders, not just |addStream|-streams, then this will be EXPECT_TRUE.
+  // https://crbug.com/738918
+  EXPECT_FALSE(
+      HasLocalStreamWithTrack(left_tab_, audio_stream_id, audio_track_id));
+  EXPECT_FALSE(
+      HasLocalStreamWithTrack(left_tab_, video_stream_id, video_track_id));
+  EXPECT_TRUE(HasSenderWithTrack(left_tab_, audio_track_id));
+  EXPECT_TRUE(HasSenderWithTrack(left_tab_, video_track_id));
+  VerifyRtpSenders(left_tab_, 2);
+  // Negotiate call, sets remote description.
+  NegotiateCall(left_tab_, right_tab_);
+  EXPECT_TRUE(
+      HasRemoteStreamWithTrack(right_tab_, audio_stream_id, audio_track_id));
+  EXPECT_TRUE(
+      HasRemoteStreamWithTrack(right_tab_, video_stream_id, video_track_id));
+  EXPECT_TRUE(HasReceiverWithTrack(right_tab_, audio_track_id));
+  EXPECT_TRUE(HasReceiverWithTrack(right_tab_, video_track_id));
+  VerifyRtpReceivers(right_tab_, 2);
+
+  // Remove first track.
+  RemoveTrack(left_tab_, audio_track_id);
+  WaitUntilOnNegotiationCountIs(left_tab_, 3);
+  EXPECT_FALSE(HasSenderWithTrack(left_tab_, audio_track_id));
+  EXPECT_TRUE(HasSenderWithTrack(left_tab_, video_track_id));
+  VerifyRtpSenders(left_tab_, 1);
+  // Re-negotiate call, sets remote description again.
+  NegotiateCall(left_tab_, right_tab_);
+  EXPECT_FALSE(
+      HasRemoteStreamWithTrack(right_tab_, audio_stream_id, audio_track_id));
+  EXPECT_TRUE(
+      HasRemoteStreamWithTrack(right_tab_, video_stream_id, video_track_id));
+  EXPECT_FALSE(HasReceiverWithTrack(right_tab_, audio_track_id));
+  EXPECT_TRUE(HasReceiverWithTrack(right_tab_, video_track_id));
+  VerifyRtpReceivers(right_tab_, 1);
+
+  // Remove second track.
+  RemoveTrack(left_tab_, video_track_id);
+  WaitUntilOnNegotiationCountIs(left_tab_, 4);
+  EXPECT_FALSE(HasSenderWithTrack(left_tab_, audio_track_id));
+  EXPECT_FALSE(HasSenderWithTrack(left_tab_, video_track_id));
+  VerifyRtpSenders(left_tab_, 0);
+  // Re-negotiate call, sets remote description again.
+  NegotiateCall(left_tab_, right_tab_);
+  EXPECT_FALSE(
+      HasRemoteStreamWithTrack(right_tab_, audio_stream_id, audio_track_id));
+  EXPECT_FALSE(
+      HasRemoteStreamWithTrack(right_tab_, video_stream_id, video_track_id));
+  EXPECT_FALSE(HasReceiverWithTrack(right_tab_, audio_track_id));
+  EXPECT_FALSE(HasReceiverWithTrack(right_tab_, video_track_id));
+  VerifyRtpReceivers(right_tab_, 0);
+}

[Taylor_Brandstetter, 2017/07/07 19:44:13]

I like this change; it's much easier to see what the tests are testing by
reading this code by itself.

It still may be a good idea to move the "negotiationneeded" testing and
"removeTrack" testing into standalone tests, though. I'll let you decide if you
want to do that.

[hbos_chromium, 2017/07/10 12:32:59]

I'd like that except the setup for the remove track tests would be identical to
all of the addtrack tests and I'd like to avoid helper functions that basically
contain an entire test or copying this many lines of code in multiple tests.