Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2685893002:
Support N unsignaled audio streams (Closed)
DescriptionSupport N unsignaled audio streams.
BUG=webrtc:7175, webrtc:5208
Review-Url: https://codereview.webrtc.org/2685893002
Cr-Commit-Position: refs/heads/master@{#16952}
Committed: https://chromium.googlesource.com/external/webrtc/+/2100c0ba13d8d121f88fb16e48246c333873f2e2
Patch Set 1 #Patch Set 2 : working #Patch Set 3 : bad comment #
Total comments: 18
Patch Set 4 : comments #
Total comments: 7
Patch Set 5 : rebase #Patch Set 6 : gs #Patch Set 7 : more comments #Patch Set 8 : rebase #Patch Set 9 : add histogram for # unsignaled audio streams #
Messages
Total messages: 37 (20 generated)
Description was changed from ========== WIP BUG=b/34746131 ========== to ========== Support N unsignaled audio streams. BUG=webrtc:7175, webrtc:5208, b/34746131 ==========
solenberg@webrtc.org changed reviewers: + deadbeef@webrtc.org
Description was changed from ========== Support N unsignaled audio streams. BUG=webrtc:7175, webrtc:5208, b/34746131 ========== to ========== Support N unsignaled audio streams. BUG=webrtc:7175, webrtc:5208 ==========
https://codereview.webrtc.org/2685893002/diff/40001/webrtc/media/engine/webrt... File webrtc/media/engine/webrtcvoiceengine.cc (right): https://codereview.webrtc.org/2685893002/diff/40001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvoiceengine.cc:2502: // Only set audio sink for first unsignaled stream. Is this ok? And should there be a test for this? https://codereview.webrtc.org/2685893002/diff/40001/webrtc/media/engine/webrt... File webrtc/media/engine/webrtcvoiceengine.h (left): https://codereview.webrtc.org/2685893002/diff/40001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvoiceengine.h:182: bool AddRecvStream(const StreamParams& sp) override; It would be good to document this new behavior in a comment somewhere. I'd suggest above AddRecvStream here, or in mediachannel.h. https://codereview.webrtc.org/2685893002/diff/40001/webrtc/media/engine/webrt... File webrtc/media/engine/webrtcvoiceengine.h (right): https://codereview.webrtc.org/2685893002/diff/40001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvoiceengine.h:239: bool TryDeregisterUnsignaledRecvStream(uint32_t ssrc); nit: "Try" sounds like something that might fail. I'd use "Maybe" instead. Also, a brief comment above its declaration would be good. https://codereview.webrtc.org/2685893002/diff/40001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvoiceengine.h:261: // Volume for unsignaled stream, which may be set before the stream exists. nit: "stream(s)" now? https://codereview.webrtc.org/2685893002/diff/40001/webrtc/media/engine/webrt... File webrtc/media/engine/webrtcvoiceengine_unittest.cc (right): https://codereview.webrtc.org/2685893002/diff/40001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvoiceengine_unittest.cc:2775: // Send on the same SSRCs again should not create new streams. nit: "Sending" https://codereview.webrtc.org/2685893002/diff/40001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvoiceengine_unittest.cc:2800: EXPECT_EQ(kMaxUnsignaledCount, i); What's the purpose of the final ++i and EXPECT_EQ? Is it a just a sanity check that every stream was counted? If so could you leave a comment saying that? https://codereview.webrtc.org/2685893002/diff/40001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvoiceengine_unittest.cc:2805: TEST_F(WebRtcVoiceEngineTestFake, RecvUnsignaledAfterSignalled) { nit: This uses both "Unsignaled" and "Unsignalled" https://codereview.webrtc.org/2685893002/diff/40001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvoiceengine_unittest.cc:3330: DeliverPacket(kPcmuFrame, sizeof(kPcmuFrame)); nit: May be more readable if "kPcmuFrame2" was delivered after "kPcmuFrame"
https://codereview.webrtc.org/2685893002/diff/40001/webrtc/media/engine/webrt... File webrtc/media/engine/webrtcvoiceengine.cc (right): https://codereview.webrtc.org/2685893002/diff/40001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvoiceengine.cc:2502: // Only set audio sink for first unsignaled stream. On 2017/02/17 08:36:14, Taylor Brandstetter wrote: > Is this ok? And should there be a test for this? A) No. I changed it to adding the default sink to the latest unsignaled stream. That way we will support endpoints switching SSRC for a (logically) single unsignaled stream, e.g. webrtc:5208. B) Now there is. https://codereview.webrtc.org/2685893002/diff/40001/webrtc/media/engine/webrt... File webrtc/media/engine/webrtcvoiceengine.h (left): https://codereview.webrtc.org/2685893002/diff/40001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvoiceengine.h:182: bool AddRecvStream(const StreamParams& sp) override; On 2017/02/17 08:36:14, Taylor Brandstetter wrote: > It would be good to document this new behavior in a comment somewhere. I'd > suggest above AddRecvStream here, or in mediachannel.h. Not sure that's the right place. Above OnPacketReceived()? https://codereview.webrtc.org/2685893002/diff/40001/webrtc/media/engine/webrt... File webrtc/media/engine/webrtcvoiceengine.h (right): https://codereview.webrtc.org/2685893002/diff/40001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvoiceengine.h:239: bool TryDeregisterUnsignaledRecvStream(uint32_t ssrc); On 2017/02/17 08:36:14, Taylor Brandstetter wrote: > nit: "Try" sounds like something that might fail. I'd use "Maybe" instead. Also, > a brief comment above its declaration would be good. "Maybe" is fine with me. https://codereview.webrtc.org/2685893002/diff/40001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvoiceengine.h:261: // Volume for unsignaled stream, which may be set before the stream exists. On 2017/02/17 08:36:14, Taylor Brandstetter wrote: > nit: "stream(s)" now? Done. https://codereview.webrtc.org/2685893002/diff/40001/webrtc/media/engine/webrt... File webrtc/media/engine/webrtcvoiceengine_unittest.cc (right): https://codereview.webrtc.org/2685893002/diff/40001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvoiceengine_unittest.cc:2775: // Send on the same SSRCs again should not create new streams. On 2017/02/17 08:36:15, Taylor Brandstetter wrote: > nit: "Sending" Done. https://codereview.webrtc.org/2685893002/diff/40001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvoiceengine_unittest.cc:2800: EXPECT_EQ(kMaxUnsignaledCount, i); On 2017/02/17 08:36:15, Taylor Brandstetter wrote: > What's the purpose of the final ++i and EXPECT_EQ? Is it a just a sanity check > that every stream was counted? If so could you leave a comment saying that? Yes, basically to "complete" the loop https://codereview.webrtc.org/2685893002/diff/40001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvoiceengine_unittest.cc:2805: TEST_F(WebRtcVoiceEngineTestFake, RecvUnsignaledAfterSignalled) { On 2017/02/17 08:36:15, Taylor Brandstetter wrote: > nit: This uses both "Unsignaled" and "Unsignalled" Done. https://codereview.webrtc.org/2685893002/diff/40001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvoiceengine_unittest.cc:3330: DeliverPacket(kPcmuFrame, sizeof(kPcmuFrame)); On 2017/02/17 08:36:14, Taylor Brandstetter wrote: > nit: May be more readable if "kPcmuFrame2" was delivered after "kPcmuFrame" Done.
lgtm % comments https://codereview.webrtc.org/2685893002/diff/40001/webrtc/media/engine/webrt... File webrtc/media/engine/webrtcvoiceengine.h (left): https://codereview.webrtc.org/2685893002/diff/40001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvoiceengine.h:182: bool AddRecvStream(const StreamParams& sp) override; On 2017/02/17 10:10:57, the sun wrote: > On 2017/02/17 08:36:14, Taylor Brandstetter wrote: > > It would be good to document this new behavior in a comment somewhere. I'd > > suggest above AddRecvStream here, or in mediachannel.h. > > Not sure that's the right place. Above OnPacketReceived()? I realize now I was thinking of "SetOutputVolume". That's what gets called with a special SSRC of 0, which represents "all unsignaled streams". Anyway, wherever you think is best. https://codereview.webrtc.org/2685893002/diff/60001/webrtc/media/engine/webrt... File webrtc/media/engine/webrtcvoiceengine.h (right): https://codereview.webrtc.org/2685893002/diff/60001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvoiceengine.h:240: // unsignaled anymore (i.e. it is now removed, or signaled). nit: add "and return true" https://codereview.webrtc.org/2685893002/diff/60001/webrtc/media/engine/webrt... File webrtc/media/engine/webrtcvoiceengine_unittest.cc (right): https://codereview.webrtc.org/2685893002/diff/60001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvoiceengine_unittest.cc:141: return AddRecvStream(kSsrc9); What was the motivation for changing to kSsrc9? I think it was ok before. To me, "kSsrc1" just means "an SSRC that's not equal to kSsrc2, kSsrc3 or kSsrc4". https://codereview.webrtc.org/2685893002/diff/60001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvoiceengine_unittest.cc:2702: rtc::SetBE32(packets[i] + 8, static_cast<uint32_t>(i)); This test puts the literal values 1, 2 and 3 in the packets, so it shouldn't be relying on constants defined outside of the test (kSsrc1/kSsrc2/kSsrc3). Or, it should be using kSsrc1/kSsrc2/kSsrc3 in rtc::SetBE32.
https://codereview.webrtc.org/2685893002/diff/40001/webrtc/media/engine/webrt... File webrtc/media/engine/webrtcvoiceengine.h (left): https://codereview.webrtc.org/2685893002/diff/40001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvoiceengine.h:182: bool AddRecvStream(const StreamParams& sp) override; On 2017/02/17 10:34:46, Taylor Brandstetter wrote: > On 2017/02/17 10:10:57, the sun wrote: > > On 2017/02/17 08:36:14, Taylor Brandstetter wrote: > > > It would be good to document this new behavior in a comment somewhere. I'd > > > suggest above AddRecvStream here, or in mediachannel.h. > > > > Not sure that's the right place. Above OnPacketReceived()? > > I realize now I was thinking of "SetOutputVolume". That's what gets called with > a special SSRC of 0, which represents "all unsignaled streams". Anyway, wherever > you think is best. Done. https://codereview.webrtc.org/2685893002/diff/60001/webrtc/media/engine/webrt... File webrtc/media/engine/webrtcvoiceengine.h (right): https://codereview.webrtc.org/2685893002/diff/60001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvoiceengine.h:240: // unsignaled anymore (i.e. it is now removed, or signaled). On 2017/02/17 10:34:46, Taylor Brandstetter wrote: > nit: add "and return true" Done. https://codereview.webrtc.org/2685893002/diff/60001/webrtc/media/engine/webrt... File webrtc/media/engine/webrtcvoiceengine_unittest.cc (right): https://codereview.webrtc.org/2685893002/diff/60001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvoiceengine_unittest.cc:141: return AddRecvStream(kSsrc9); On 2017/02/17 10:34:46, Taylor Brandstetter wrote: > What was the motivation for changing to kSsrc9? I think it was ok before. To me, > "kSsrc1" just means "an SSRC that's not equal to kSsrc2, kSsrc3 or kSsrc4". kPcmuFrame has hard coded SSRC=1, so it is sometimes convenient to test explicitly against that. I've renamed these "arbitrary" test SSRCs to kSsrcX, kSsrcY, kSsrcZ instead, and added a constant for kSsrc0. Hopefully it makes the tests a little easier to read. https://codereview.webrtc.org/2685893002/diff/60001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvoiceengine_unittest.cc:2702: rtc::SetBE32(packets[i] + 8, static_cast<uint32_t>(i)); On 2017/02/17 10:34:46, Taylor Brandstetter wrote: > This test puts the literal values 1, 2 and 3 in the packets, so it shouldn't be > relying on constants defined outside of the test (kSsrc1/kSsrc2/kSsrc3). Or, it > should be using kSsrc1/kSsrc2/kSsrc3 in rtc::SetBE32. Oh, well spotted. Reverted. Thanks!
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.
On 2017/02/17 11:22:47, the sun wrote: > https://codereview.webrtc.org/2685893002/diff/40001/webrtc/media/engine/webrt... > File webrtc/media/engine/webrtcvoiceengine.h (left): > > https://codereview.webrtc.org/2685893002/diff/40001/webrtc/media/engine/webrt... > webrtc/media/engine/webrtcvoiceengine.h:182: bool AddRecvStream(const > StreamParams& sp) override; > On 2017/02/17 10:34:46, Taylor Brandstetter wrote: > > On 2017/02/17 10:10:57, the sun wrote: > > > On 2017/02/17 08:36:14, Taylor Brandstetter wrote: > > > > It would be good to document this new behavior in a comment somewhere. I'd > > > > suggest above AddRecvStream here, or in mediachannel.h. > > > > > > Not sure that's the right place. Above OnPacketReceived()? > > > > I realize now I was thinking of "SetOutputVolume". That's what gets called > with > > a special SSRC of 0, which represents "all unsignaled streams". Anyway, > wherever > > you think is best. > > Done. > > https://codereview.webrtc.org/2685893002/diff/60001/webrtc/media/engine/webrt... > File webrtc/media/engine/webrtcvoiceengine.h (right): > > https://codereview.webrtc.org/2685893002/diff/60001/webrtc/media/engine/webrt... > webrtc/media/engine/webrtcvoiceengine.h:240: // unsignaled anymore (i.e. it is > now removed, or signaled). > On 2017/02/17 10:34:46, Taylor Brandstetter wrote: > > nit: add "and return true" > > Done. > > https://codereview.webrtc.org/2685893002/diff/60001/webrtc/media/engine/webrt... > File webrtc/media/engine/webrtcvoiceengine_unittest.cc (right): > > https://codereview.webrtc.org/2685893002/diff/60001/webrtc/media/engine/webrt... > webrtc/media/engine/webrtcvoiceengine_unittest.cc:141: return > AddRecvStream(kSsrc9); > On 2017/02/17 10:34:46, Taylor Brandstetter wrote: > > What was the motivation for changing to kSsrc9? I think it was ok before. To > me, > > "kSsrc1" just means "an SSRC that's not equal to kSsrc2, kSsrc3 or kSsrc4". > > kPcmuFrame has hard coded SSRC=1, so it is sometimes convenient to test > explicitly against that. I've renamed these "arbitrary" test SSRCs to kSsrcX, > kSsrcY, kSsrcZ instead, and added a constant for kSsrc0. Hopefully it makes the > tests a little easier to read. > > https://codereview.webrtc.org/2685893002/diff/60001/webrtc/media/engine/webrt... > webrtc/media/engine/webrtcvoiceengine_unittest.cc:2702: rtc::SetBE32(packets[i] > + 8, static_cast<uint32_t>(i)); > On 2017/02/17 10:34:46, Taylor Brandstetter wrote: > > This test puts the literal values 1, 2 and 3 in the packets, so it shouldn't > be > > relying on constants defined outside of the test (kSsrc1/kSsrc2/kSsrc3). Or, > it > > should be using kSsrc1/kSsrc2/kSsrc3 in rtc::SetBE32. > > Oh, well spotted. Reverted. Thanks! Let me know if this still LGTY...
https://codereview.webrtc.org/2685893002/diff/60001/webrtc/media/engine/webrt... File webrtc/media/engine/webrtcvoiceengine_unittest.cc (right): https://codereview.webrtc.org/2685893002/diff/60001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvoiceengine_unittest.cc:141: return AddRecvStream(kSsrc9); On 2017/02/17 11:22:47, the sun wrote: > On 2017/02/17 10:34:46, Taylor Brandstetter wrote: > > What was the motivation for changing to kSsrc9? I think it was ok before. To > me, > > "kSsrc1" just means "an SSRC that's not equal to kSsrc2, kSsrc3 or kSsrc4". > > kPcmuFrame has hard coded SSRC=1, so it is sometimes convenient to test > explicitly against that. I've renamed these "arbitrary" test SSRCs to kSsrcX, > kSsrcY, kSsrcZ instead, and added a constant for kSsrc0. Hopefully it makes the > tests a little easier to read. I'd suggest fixing that problem by either renaming "kPcmuFrame" to "kPcmuFrameWithSsrc1", so it's clear that tests are depending on that behavior, or call a "MakePcmuFrame" method instead of using kPcmuFrame directly.
On 2017/02/17 16:10:53, Taylor Brandstetter wrote: > https://codereview.webrtc.org/2685893002/diff/60001/webrtc/media/engine/webrt... > File webrtc/media/engine/webrtcvoiceengine_unittest.cc (right): > > https://codereview.webrtc.org/2685893002/diff/60001/webrtc/media/engine/webrt... > webrtc/media/engine/webrtcvoiceengine_unittest.cc:141: return > AddRecvStream(kSsrc9); > On 2017/02/17 11:22:47, the sun wrote: > > On 2017/02/17 10:34:46, Taylor Brandstetter wrote: > > > What was the motivation for changing to kSsrc9? I think it was ok before. To > > me, > > > "kSsrc1" just means "an SSRC that's not equal to kSsrc2, kSsrc3 or kSsrc4". > > > > kPcmuFrame has hard coded SSRC=1, so it is sometimes convenient to test > > explicitly against that. I've renamed these "arbitrary" test SSRCs to kSsrcX, > > kSsrcY, kSsrcZ instead, and added a constant for kSsrc0. Hopefully it makes > the > > tests a little easier to read. > > I'd suggest fixing that problem by either renaming "kPcmuFrame" to > "kPcmuFrameWithSsrc1", so it's clear that tests are depending on that behavior, > or call a "MakePcmuFrame" method instead of using kPcmuFrame directly. Would you like me to revert the renaming? (I think this is an improvement regardless, so I'd rather not...)
On 2017/02/17 17:07:00, the sun wrote: > On 2017/02/17 16:10:53, Taylor Brandstetter wrote: > > > https://codereview.webrtc.org/2685893002/diff/60001/webrtc/media/engine/webrt... > > File webrtc/media/engine/webrtcvoiceengine_unittest.cc (right): > > > > > https://codereview.webrtc.org/2685893002/diff/60001/webrtc/media/engine/webrt... > > webrtc/media/engine/webrtcvoiceengine_unittest.cc:141: return > > AddRecvStream(kSsrc9); > > On 2017/02/17 11:22:47, the sun wrote: > > > On 2017/02/17 10:34:46, Taylor Brandstetter wrote: > > > > What was the motivation for changing to kSsrc9? I think it was ok before. > To > > > me, > > > > "kSsrc1" just means "an SSRC that's not equal to kSsrc2, kSsrc3 or > kSsrc4". > > > > > > kPcmuFrame has hard coded SSRC=1, so it is sometimes convenient to test > > > explicitly against that. I've renamed these "arbitrary" test SSRCs to > kSsrcX, > > > kSsrcY, kSsrcZ instead, and added a constant for kSsrc0. Hopefully it makes > > the > > > tests a little easier to read. > > > > I'd suggest fixing that problem by either renaming "kPcmuFrame" to > > "kPcmuFrameWithSsrc1", so it's clear that tests are depending on that > behavior, > > or call a "MakePcmuFrame" method instead of using kPcmuFrame directly. > > Would you like me to revert the renaming? (I think this is an improvement > regardless, so I'd rather not...) and, would you like that fix in this CL?
On 2017/02/17 17:07:41, the sun wrote: > On 2017/02/17 17:07:00, the sun wrote: > > Would you like me to revert the renaming? (I think this is an improvement > > regardless, so I'd rather not...) > > and, would you like that fix in this CL? You can decide; I don't feel too strongly about the "kSsrc9" renaming. But I do think the assumption that "kPcmuFrame" uses a specific SSRC needs to be made more explicit somehow.
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/...
On 2017/02/17 17:27:38, Taylor Brandstetter wrote: > On 2017/02/17 17:07:41, the sun wrote: > > On 2017/02/17 17:07:00, the sun wrote: > > > Would you like me to revert the renaming? (I think this is an improvement > > > regardless, so I'd rather not...) > > > > and, would you like that fix in this CL? > > You can decide; I don't feel too strongly about the "kSsrc9" renaming. But I do > think the assumption that "kPcmuFrame" uses a specific SSRC needs to be made > more explicit somehow. I'll address that separately, as mentioned in the other default streams CL.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2017/02/17 20:45:33, the sun wrote: > On 2017/02/17 17:27:38, Taylor Brandstetter wrote: > > On 2017/02/17 17:07:41, the sun wrote: > > > On 2017/02/17 17:07:00, the sun wrote: > > > > Would you like me to revert the renaming? (I think this is an improvement > > > > regardless, so I'd rather not...) > > > > > > and, would you like that fix in this CL? > > > > You can decide; I don't feel too strongly about the "kSsrc9" renaming. But I > do > > think the assumption that "kPcmuFrame" uses a specific SSRC needs to be made > > more explicit somehow. > > I'll address that separately, as mentioned in the other default streams CL. FYI, getstats calls will also produce warnings for unsignaled receive streams: https://cs.chromium.org/chromium/src/third_party/webrtc/pc/statscollector.cc?... So if we allow more, we'll get a lot more warning spam.
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/...
solenberg@webrtc.org changed reviewers: + tommi@webrtc.org
On 2017/02/24 18:44:33, noahric wrote: > On 2017/02/17 20:45:33, the sun wrote: > > On 2017/02/17 17:27:38, Taylor Brandstetter wrote: > > > On 2017/02/17 17:07:41, the sun wrote: > > > > On 2017/02/17 17:07:00, the sun wrote: > > > > > Would you like me to revert the renaming? (I think this is an > improvement > > > > > regardless, so I'd rather not...) > > > > > > > > and, would you like that fix in this CL? > > > > > > You can decide; I don't feel too strongly about the "kSsrc9" renaming. But I > > do > > > think the assumption that "kPcmuFrame" uses a specific SSRC needs to be made > > > more explicit somehow. > > > > I'll address that separately, as mentioned in the other default streams CL. > > FYI, getstats calls will also produce warnings for unsignaled receive streams: > https://cs.chromium.org/chromium/src/third_party/webrtc/pc/statscollector.cc?... > > So if we allow more, we'll get a lot more warning spam. Tommi, could you take a look at the last patch set; does this look like a good stat to you?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2017/02/28 22:03:40, the sun wrote: > On 2017/02/24 18:44:33, noahric wrote: > > On 2017/02/17 20:45:33, the sun wrote: > > > On 2017/02/17 17:27:38, Taylor Brandstetter wrote: > > > > On 2017/02/17 17:07:41, the sun wrote: > > > > > On 2017/02/17 17:07:00, the sun wrote: > > > > > > Would you like me to revert the renaming? (I think this is an > > improvement > > > > > > regardless, so I'd rather not...) > > > > > > > > > > and, would you like that fix in this CL? > > > > > > > > You can decide; I don't feel too strongly about the "kSsrc9" renaming. But > I > > > do > > > > think the assumption that "kPcmuFrame" uses a specific SSRC needs to be > made > > > > more explicit somehow. > > > > > > I'll address that separately, as mentioned in the other default streams CL. > > > > FYI, getstats calls will also produce warnings for unsignaled receive streams: > > > https://cs.chromium.org/chromium/src/third_party/webrtc/pc/statscollector.cc?... > > > > So if we allow more, we'll get a lot more warning spam. > > Tommi, could you take a look at the last patch set; does this look like a good > stat to you? I think so yes, but I don't think I have all the context. But fwiw rs lgtm.
The CQ bit was checked by solenberg@webrtc.org
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/2685893002/#ps160001 (title: "add histogram for # unsignaled audio streams")
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": 160001, "attempt_start_ts": 1488396351013230,
"parent_rev": "1f50daeb801677c51541b7f02efb7af9b6179757", "commit_rev":
"2100c0ba13d8d121f88fb16e48246c333873f2e2"}
Message was sent while issue was closed.
Description was changed from ========== Support N unsignaled audio streams. BUG=webrtc:7175, webrtc:5208 ========== to ========== Support N unsignaled audio streams. BUG=webrtc:7175, webrtc:5208 Review-Url: https://codereview.webrtc.org/2685893002 Cr-Commit-Position: refs/heads/master@{#16952} Committed: https://chromium.googlesource.com/external/webrtc/+/2100c0ba13d8d121f88fb16e4... ==========
Message was sent while issue was closed.
Committed patchset #9 (id:160001) as https://chromium.googlesource.com/external/webrtc/+/2100c0ba13d8d121f88fb16e4... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
