|
|
DescriptionBe 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
Patch Set 1 #
Total comments: 1
Patch Set 2 : added tests + maybe recreate streams #Patch Set 3 : fix #Patch Set 4 : improved prose #
Total comments: 7
Patch Set 5 : rebase #
Messages
Total messages: 30 (16 generated)
solenberg@webrtc.org changed reviewers: + deadbeef@webrtc.org, ossu@webrtc.org
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 ==========
solenberg@webrtc.org changed reviewers: + kwiberg@webrtc.org - ossu@webrtc.org
ossu@webrtc.org changed reviewers: + ossu@webrtc.org
Oh, so we would forcibly recreate streams when going from "default stream" state to a fully signalled one? Seems unnecessary. This lgtm.
solenberg@webrtc.org changed reviewers: - kwiberg@webrtc.org
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?
Also, it would be nice to have a bug for this in the public bug repo
PTAL
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 CQ bit was checked by solenberg@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
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.
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.
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?
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 CQ bit was checked by solenberg@webrtc.org
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")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
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 CQ bit was checked by solenberg@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
CQ is committing da patch. Bot data: {"patchset_id": 80001, "attempt_start_ts": 1487360672395280, "parent_rev": "8022d518a4249887abfdaca2686805a48bb71c3a", "commit_rev": "4904fb6f462e08da5b8ad7492781929fa5c0780c"}
Message was sent while issue was closed.
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... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/external/webrtc/+/4904fb6f462e08da5b8ad7492... |