Be less pessimistic about turning "default" receive streams into signaled streams.

Be less pessimistic about turning "default" receive streams into signaled streams.

BUG=webrtc:7179
, b/34746131

Review-Url: https://codereview.webrtc.org/2685573003
Cr-Commit-Position: refs/heads/master@{#16673}
Committed: https://chromium.googlesource.com/external/webrtc/+/4904fb6f462e08da5b8ad7492781929fa5c0780c

[the sun, 2017-02-08 10:29:57]

solenberg@webrtc.org changed reviewers:
+ deadbeef@webrtc.org, ossu@webrtc.org

[the sun, 2017-02-08 12:00:24]

Description was changed from

==========
Be less pessimistic about turning "default" receive streams into signaled
streams.

BUG=b/34746131
==========

to

==========
Be less pessimistic about turning "default" receive streams into signaled
streams.

BUG=b/34746131
==========

[the sun, 2017-02-08 12:00:24]

solenberg@webrtc.org changed reviewers:
+ kwiberg@webrtc.org
- ossu@webrtc.org

[ossu, 2017-02-08 14:32:43]

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

[ossu, 2017-02-08 14:32:43]

Oh, so we would forcibly recreate streams when going from "default stream" state
to a fully signalled one? Seems unnecessary. This lgtm.

[the sun, 2017-02-08 15:58:10]

solenberg@webrtc.org changed reviewers:
- kwiberg@webrtc.org

[Taylor Brandstetter, 2017-02-08 17:49:33]

https://codereview.webrtc.org/2685573003/diff/1/webrtc/media/engine/webrtcvoi...
File webrtc/media/engine/webrtcvoiceengine.cc (right):

https://codereview.webrtc.org/2685573003/diff/1/webrtc/media/engine/webrtcvoi...
webrtc/media/engine/webrtcvoiceengine.cc:2278: default_recv_ssrc_ = -1;
If you do this, does the default stream get sp.sync_label and end up synced with
video?

[Taylor Brandstetter, 2017-02-08 17:52:09]

Also, it would be nice to have a bug for this in the public bug repo

[the sun, 2017-02-17 13:08:02]

PTAL

[the sun, 2017-02-17 13:08:13]

Description was changed from

==========
Be less pessimistic about turning "default" receive streams into signaled
streams.

BUG=b/34746131
==========

to

==========
Be less pessimistic about turning "default" receive streams into signaled
streams.

BUG=webrtc:7179, b/34746131
==========

[the sun, 2017-02-17 13:15:57]

The CQ bit was checked by solenberg@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-17 13:16:05]

Dry run: 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 16:22:23]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-17 16:22:24]

Dry run: This issue passed the CQ dry run.

[Taylor Brandstetter, 2017-02-17 16:24:51]

lgtm, though not an owner

https://codereview.webrtc.org/2685573003/diff/60001/webrtc/media/engine/webrt...
File webrtc/media/engine/webrtcvoiceengine_unittest.cc (right):

https://codereview.webrtc.org/2685573003/diff/60001/webrtc/media/engine/webrt...
webrtc/media/engine/webrtcvoiceengine_unittest.cc:2820:
TEST_F(WebRtcVoiceEngineTestFake, AddRecvStreamAfterUnsignaled_NoRecreate) {
nit: I'd call this 

AddRecvStreamAfterUnsignaledNotRecreatedIfParamsNotChanged, and the other
AddRecvStreamAfterUnsignaledRecreatedIfParamsChanged. But that may just be my
preference, I like test names that are as descriptive as possible within 80
chars.

https://codereview.webrtc.org/2685573003/diff/60001/webrtc/media/engine/webrt...
webrtc/media/engine/webrtcvoiceengine_unittest.cc:2824:
DeliverPacket(kPcmuFrame, sizeof(kPcmuFrame));
This test relies on the fact that kPcmuFrame uses SSRC 1. Similar to in your
other CL, I'd suggest either having a "MakePcmuFrame" method that takes an SSRC,
or rename it to "kPcmuFrameWithSsrcX", where "kSsrcX" is equal to the SSRC in
it.

[ossu, 2017-02-17 17:05:10]

I'll lgtm you again because it's Friday. :)

Looks good!

https://codereview.webrtc.org/2685573003/diff/60001/webrtc/media/engine/webrt...
File webrtc/media/engine/webrtcvoiceengine_unittest.cc (right):

https://codereview.webrtc.org/2685573003/diff/60001/webrtc/media/engine/webrt...
webrtc/media/engine/webrtcvoiceengine_unittest.cc:2820:
TEST_F(WebRtcVoiceEngineTestFake, AddRecvStreamAfterUnsignaled_NoRecreate) {
On 2017/02/17 16:24:51, Taylor Brandstetter wrote:
> nit: I'd call this 
> 
> AddRecvStreamAfterUnsignaledNotRecreatedIfParamsNotChanged, and the other
> AddRecvStreamAfterUnsignaledRecreatedIfParamsChanged. But that may just be my
> preference, I like test names that are as descriptive as possible within 80
> chars.

Not sure if we do this in other places, but I'd find it much easier to read
these long names if there were a _ separating the "group"
AddRecvStreamAfterUnsignaled and the rest (like in the current name). Other than
that, the extra clarity is good.

[the sun, 2017-02-17 17:19:09]

https://codereview.webrtc.org/2685573003/diff/60001/webrtc/media/engine/webrt...
File webrtc/media/engine/webrtcvoiceengine_unittest.cc (right):

https://codereview.webrtc.org/2685573003/diff/60001/webrtc/media/engine/webrt...
webrtc/media/engine/webrtcvoiceengine_unittest.cc:2820:
TEST_F(WebRtcVoiceEngineTestFake, AddRecvStreamAfterUnsignaled_NoRecreate) {
On 2017/02/17 17:05:10, ossu wrote:
> On 2017/02/17 16:24:51, Taylor Brandstetter wrote:
> > nit: I'd call this 
> > 
> > AddRecvStreamAfterUnsignaledNotRecreatedIfParamsNotChanged, and the other
> > AddRecvStreamAfterUnsignaledRecreatedIfParamsChanged. But that may just be
my
> > preference, I like test names that are as descriptive as possible within 80
> > chars.
> 
> Not sure if we do this in other places, but I'd find it much easier to read
> these long names if there were a _ separating the "group"
> AddRecvStreamAfterUnsignaled and the rest (like in the current name). Other
than
> that, the extra clarity is good.

Uhm. I think super long names are not very readable. In this case the extra
information is offset by natural language parsing difficulties. :)

The important bit in these name(s) to me is "AddRecvStreamAfterUnsignaled". I
need to nevertheless look up the test case to see what exactly is going wrong (I
don't spend much time reading through test logs for tests that succeed). Then
the extra long name just makes it harder to remember when I switch from shell to
editor.

So, unless you can come up with a strong argument, I'll keep the current names.

https://codereview.webrtc.org/2685573003/diff/60001/webrtc/media/engine/webrt...
webrtc/media/engine/webrtcvoiceengine_unittest.cc:2824:
DeliverPacket(kPcmuFrame, sizeof(kPcmuFrame));
On 2017/02/17 16:24:51, Taylor Brandstetter wrote:
> This test relies on the fact that kPcmuFrame uses SSRC 1. Similar to in your
> other CL, I'd suggest either having a "MakePcmuFrame" method that takes an
SSRC,
> or rename it to "kPcmuFrameWithSsrcX", where "kSsrcX" is equal to the SSRC in
> it.

I think I'll instead make a helper:

void DeliverPcmuFrame(uint32_t ssrc);

...but do that cleanup in a separate CL. Ok?

[Taylor Brandstetter, 2017-02-17 17:53:44]

https://codereview.webrtc.org/2685573003/diff/60001/webrtc/media/engine/webrt...
File webrtc/media/engine/webrtcvoiceengine_unittest.cc (right):

https://codereview.webrtc.org/2685573003/diff/60001/webrtc/media/engine/webrt...
webrtc/media/engine/webrtcvoiceengine_unittest.cc:2820:
TEST_F(WebRtcVoiceEngineTestFake, AddRecvStreamAfterUnsignaled_NoRecreate) {
On 2017/02/17 17:19:09, the sun wrote:
> On 2017/02/17 17:05:10, ossu wrote:
> > On 2017/02/17 16:24:51, Taylor Brandstetter wrote:
> > > nit: I'd call this 
> > > 
> > > AddRecvStreamAfterUnsignaledNotRecreatedIfParamsNotChanged, and the other
> > > AddRecvStreamAfterUnsignaledRecreatedIfParamsChanged. But that may just be
> my
> > > preference, I like test names that are as descriptive as possible within
80
> > > chars.
> > 
> > Not sure if we do this in other places, but I'd find it much easier to read
> > these long names if there were a _ separating the "group"
> > AddRecvStreamAfterUnsignaled and the rest (like in the current name). Other
> than
> > that, the extra clarity is good.
> 
> Uhm. I think super long names are not very readable. In this case the extra
> information is offset by natural language parsing difficulties. :)
> 
> The important bit in these name(s) to me is "AddRecvStreamAfterUnsignaled". I
> need to nevertheless look up the test case to see what exactly is going wrong
(I
> don't spend much time reading through test logs for tests that succeed). Then
> the extra long name just makes it harder to remember when I switch from shell
to
> editor.
> 
> So, unless you can come up with a strong argument, I'll keep the current
names.

Yeah, that's fine.

https://codereview.webrtc.org/2685573003/diff/60001/webrtc/media/engine/webrt...
webrtc/media/engine/webrtcvoiceengine_unittest.cc:2824:
DeliverPacket(kPcmuFrame, sizeof(kPcmuFrame));
On 2017/02/17 17:19:09, the sun wrote:
> On 2017/02/17 16:24:51, Taylor Brandstetter wrote:
> > This test relies on the fact that kPcmuFrame uses SSRC 1. Similar to in your
> > other CL, I'd suggest either having a "MakePcmuFrame" method that takes an
> SSRC,
> > or rename it to "kPcmuFrameWithSsrcX", where "kSsrcX" is equal to the SSRC
in
> > it.
> 
> I think I'll instead make a helper:
> 
> void DeliverPcmuFrame(uint32_t ssrc);
> 
> ...but do that cleanup in a separate CL. Ok?

Sounds good.

[the sun, 2017-02-17 18:30:22]

The CQ bit was checked by solenberg@webrtc.org

[the sun, 2017-02-17 18:30:23]

The patchset sent to the CQ was uploaded after l-g-t-m from ossu@webrtc.org,
deadbeef@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2685573003/#ps80001 (title:
"rebase")

[commit-bot: I haz the power, 2017-02-17 18:30:31]

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 19:09:21]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-17 19:09:22]

Try jobs failed on following builders:
  win_baremetal on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/win_baremetal/builds/18771)

[the sun, 2017-02-17 19:44:32]

The CQ bit was checked by solenberg@webrtc.org

[commit-bot: I haz the power, 2017-02-17 19:44:39]

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 20:01:15]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1487360672395280,
"parent_rev": "8022d518a4249887abfdaca2686805a48bb71c3a", "commit_rev":
"4904fb6f462e08da5b8ad7492781929fa5c0780c"}

[commit-bot: I haz the power, 2017-02-17 20:01:19]

Description was changed from

==========
Be less pessimistic about turning "default" receive streams into signaled
streams.

BUG=webrtc:7179, b/34746131
==========

to

==========
Be less pessimistic about turning "default" receive streams into signaled
streams.

BUG=webrtc:7179, b/34746131

Review-Url: https://codereview.webrtc.org/2685573003
Cr-Commit-Position: refs/heads/master@{#16673}
Committed:
https://chromium.googlesource.com/external/webrtc/+/4904fb6f462e08da5b8ad7492...
==========

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

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/external/webrtc/+/4904fb6f462e08da5b8ad7492...