Allow default video receive channel to receive on any unsignalled SSRC,

Allow any unsignalled SSRC changes on default video receive channel.

The first unsignalled SSRC creates a default receive channel.
Any unsignalled SSRC changes after that replace the default SSRC.
Add unit tests for changing unsignalled SSRCs.

BUG=webrtc:5208
Review-Url: https://codereview.webrtc.org/2692993009
Cr-Commit-Position: refs/heads/master@{#16682}
Committed: https://chromium.googlesource.com/external/webrtc/+/8a855d6916194d4a6941417d63c2c00dd536f6fc

[mo.zanaty, 2017-02-15 21:31:21]

Description was changed from

==========
Allow default video receive channel to receive on any unsignalled SSRC,
even if it changes.
Add unit tests for changing unsignalled SSRCs.

BUG=webrtc:5208
==========

to

==========
Allow default video receive channel to receive on any unsignalled SSRC,
even if it changes.
Add unit tests for changing unsignalled SSRCs.

BUG=webrtc:5208
==========

[mo.zanaty, 2017-02-15 21:31:21]

mzanaty@cisco.com changed reviewers:
+ pthatcher@webrtc.org

[marpan, 2017-02-16 00:21:42]

marpan@google.com changed reviewers:
+ mflodman@webrtc.org

[Taylor Brandstetter, 2017-02-16 00:36:33]

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

[Taylor Brandstetter, 2017-02-16 00:36:34]

Looks good; just some minor nits, and suggestion for the unit test. Could you
also edit the CL description to be in the form:

<80-character wide summary>
<newline>
<More description>

Sidenote: eventually it would be good to have a fully end-to-end test for this
(peerconnection_unittest.cc), though that would be a lot easier after I finish
cleaning up our test infrastructure. Did you test this end-to-end in a local
build of Chrome yet?

https://codereview.webrtc.org/2692993009/diff/1/webrtc/media/engine/webrtcvid...
File webrtc/media/engine/webrtcvideoengine2_unittest.cc (right):

https://codereview.webrtc.org/2692993009/diff/1/webrtc/media/engine/webrtcvid...
webrtc/media/engine/webrtcvideoengine2_unittest.cc:3761:
TEST_F(WebRtcVideoChannel2Test, RecvDiffUnsignalledSsrc) {
nit: "ReceiveDifferentUnsignaledSsrcs"

https://codereview.webrtc.org/2692993009/diff/1/webrtc/media/engine/webrtcvid...
webrtc/media/engine/webrtcvideoengine2_unittest.cc:3763: // Allow receiving VP8,
VP9, H264 (if enabled)
nit: Our style is to have periods even after one-line comments.

https://codereview.webrtc.org/2692993009/diff/1/webrtc/media/engine/webrtcvid...
webrtc/media/engine/webrtcvideoengine2_unittest.cc:3775: ASSERT_EQ(0u,
fake_call_->GetVideoReceiveStreams().size());
To make this test more robust, it would also be good to test that the recreated
receive streams hook up to a video renderer (which equates to a MediaStreamTrack
at the API level).

To do this, you can do:

cricket::FakeVideoRenderer renderer;
EXPECT_TRUE(channel_->SetSink(kDefaultRecvSsrc, &renderer));

(defining kDefaultRecvSsrc to 0 at the top of the file)

https://codereview.webrtc.org/2692993009/diff/1/webrtc/media/engine/webrtcvid...
webrtc/media/engine/webrtcvideoengine2_unittest.cc:3791:
EXPECT_EQ(rtpHeader.ssrc, recv_stream->GetConfig().rtp.remote_ssrc);
Then here, you can do:

recv_stream->InjectFrame(frame);
EXPECT_EQ(1, renderer.num_rendered_frames());

And same below. You can look elsewhere in this file for examples of constructing
black video frames for testing.

[mo.zanaty, 2017-02-16 02:52:40]

Description was changed from

==========
Allow default video receive channel to receive on any unsignalled SSRC,
even if it changes.
Add unit tests for changing unsignalled SSRCs.

BUG=webrtc:5208
==========

to

==========
Allow any unsignalled SSRC changes on default video receive channel.

The first unsignalled SSRC creates a default receive channel.
Any unsignalled SSRC changes after that replace the default SSRC.
Add unit tests for changing unsignalled SSRCs.

BUG=webrtc:5208
==========

[mo.zanaty, 2017-02-16 03:05:19]

On 2017/02/16 00:36:34, Taylor Brandstetter wrote:
...snip...
> Sidenote: eventually it would be good to have a fully end-to-end test for this
> (peerconnection_unittest.cc), though that would be a lot easier after I finish
> cleaning up our test infrastructure. Did you test this end-to-end in a local
> build of Chrome yet?

Yes, tested in a production video service (ciscospark.com) which changes SSRC
per speaker.

All other review comments fixed and new CL uploaded. Thanks!

[mo.zanaty, 2017-02-17 00:43:04]

https://codereview.webrtc.org/2692993009/diff/1/webrtc/media/engine/webrtcvid...
File webrtc/media/engine/webrtcvideoengine2_unittest.cc (right):

https://codereview.webrtc.org/2692993009/diff/1/webrtc/media/engine/webrtcvid...
webrtc/media/engine/webrtcvideoengine2_unittest.cc:3761:
TEST_F(WebRtcVideoChannel2Test, RecvDiffUnsignalledSsrc) {
On 2017/02/16 00:36:34, Taylor Brandstetter wrote:
> nit: "ReceiveDifferentUnsignaledSsrcs"

Done.

https://codereview.webrtc.org/2692993009/diff/1/webrtc/media/engine/webrtcvid...
webrtc/media/engine/webrtcvideoengine2_unittest.cc:3763: // Allow receiving VP8,
VP9, H264 (if enabled)
On 2017/02/16 00:36:34, Taylor Brandstetter wrote:
> nit: Our style is to have periods even after one-line comments.

Done.

https://codereview.webrtc.org/2692993009/diff/1/webrtc/media/engine/webrtcvid...
webrtc/media/engine/webrtcvideoengine2_unittest.cc:3775: ASSERT_EQ(0u,
fake_call_->GetVideoReceiveStreams().size());
On 2017/02/16 00:36:34, Taylor Brandstetter wrote:
> To make this test more robust, it would also be good to test that the
recreated
> receive streams hook up to a video renderer (which equates to a
MediaStreamTrack
> at the API level).
> 
> To do this, you can do:
> 
> cricket::FakeVideoRenderer renderer;
> EXPECT_TRUE(channel_->SetSink(kDefaultRecvSsrc, &renderer));
> 
> (defining kDefaultRecvSsrc to 0 at the top of the file)

Done.

https://codereview.webrtc.org/2692993009/diff/1/webrtc/media/engine/webrtcvid...
webrtc/media/engine/webrtcvideoengine2_unittest.cc:3791:
EXPECT_EQ(rtpHeader.ssrc, recv_stream->GetConfig().rtp.remote_ssrc);
On 2017/02/16 00:36:34, Taylor Brandstetter wrote:
> Then here, you can do:
> 
> recv_stream->InjectFrame(frame);
> EXPECT_EQ(1, renderer.num_rendered_frames());
> 
> And same below. You can look elsewhere in this file for examples of
constructing
> black video frames for testing.

Done.

[Taylor Brandstetter, 2017-02-17 02:29:31]

lgtm, though I'm not an owner.

pthatcher/mflodman, can one of you take a look?

[pthatcher1, 2017-02-17 18:10:37]

It looks good.  Could you just make one little change before I approve?

https://codereview.webrtc.org/2692993009/diff/20001/webrtc/media/engine/webrt...
File webrtc/media/engine/webrtcvideoengine2.cc (right):

https://codereview.webrtc.org/2692993009/diff/20001/webrtc/media/engine/webrt...
webrtc/media/engine/webrtcvideoengine2.cc:432:
channel->RemoveRecvStream(default_recv_ssrc_);
I know default_recv_ssrc_ is set down below, but it would be more robust to
future code changes to add this here:

default_recv_ssrc_ = 0;


Imagine, for example, if there were an early return somewhere down below.  Then
we'd end up in a broken state.

[mo.zanaty, 2017-02-17 18:29:05]

https://codereview.webrtc.org/2692993009/diff/20001/webrtc/media/engine/webrt...
File webrtc/media/engine/webrtcvideoengine2.cc (right):

https://codereview.webrtc.org/2692993009/diff/20001/webrtc/media/engine/webrt...
webrtc/media/engine/webrtcvideoengine2.cc:432:
channel->RemoveRecvStream(default_recv_ssrc_);
On 2017/02/17 18:10:37, pthatcher1 wrote:
> I know default_recv_ssrc_ is set down below, but it would be more robust to
> future code changes to add this here:
> 
> default_recv_ssrc_ = 0;
> 
> 
> Imagine, for example, if there were an early return somewhere down below. 
Then
> we'd end up in a broken state.  

Agreed. Done.

[pthatcher1, 2017-02-17 22:47:46]

lgtm

[pthatcher1, 2017-02-17 22:48:01]

The CQ bit was checked by pthatcher@webrtc.org

[pthatcher1, 2017-02-17 22:48:02]

The patchset sent to the CQ was uploaded after l-g-t-m from deadbeef@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2692993009/#ps40001 (title:
"Defensive code: Zero SSRC after removing default stream.")

[commit-bot: I haz the power, 2017-02-17 22:48:07]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2017-02-17 23:46:43]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1487371681993090,
"parent_rev": "b18af1b7d70c8018dd062b714a1a4bd041809006", "commit_rev":
"8a855d6916194d4a6941417d63c2c00dd536f6fc"}

[commit-bot: I haz the power, 2017-02-17 23:46:46]

Description was changed from

==========
Allow any unsignalled SSRC changes on default video receive channel.

The first unsignalled SSRC creates a default receive channel.
Any unsignalled SSRC changes after that replace the default SSRC.
Add unit tests for changing unsignalled SSRCs.

BUG=webrtc:5208
==========

to

==========
Allow any unsignalled SSRC changes on default video receive channel.

The first unsignalled SSRC creates a default receive channel.
Any unsignalled SSRC changes after that replace the default SSRC.
Add unit tests for changing unsignalled SSRCs.

BUG=webrtc:5208

Review-Url: https://codereview.webrtc.org/2692993009
Cr-Commit-Position: refs/heads/master@{#16682}
Committed:
https://chromium.googlesource.com/external/webrtc/+/8a855d6916194d4a6941417d6...
==========

[commit-bot: I haz the power, 2017-02-17 23:46:48]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/external/webrtc/+/8a855d6916194d4a6941417d6...