Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2920993002:
Add RSID-based demuxing to RtpDemuxer (Closed)
|
Created:
3 years, 6 months ago by eladalon Modified:
3 years, 6 months ago CC:
webrtc-reviews_webrtc.org, interface-changes_webrtc.org, yujie_mao (webrtc), stefan-webrtc, tterriberry_mozilla.com, qiang.lu, niklas.enbom, peah-webrtc, the sun, mflodman Target Ref:
refs/heads/master Project:
webrtc Visibility:
Public. |
DescriptionAdd RSID-based demuxing to RtpDemuxer
Make RtpDemuxer able to demux RTP packets according to RSID (RTP Stream ID), as well as the (pre-existing) ability to demux according to SSRC.
BUG=None
Review-Url: https://codereview.webrtc.org/2920993002
Cr-Commit-Position: refs/heads/master@{#18495}
Committed: https://chromium.googlesource.com/external/webrtc/+/d0244c21cdb2985df9abec84a0aed6d0d0cbeec0
Patch Set 1 #
Total comments: 39
Patch Set 2 : Response to CR #
Total comments: 28
Patch Set 3 : 1. Response to CR. 2. More tests. 3. RemoveSink's return type changed. 4. Some UT changes. #Patch Set 4 : Remove forgotten TODO #
Total comments: 7
Patch Set 5 : . #Patch Set 6 : Missed one cast. #
Messages
Total messages: 26 (11 generated)
Description was changed from ========== Add RSID-based demuxing to RtpDemuxer Make RtpDemuxer able to demux RTP packets according to RSID (RTP Stream ID), as well as the (pre-existing) ability to demux according to SSRC. BUG=None ========== to ========== Add RSID-based demuxing to RtpDemuxer Make RtpDemuxer able to demux RTP packets according to RSID (RTP Stream ID), as well as the (pre-existing) ability to demux according to SSRC. BUG=None ==========
eladalon@webrtc.org changed reviewers: + danilchap@webrtc.org, nisse@webrtc.org, stefan@webrtc.org
PTAL
danilchap@webrtc.org changed reviewers: + danilchap@webrtc.org
took a overview look, looks good. Will take a deeper look on Monday https://codereview.webrtc.org/2920993002/diff/1/webrtc/call/rtp_demuxer_unitt... File webrtc/call/rtp_demuxer_unittest.cc (right): https://codereview.webrtc.org/2920993002/diff/1/webrtc/call/rtp_demuxer_unitt... webrtc/call/rtp_demuxer_unittest.cc:54: rtp::Packet::ExtensionManager extensions_manager(extensions); why not just RtpPacketReceieved::ExtensionManager extensions; extensions.Register<webrtc::RtpStreamId>(kRsidExtensionId); auto packet = rtc::MakeUnique<RtpPacketReceived>(&extenions); if (rsid != nullptr) { // or if (!rsid.empty()) packet->SetExtension<webrtc::RtpStreamId>(*rsid); } https://codereview.webrtc.org/2920993002/diff/1/webrtc/call/rtp_demuxer_unitt... webrtc/call/rtp_demuxer_unittest.cc:65: RtpDemuxerTest() = default; feel free to omit default constructor/destructor for class declared in .cc file https://codereview.webrtc.org/2920993002/diff/1/webrtc/call/rtp_demuxer_unitt... webrtc/call/rtp_demuxer_unittest.cc:68: RtpDemuxer demuxer; fixture that small easy not to use at all: just an extra line per test, but you demonstrate that RtpDemuxer doesn't need any complicated setup. https://codereview.webrtc.org/2920993002/diff/1/webrtc/call/rtp_demuxer_unitt... webrtc/call/rtp_demuxer_unittest.cc:80: EXPECT_CALL(sinks[i], OnRtpPacket(SamePacketAs(*packet))).Times(1); .Times(1) can by omitted if you wish https://github.com/google/googletest/blob/master/googlemock/docs/ForDummies.m... https://codereview.webrtc.org/2920993002/diff/1/webrtc/call/rtp_demuxer_unitt... webrtc/call/rtp_demuxer_unittest.cc:81: EXPECT_EQ(demuxer.OnRtpPacket(*packet), true); EXPECT_TRUE https://codereview.webrtc.org/2920993002/diff/1/webrtc/call/rtp_demuxer_unitt... webrtc/call/rtp_demuxer_unittest.cc:84: // Test tear-down end comments with a '.' https://codereview.webrtc.org/2920993002/diff/1/webrtc/call/rtp_demuxer_unitt... webrtc/call/rtp_demuxer_unittest.cc:99: packets[i] = CreateRtpPacketReceived(ssrcs[0], static_cast<uint16_t>(i)); if you wish to be more explicit, then beeing explicit about parameter name might be better than about parameter type: CreateRtpPacketReceived(ssrc[0], /*sequence_number=*/i); https://codereview.webrtc.org/2920993002/diff/1/webrtc/call/rtp_demuxer_unitt... webrtc/call/rtp_demuxer_unittest.cc:103: for (auto& packet : packets) { may by const auto& since you do not change the value https://codereview.webrtc.org/2920993002/diff/1/webrtc/call/rtp_demuxer_unitt... webrtc/call/rtp_demuxer_unittest.cc:133: for (auto& sink : sinks) { may be const auto& https://codereview.webrtc.org/2920993002/diff/1/webrtc/call/rtp_demuxer_unitt... webrtc/call/rtp_demuxer_unittest.cc:158: constexpr uint32_t ssrc = 404; 404 is very good number for test where ssrc will not be found :) https://codereview.webrtc.org/2920993002/diff/1/webrtc/call/rtp_demuxer_unitt... webrtc/call/rtp_demuxer_unittest.cc:167: EXPECT_EQ(demuxer.OnRtpPacket(*packet), false); EXPECT_FALSE https://codereview.webrtc.org/2920993002/diff/1/webrtc/call/rtp_demuxer_unitt... webrtc/call/rtp_demuxer_unittest.cc:176: uint16_t seq_num; do you use the variable outside the for loop? https://codereview.webrtc.org/2920993002/diff/1/webrtc/call/rtp_demuxer_unitt... webrtc/call/rtp_demuxer_unittest.cc:331: const uint32_t ssrc = 1000 + static_cast<uint32_t>(i); static_cast is probably an overkill here. Do you think it is more readable with or without it? https://codereview.webrtc.org/2920993002/diff/1/webrtc/call/rtp_demuxer_unitt... webrtc/call/rtp_demuxer_unittest.cc:400: } std::string long_rsid(StreamId::kMaxSize + 1, 'a'); https://codereview.webrtc.org/2920993002/diff/1/webrtc/common_types.cc File webrtc/common_types.cc (right): https://codereview.webrtc.org/2920993002/diff/1/webrtc/common_types.cc#newcode29 webrtc/common_types.cc:29: std::all_of(data, data + size, isalnum)); may be remove () around the expression. https://google.github.io/styleguide/cppguide.html#Return_Values https://codereview.webrtc.org/2920993002/diff/1/webrtc/common_types.cc#newcode34 webrtc/common_types.cc:34: RTC_DCHECK_LE(size, kMaxSize); // TODO(eladalon): IsLegalName? probably not: Set is called by parser too. While parser should guarantee the size, it doesn't check if name isalnum. of-course it can't guarantee that other side put only allowed characters into the buffer. https://codereview.webrtc.org/2920993002/diff/1/webrtc/common_types.h File webrtc/common_types.h (right): https://codereview.webrtc.org/2920993002/diff/1/webrtc/common_types.h#newcode710 webrtc/common_types.h:710: static bool IsLegalName(const char* data, size_t size); until std::string_view is available, may be use rtc::ArrayView<const char>: to stress data+size is one entity, not two independent. bonus: std::string should be implicetly convertable to rtc::ArrayView<const char> May be do not use word 'Legal' how about name 'IsValid'?
https://codereview.webrtc.org/2920993002/diff/1/webrtc/call/rtp_demuxer_unitt... File webrtc/call/rtp_demuxer_unittest.cc (right): https://codereview.webrtc.org/2920993002/diff/1/webrtc/call/rtp_demuxer_unitt... webrtc/call/rtp_demuxer_unittest.cc:54: rtp::Packet::ExtensionManager extensions_manager(extensions); On 2017/06/02 17:42:19, danilchap wrote: > why not just > RtpPacketReceieved::ExtensionManager extensions; > extensions.Register<webrtc::RtpStreamId>(kRsidExtensionId); > auto packet = rtc::MakeUnique<RtpPacketReceived>(&extenions); > if (rsid != nullptr) { // or if (!rsid.empty()) > packet->SetExtension<webrtc::RtpStreamId>(*rsid); > } 1. I'd like to make it very clear when the packet has an RSID, and when it doesn't. So not even the ExtensionManager when no RSID is desired. 2. Can't do !rsid.empty() because I also check for invalid RSIDs, and an empty-string is also a possible invalid RSID. 3. Removed the RtpHeaderExtensionMap, as per your suggestion. https://codereview.webrtc.org/2920993002/diff/1/webrtc/call/rtp_demuxer_unitt... webrtc/call/rtp_demuxer_unittest.cc:65: RtpDemuxerTest() = default; On 2017/06/02 17:42:19, danilchap wrote: > feel free to omit default constructor/destructor for class declared in .cc file Done. https://codereview.webrtc.org/2920993002/diff/1/webrtc/call/rtp_demuxer_unitt... webrtc/call/rtp_demuxer_unittest.cc:68: RtpDemuxer demuxer; On 2017/06/02 17:42:20, danilchap wrote: > fixture that small easy not to use at all: > just an extra line per test, but you demonstrate that RtpDemuxer doesn't need > any complicated setup. Done. https://codereview.webrtc.org/2920993002/diff/1/webrtc/call/rtp_demuxer_unitt... webrtc/call/rtp_demuxer_unittest.cc:80: EXPECT_CALL(sinks[i], OnRtpPacket(SamePacketAs(*packet))).Times(1); On 2017/06/02 17:42:19, danilchap wrote: > .Times(1) can by omitted if you wish > https://github.com/google/googletest/blob/master/googlemock/docs/ForDummies.m... Some people see it as self-documentation. I am still on the fence. I'll keep it for now. https://codereview.webrtc.org/2920993002/diff/1/webrtc/call/rtp_demuxer_unitt... webrtc/call/rtp_demuxer_unittest.cc:81: EXPECT_EQ(demuxer.OnRtpPacket(*packet), true); On 2017/06/02 17:42:19, danilchap wrote: > EXPECT_TRUE Yes, that does look silly.Almost like "if (x == true)". Thanks for pointing it out. https://codereview.webrtc.org/2920993002/diff/1/webrtc/call/rtp_demuxer_unitt... webrtc/call/rtp_demuxer_unittest.cc:84: // Test tear-down On 2017/06/02 17:42:19, danilchap wrote: > end comments with a '.' I believe full stops belong at the end of a sentence, not of a title. "War and Peace" vs. "War and Peace." https://codereview.webrtc.org/2920993002/diff/1/webrtc/call/rtp_demuxer_unitt... webrtc/call/rtp_demuxer_unittest.cc:99: packets[i] = CreateRtpPacketReceived(ssrcs[0], static_cast<uint16_t>(i)); On 2017/06/02 17:42:20, danilchap wrote: > if you wish to be more explicit, then beeing explicit about parameter name might > be better than about parameter type: > CreateRtpPacketReceived(ssrc[0], /*sequence_number=*/i); I'm actually doing this to avoid compiler errors. Implicit narrowing conversions stress one of the trybots out. https://codereview.webrtc.org/2920993002/diff/1/webrtc/call/rtp_demuxer_unitt... webrtc/call/rtp_demuxer_unittest.cc:103: for (auto& packet : packets) { On 2017/06/02 17:42:19, danilchap wrote: > may by const auto& since you do not change the value Done. https://codereview.webrtc.org/2920993002/diff/1/webrtc/call/rtp_demuxer_unitt... webrtc/call/rtp_demuxer_unittest.cc:133: for (auto& sink : sinks) { On 2017/06/02 17:42:20, danilchap wrote: > may be const auto& Done. https://codereview.webrtc.org/2920993002/diff/1/webrtc/call/rtp_demuxer_unitt... webrtc/call/rtp_demuxer_unittest.cc:158: constexpr uint32_t ssrc = 404; On 2017/06/02 17:42:19, danilchap wrote: > 404 is very good number for test where ssrc will not be found :) :-) https://codereview.webrtc.org/2920993002/diff/1/webrtc/call/rtp_demuxer_unitt... webrtc/call/rtp_demuxer_unittest.cc:167: EXPECT_EQ(demuxer.OnRtpPacket(*packet), false); On 2017/06/02 17:42:19, danilchap wrote: > EXPECT_FALSE Done. https://codereview.webrtc.org/2920993002/diff/1/webrtc/call/rtp_demuxer_unitt... webrtc/call/rtp_demuxer_unittest.cc:176: uint16_t seq_num; On 2017/06/02 17:42:19, danilchap wrote: > do you use the variable outside the for loop? Actually, I'd meant to do it for the last packet (the one after the sink's removal). I've added that now; thanks. (It's not really necessary, but something feels odd about sending a sequence of RTP packets with specific, incrementing sequence numbers, then just forgetting about it and sending one with an undetermined sequence number. One could ask oneself if the test didn't perhaps only pass because of a bad sequence number. This eliminates such concerns.) https://codereview.webrtc.org/2920993002/diff/1/webrtc/call/rtp_demuxer_unitt... webrtc/call/rtp_demuxer_unittest.cc:331: const uint32_t ssrc = 1000 + static_cast<uint32_t>(i); On 2017/06/02 17:42:19, danilchap wrote: > static_cast is probably an overkill here. > Do you think it is more readable with or without it? Not readability; trying to keep the trybots' various compilers calm and happy. https://codereview.webrtc.org/2920993002/diff/1/webrtc/call/rtp_demuxer_unitt... webrtc/call/rtp_demuxer_unittest.cc:400: } On 2017/06/02 17:42:19, danilchap wrote: > std::string long_rsid(StreamId::kMaxSize + 1, 'a'); Done. https://codereview.webrtc.org/2920993002/diff/1/webrtc/common_types.cc File webrtc/common_types.cc (right): https://codereview.webrtc.org/2920993002/diff/1/webrtc/common_types.cc#newcode29 webrtc/common_types.cc:29: std::all_of(data, data + size, isalnum)); On 2017/06/02 17:42:20, danilchap wrote: > may be remove () around the expression. > https://google.github.io/styleguide/cppguide.html#Return_Values That link seems to suggest that the parenthesis are in fact okay in here. https://codereview.webrtc.org/2920993002/diff/1/webrtc/common_types.cc#newcode34 webrtc/common_types.cc:34: RTC_DCHECK_LE(size, kMaxSize); // TODO(eladalon): IsLegalName? On 2017/06/02 17:42:20, danilchap wrote: > probably not: > Set is called by parser too. While parser should guarantee the size, it doesn't > check if name isalnum. > of-course it can't guarantee that other side put only allowed characters into > the buffer. My intention with this TODO is to come back to this at a later CL and change the CHECK_LE into a CHECK(IsLegalName()), after the necessary extra work has been done - making sure anyone who could call StreamId would fail gracefully instead of calling StreamId() with a disallowed RSID. Wdyt? https://codereview.webrtc.org/2920993002/diff/1/webrtc/common_types.h File webrtc/common_types.h (right): https://codereview.webrtc.org/2920993002/diff/1/webrtc/common_types.h#newcode710 webrtc/common_types.h:710: static bool IsLegalName(const char* data, size_t size); On 2017/06/02 17:42:20, danilchap wrote: > until std::string_view is available, may be use rtc::ArrayView<const char>: > to stress data+size is one entity, not two independent. > bonus: std::string should be implicetly convertable to rtc::ArrayView<const > char> > > May be do not use word 'Legal' > how about name 'IsValid'? 1. About ArrayView - done. 2. About "valid" vs. "legal", I'm not sure. I was going to accept this suggestion, but right before the search/replace, noticed that IsValidName() does not occur anywhere in the codebase, but IsLegalName() does. In light of this, wdyt?
https://codereview.webrtc.org/2920993002/diff/1/webrtc/call/rtp_demuxer_unitt... File webrtc/call/rtp_demuxer_unittest.cc (right): https://codereview.webrtc.org/2920993002/diff/1/webrtc/call/rtp_demuxer_unitt... webrtc/call/rtp_demuxer_unittest.cc:54: rtp::Packet::ExtensionManager extensions_manager(extensions); On 2017/06/02 19:44:04, eladalon wrote: > On 2017/06/02 17:42:19, danilchap wrote: > > why not just > > RtpPacketReceieved::ExtensionManager extensions; > > extensions.Register<webrtc::RtpStreamId>(kRsidExtensionId); > > auto packet = rtc::MakeUnique<RtpPacketReceived>(&extenions); > > if (rsid != nullptr) { // or if (!rsid.empty()) > > packet->SetExtension<webrtc::RtpStreamId>(*rsid); > > } > > 1. I'd like to make it very clear when the packet has an RSID, and when it > doesn't. So not even the ExtensionManager when no RSID is desired. Good reason! Did you consider two functions to make it clearer? CreateRtpPacket(ssrc, seq_no = 0) CreateRtpPacketWithRsid(rsid, ssrc, seq_no = 0) https://codereview.webrtc.org/2920993002/diff/1/webrtc/common_types.cc File webrtc/common_types.cc (right): https://codereview.webrtc.org/2920993002/diff/1/webrtc/common_types.cc#newcode29 webrtc/common_types.cc:29: std::all_of(data, data + size, isalnum)); On 2017/06/02 19:44:05, eladalon wrote: > On 2017/06/02 17:42:20, danilchap wrote: > > may be remove () around the expression. > > https://google.github.io/styleguide/cppguide.html#Return_Values > > That link seems to suggest that the parenthesis are in fact okay in here. yep, in this case it is ok, thus 'may be' https://codereview.webrtc.org/2920993002/diff/1/webrtc/common_types.cc#newcode34 webrtc/common_types.cc:34: RTC_DCHECK_LE(size, kMaxSize); // TODO(eladalon): IsLegalName? On 2017/06/02 19:44:05, eladalon wrote: > On 2017/06/02 17:42:20, danilchap wrote: > > probably not: > > Set is called by parser too. While parser should guarantee the size, it > doesn't > > check if name isalnum. > > of-course it can't guarantee that other side put only allowed characters into > > the buffer. > > My intention with this TODO is to come back to this at a later CL and change the > CHECK_LE into a CHECK(IsLegalName()), after the necessary extra work has been > done - making sure anyone who could call StreamId would fail gracefully instead > of calling StreamId() with a disallowed RSID. Wdyt? I prefer to think about StreamId as a special implementation of string (and eventually plan to replace all uses of it by string) so prefer to put as little as possible extra logic in it. As for users of the StreamId - I do not think it would be benefitial to check(IsLegalName()) when receiving a packet - illegal names will be discarded automatically when compared to configured legal names. https://codereview.webrtc.org/2920993002/diff/1/webrtc/common_types.h File webrtc/common_types.h (right): https://codereview.webrtc.org/2920993002/diff/1/webrtc/common_types.h#newcode710 webrtc/common_types.h:710: static bool IsLegalName(const char* data, size_t size); On 2017/06/02 19:44:05, eladalon wrote: > On 2017/06/02 17:42:20, danilchap wrote: > > until std::string_view is available, may be use rtc::ArrayView<const char>: > > to stress data+size is one entity, not two independent. > > bonus: std::string should be implicetly convertable to rtc::ArrayView<const > > char> > > > > May be do not use word 'Legal' > > how about name 'IsValid'? > > 1. About ArrayView - done. > 2. About "valid" vs. "legal", I'm not sure. I was going to accept this > suggestion, but right before the search/replace, noticed that IsValidName() does > not occur anywhere in the codebase, but IsLegalName() does. In light of this, > wdyt? IsValidName doesn't occur, but IsValid occur way more often than IsLegal (found only one class that use IsLegalName) On the other side, it should be valid to put and use illegal name in this type. i.e. IsLegalName probably better reflect what this function is checking. https://codereview.webrtc.org/2920993002/diff/20001/webrtc/call/rtp_demuxer.cc File webrtc/call/rtp_demuxer.cc (right): https://codereview.webrtc.org/2920993002/diff/20001/webrtc/call/rtp_demuxer.c... webrtc/call/rtp_demuxer.cc:33: size_t RemoveFromMultimapByValue(std::multimap<Key, Value*>& multimap, Pass by pointer. "All parameters passed by reference must be labeled const." https://google.github.io/styleguide/cppguide.html#Reference_Arguments https://codereview.webrtc.org/2920993002/diff/20001/webrtc/call/rtp_demuxer.c... webrtc/call/rtp_demuxer.cc:88: FindSsrcAssociations(packet); I wonder if it will be better to do like this instead of using processed_ssrc: auto it_range = sinks_.equal_range(ssrc); if (it_range.empty()) { if (!FindSsrcAssociations(packet)) return; it_range = sinks_.equal_range(ssrc); } // Returns true if new ssrc assosiation was descovered and created. bool FindSsrcAssosiations() { } https://codereview.webrtc.org/2920993002/diff/20001/webrtc/call/rtp_demuxer.h File webrtc/call/rtp_demuxer.h (right): https://codereview.webrtc.org/2920993002/diff/20001/webrtc/call/rtp_demuxer.h... webrtc/call/rtp_demuxer.h:78: std::set<uint32_t> processed_ssrcs_; In 'all good' scenario this set cause extra lookup by ssrc per packet. In 'bad user' scenario this set prevents lookup by rsid. I'm not entirely sure it worse it. Specially that this structure also adds a bit of code complexity. https://codereview.webrtc.org/2920993002/diff/20001/webrtc/call/rtp_demuxer_u... File webrtc/call/rtp_demuxer_unittest.cc (right): https://codereview.webrtc.org/2920993002/diff/20001/webrtc/call/rtp_demuxer_u... webrtc/call/rtp_demuxer_unittest.cc:19: #include "webrtc/modules/rtp_rtcp/source/rtp_header_extension.h" thanks to your poke this can be replaced with /rtp_rtcp/include/rtp_header_extension_map.h" which is (hopefully) cleaner. https://codereview.webrtc.org/2920993002/diff/20001/webrtc/call/rtp_demuxer_u... webrtc/call/rtp_demuxer_unittest.cc:52: rtp::Packet::ExtensionManager extension_manager; may be RtpPacketReceived instead of rtp::Packet (this would avoid complicating refactoring/renaming of the rtp::Packet - it probably shouldn't live in it's own namespace) https://codereview.webrtc.org/2920993002/diff/20001/webrtc/call/rtp_demuxer_u... webrtc/call/rtp_demuxer_unittest.cc:62: TEST(RtpDemuxerTest, OnRtpPacketCalledOnCorrectSink) { with new demuxer responsibilities may be describe what 'Correct' mean: e.g. TEST(RtpDemuxerTest, DemuxRtpPacketsBySsrc) https://codereview.webrtc.org/2920993002/diff/20001/webrtc/call/rtp_demuxer_u... webrtc/call/rtp_demuxer_unittest.cc:71: for (size_t i = 0; i < arraysize(sinks); i++) { may be, for in-test consistency, use arraysize(same variable) https://codereview.webrtc.org/2920993002/diff/20001/webrtc/call/rtp_demuxer_u... webrtc/call/rtp_demuxer_unittest.cc:78: for (size_t i = 0; i < arraysize(ssrcs); i++) { may be use c++11 for loop like in MultipleSinksMappedToSameSsrc test tear down. https://codereview.webrtc.org/2920993002/diff/20001/webrtc/call/rtp_demuxer_u... webrtc/call/rtp_demuxer_unittest.cc:83: TEST(RtpDemuxerTest, PacketsDeliveredInRightOrder) { I'm not sure what is the value of this test. Packets delivered using callback (i.e. sink::OnRtpPacket called before demuxer::OnRtpPacket returns). That make this test too trivial in synchronous scenario. If you pretend not to know demuxer implementation, then it isn't demuxer's responsibility to keep packets order. (they might arrive from network in wrong order anyway and it is next module responsibility to reorder packets by sequence number) https://codereview.webrtc.org/2920993002/diff/20001/webrtc/call/rtp_demuxer_u... webrtc/call/rtp_demuxer_unittest.cc:112: TEST(RtpDemuxerTest, MultipleSinksMappedToSameSsrc) { may be improve test name by describing what Demuxer should do in this scenario: DeliverRtpPacketToMultipleSinksMappedToSameSsrc https://codereview.webrtc.org/2920993002/diff/20001/webrtc/call/rtp_demuxer_u... webrtc/call/rtp_demuxer_unittest.cc:225: packets[i] = CreateRtpPacketReceived(rsid_ssrc, static_cast<uint16_t>(i)); may be change CreateRtpPacket to take int for sequence number and convert to uint16_t inside the function to make use of it cleaner while bots happy. https://codereview.webrtc.org/2920993002/diff/20001/webrtc/call/rtp_demuxer_u... webrtc/call/rtp_demuxer_unittest.cc:300: constexpr uint32_t kSharedSsrc = 100; sometimes you use constant name style for ssrc constants, sometimes variable name style. Either is acceptable, but can you use only one style through the file. https://google.github.io/styleguide/cppguide.html#Constant_Names https://codereview.webrtc.org/2920993002/diff/20001/webrtc/call/rtp_demuxer_u... webrtc/call/rtp_demuxer_unittest.cc:304: auto packet_a = since packet 'b' doesn't map to sink 'b' may be use different numeration style for sinks and packets: sink_a, sink_b, but packet1, packet2, packet3 https://codereview.webrtc.org/2920993002/diff/20001/webrtc/call/rtp_demuxer_u... webrtc/call/rtp_demuxer_unittest.cc:399: // The RSID and the SSRC associations above have merged into one. what is deletion count? is it number of times AddSink was called for the sink? this test suggest it is not. Does that 'deletion count' need to be well-defined? May be document 'return positive number if at least one sink was deleted' and EXPECT that in tests instead of exact number. (and change to return bool in a follow-up CL). If you want to expect exact number, then it need better documentation and few dedicated tests.
https://codereview.webrtc.org/2920993002/diff/1/webrtc/call/rtp_demuxer_unitt... File webrtc/call/rtp_demuxer_unittest.cc (right): https://codereview.webrtc.org/2920993002/diff/1/webrtc/call/rtp_demuxer_unitt... webrtc/call/rtp_demuxer_unittest.cc:54: rtp::Packet::ExtensionManager extensions_manager(extensions); On 2017/06/07 13:45:41, danilchap wrote: > On 2017/06/02 19:44:04, eladalon wrote: > > On 2017/06/02 17:42:19, danilchap wrote: > > > why not just > > > RtpPacketReceieved::ExtensionManager extensions; > > > extensions.Register<webrtc::RtpStreamId>(kRsidExtensionId); > > > auto packet = rtc::MakeUnique<RtpPacketReceived>(&extenions); > > > if (rsid != nullptr) { // or if (!rsid.empty()) > > > packet->SetExtension<webrtc::RtpStreamId>(*rsid); > > > } > > > > 1. I'd like to make it very clear when the packet has an RSID, and when it > > doesn't. So not even the ExtensionManager when no RSID is desired. > > Good reason! > Did you consider two functions to make it clearer? > CreateRtpPacket(ssrc, seq_no = 0) > CreateRtpPacketWithRsid(rsid, ssrc, seq_no = 0) Done. https://codereview.webrtc.org/2920993002/diff/20001/webrtc/call/rtp_demuxer.cc File webrtc/call/rtp_demuxer.cc (right): https://codereview.webrtc.org/2920993002/diff/20001/webrtc/call/rtp_demuxer.c... webrtc/call/rtp_demuxer.cc:33: size_t RemoveFromMultimapByValue(std::multimap<Key, Value*>& multimap, On 2017/06/07 13:45:42, danilchap wrote: > Pass by pointer. > "All parameters passed by reference must be labeled const." > https://google.github.io/styleguide/cppguide.html#Reference_Arguments Done. https://codereview.webrtc.org/2920993002/diff/20001/webrtc/call/rtp_demuxer.c... webrtc/call/rtp_demuxer.cc:88: FindSsrcAssociations(packet); On 2017/06/07 13:45:42, danilchap wrote: > I wonder if it will be better to do like this instead of using processed_ssrc: > auto it_range = sinks_.equal_range(ssrc); > if (it_range.empty()) { > if (!FindSsrcAssociations(packet)) > return; > it_range = sinks_.equal_range(ssrc); > } > > // Returns true if new ssrc assosiation was descovered and created. > bool FindSsrcAssosiations() { > } Discussed offline; keeping as is. One point, in case we forget the discussion and return to it in the future - associations might exist for both an SSRC and RSID. If we don't examine extra associations on the packet because we've marked the SSRC as known, we won't find the other sinks which might be mapped to the same SSRC via an RSID. https://codereview.webrtc.org/2920993002/diff/20001/webrtc/call/rtp_demuxer.h File webrtc/call/rtp_demuxer.h (right): https://codereview.webrtc.org/2920993002/diff/20001/webrtc/call/rtp_demuxer.h... webrtc/call/rtp_demuxer.h:78: std::set<uint32_t> processed_ssrcs_; On 2017/06/07 13:45:42, danilchap wrote: > In 'all good' scenario this set cause extra lookup by ssrc per packet. > In 'bad user' scenario this set prevents lookup by rsid. > I'm not entirely sure it worse it. Specially that this structure also adds a bit > of code complexity. Discussed offline; keeping as is. https://codereview.webrtc.org/2920993002/diff/20001/webrtc/call/rtp_demuxer_u... File webrtc/call/rtp_demuxer_unittest.cc (right): https://codereview.webrtc.org/2920993002/diff/20001/webrtc/call/rtp_demuxer_u... webrtc/call/rtp_demuxer_unittest.cc:19: #include "webrtc/modules/rtp_rtcp/source/rtp_header_extension.h" On 2017/06/07 13:45:42, danilchap wrote: > thanks to your poke this can be replaced with > /rtp_rtcp/include/rtp_header_extension_map.h" > which is (hopefully) cleaner. Done. https://codereview.webrtc.org/2920993002/diff/20001/webrtc/call/rtp_demuxer_u... webrtc/call/rtp_demuxer_unittest.cc:52: rtp::Packet::ExtensionManager extension_manager; On 2017/06/07 13:45:42, danilchap wrote: > may be RtpPacketReceived instead of rtp::Packet > (this would avoid complicating refactoring/renaming of the rtp::Packet - it > probably shouldn't live in it's own namespace) Done. https://codereview.webrtc.org/2920993002/diff/20001/webrtc/call/rtp_demuxer_u... webrtc/call/rtp_demuxer_unittest.cc:62: TEST(RtpDemuxerTest, OnRtpPacketCalledOnCorrectSink) { On 2017/06/07 13:45:42, danilchap wrote: > with new demuxer responsibilities may be describe what 'Correct' mean: > e.g. TEST(RtpDemuxerTest, DemuxRtpPacketsBySsrc) Appended "BySsrc". https://codereview.webrtc.org/2920993002/diff/20001/webrtc/call/rtp_demuxer_u... webrtc/call/rtp_demuxer_unittest.cc:71: for (size_t i = 0; i < arraysize(sinks); i++) { On 2017/06/07 13:45:43, danilchap wrote: > may be, for in-test consistency, use arraysize(same variable) Done. https://codereview.webrtc.org/2920993002/diff/20001/webrtc/call/rtp_demuxer_u... webrtc/call/rtp_demuxer_unittest.cc:78: for (size_t i = 0; i < arraysize(ssrcs); i++) { On 2017/06/07 13:45:42, danilchap wrote: > may be use c++11 for loop like in MultipleSinksMappedToSameSsrc test tear down. Done. https://codereview.webrtc.org/2920993002/diff/20001/webrtc/call/rtp_demuxer_u... webrtc/call/rtp_demuxer_unittest.cc:83: TEST(RtpDemuxerTest, PacketsDeliveredInRightOrder) { On 2017/06/07 13:45:42, danilchap wrote: > I'm not sure what is the value of this test. > Packets delivered using callback (i.e. sink::OnRtpPacket called before > demuxer::OnRtpPacket returns). > That make this test too trivial in synchronous scenario. > > If you pretend not to know demuxer implementation, then it isn't demuxer's > responsibility to keep packets order. (they might arrive from network in wrong > order anyway and it is next module responsibility to reorder packets by sequence > number) Clarification - the code neither re-orders the packet according to their sequence number, nor does it test that he packets are correctly ordered according to sequence number. What it does is check that the order - whether correct or wrong - remains unchanged. (Premature reordering by this module might have adversely affected other modules down the pipeline that would have, for example, attempted to gauge the network's quality.) https://codereview.webrtc.org/2920993002/diff/20001/webrtc/call/rtp_demuxer_u... webrtc/call/rtp_demuxer_unittest.cc:112: TEST(RtpDemuxerTest, MultipleSinksMappedToSameSsrc) { On 2017/06/07 13:45:42, danilchap wrote: > may be improve test name by describing what Demuxer should do in this scenario: > DeliverRtpPacketToMultipleSinksMappedToSameSsrc IMHO, clear from context - the demuxer delivers packets to the correct sinks. https://codereview.webrtc.org/2920993002/diff/20001/webrtc/call/rtp_demuxer_u... webrtc/call/rtp_demuxer_unittest.cc:225: packets[i] = CreateRtpPacketReceived(rsid_ssrc, static_cast<uint16_t>(i)); On 2017/06/07 13:45:42, danilchap wrote: > may be change CreateRtpPacket to take int for sequence number and convert to > uint16_t inside the function to make use of it cleaner while bots happy. I've adopted, but took size_t instead of int, because then we can really get rid of all of the places where we had the cast. I did not do the same for SSRC, btw, because it's needed less often. https://codereview.webrtc.org/2920993002/diff/20001/webrtc/call/rtp_demuxer_u... webrtc/call/rtp_demuxer_unittest.cc:300: constexpr uint32_t kSharedSsrc = 100; On 2017/06/07 13:45:42, danilchap wrote: > sometimes you use constant name style for ssrc constants, sometimes variable > name style. > Either is acceptable, but can you use only one style through the file. > https://google.github.io/styleguide/cppguide.html#Constant_Names Done. https://codereview.webrtc.org/2920993002/diff/20001/webrtc/call/rtp_demuxer_u... webrtc/call/rtp_demuxer_unittest.cc:304: auto packet_a = On 2017/06/07 13:45:42, danilchap wrote: > since packet 'b' doesn't map to sink 'b' may be use different numeration style > for sinks and packets: > sink_a, sink_b, but packet1, packet2, packet3 Please refer to this comment: // Second, a packet with |rsid_b| is received. Its RSID is ignored. https://codereview.webrtc.org/2920993002/diff/20001/webrtc/call/rtp_demuxer_u... webrtc/call/rtp_demuxer_unittest.cc:399: // The RSID and the SSRC associations above have merged into one. On 2017/06/07 13:45:42, danilchap wrote: > what is deletion count? is it number of times AddSink was called for the sink? > this test suggest it is not. > Does that 'deletion count' need to be well-defined? > May be document 'return positive number if at least one sink was deleted' and > EXPECT that in tests instead of exact number. > (and change to return bool in a follow-up CL). > > If you want to expect exact number, then it need better documentation and few > dedicated tests. I've previously discussed this problem with nisse@ and we've decided to keep the deletion count as the number of removed associations the sink has which are removed when RemoveSink is called. This is known to *not* always correspond to the number of AddSink() calls for the sink, since some RSID associations might be deleted in response to OnRtpPacket(). That was a known edge-case. Following our (Danil and mine) offline discussion, I've changed this function to return a boolean rather than the deletion count.
lgtm with few more nits https://codereview.webrtc.org/2920993002/diff/60001/webrtc/call/rtp_demuxer.h File webrtc/call/rtp_demuxer.h (right): https://codereview.webrtc.org/2920993002/diff/60001/webrtc/call/rtp_demuxer.h... webrtc/call/rtp_demuxer.h:40: // Removes a sink. Returns deletion true if anything removed, false otherwise. 'Returns deletion true'? did you mean 'Returns true'? likely remove 'false otherwise' https://google.github.io/styleguide/cppguide.html#Function_Comments "do not be unnecessarily verbose or state the completely obvious. Notice below that it is not necessary to say "returns false otherwise" because this is implied." https://codereview.webrtc.org/2920993002/diff/60001/webrtc/call/rtp_demuxer_u... File webrtc/call/rtp_demuxer_unittest.cc (right): https://codereview.webrtc.org/2920993002/diff/60001/webrtc/call/rtp_demuxer_u... webrtc/call/rtp_demuxer_unittest.cc:47: size_t sequence_number = 0) { https://google.github.io/styleguide/cppguide.html#Integer_Types might be cleaner to use int both for sequence_number and for iterators. Style allows int for any use, but special types encouraged only when appropriate. size_t is appropriate for iterator, but looks a bit strange for a sequence number. https://codereview.webrtc.org/2920993002/diff/60001/webrtc/call/rtp_demuxer_u... webrtc/call/rtp_demuxer_unittest.cc:70: packet->SetExtension<webrtc::RtpStreamId>(rsid); since this code already in webrtc namespace, no need to prepend RtpStreamId with webrtc::
https://codereview.webrtc.org/2920993002/diff/60001/webrtc/call/rtp_demuxer.h File webrtc/call/rtp_demuxer.h (right): https://codereview.webrtc.org/2920993002/diff/60001/webrtc/call/rtp_demuxer.h... webrtc/call/rtp_demuxer.h:40: // Removes a sink. Returns deletion true if anything removed, false otherwise. On 2017/06/08 08:04:19, danilchap wrote: > 'Returns deletion true'? did you mean 'Returns true'? > > likely remove 'false otherwise' > https://google.github.io/styleguide/cppguide.html#Function_Comments > "do not be unnecessarily verbose or state the completely obvious. Notice below > that it is not necessary to say "returns false otherwise" because this is > implied." I'll use: // Removes a sink. Return value reports if anything was actually removed. // Null pointer is not allowed. bool RemoveSink(const RtpPacketSinkInterface* sink); https://codereview.webrtc.org/2920993002/diff/60001/webrtc/call/rtp_demuxer_u... File webrtc/call/rtp_demuxer_unittest.cc (right): https://codereview.webrtc.org/2920993002/diff/60001/webrtc/call/rtp_demuxer_u... webrtc/call/rtp_demuxer_unittest.cc:47: size_t sequence_number = 0) { On 2017/06/08 08:04:19, danilchap wrote: > https://google.github.io/styleguide/cppguide.html#Integer_Types > > might be cleaner to use int both for sequence_number and for iterators. > Style allows int for any use, but special types encouraged only when > appropriate. > size_t is appropriate for iterator, but looks a bit strange for a sequence > number. My reasoning: 1. I've worked with Minyue before, and he seemed to be a bit strict about using size_t in iterators. 2. size_t is unsigned, as is uint16_t, but int isn't. Wdyt? https://codereview.webrtc.org/2920993002/diff/60001/webrtc/call/rtp_demuxer_u... webrtc/call/rtp_demuxer_unittest.cc:70: packet->SetExtension<webrtc::RtpStreamId>(rsid); On 2017/06/08 08:04:19, danilchap wrote: > since this code already in webrtc namespace, no need to prepend RtpStreamId with > webrtc:: Done.
https://codereview.webrtc.org/2920993002/diff/60001/webrtc/call/rtp_demuxer_u... File webrtc/call/rtp_demuxer_unittest.cc (right): https://codereview.webrtc.org/2920993002/diff/60001/webrtc/call/rtp_demuxer_u... webrtc/call/rtp_demuxer_unittest.cc:47: size_t sequence_number = 0) { On 2017/06/08 08:59:53, eladalon wrote: > On 2017/06/08 08:04:19, danilchap wrote: > > https://google.github.io/styleguide/cppguide.html#Integer_Types > > > > might be cleaner to use int both for sequence_number and for iterators. > > Style allows int for any use, but special types encouraged only when > > appropriate. > > size_t is appropriate for iterator, but looks a bit strange for a sequence > > number. > > My reasoning: > 1. I've worked with Minyue before, and he seemed to be a bit strict about using > size_t in iterators. > 2. size_t is unsigned, as is uint16_t, but int isn't. > Wdyt? for argument 2. style explicitly say "In particular, do not use unsigned types to say a number will never be negative." for argument 1 - yes, chromium style recommend size_t for indices (https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md...) Personally I do not find that recommendation strict. Feel free to keep size_t
The CQ bit was checked by eladalon@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from danilchap@webrtc.org Link to the patchset: https://codereview.webrtc.org/2920993002/#ps80001 (title: ".")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
stefan@webrtc.org, I think I'll need your lgtm because of common_types.h/cc.
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: presubmit on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/17902)
stefan@webrtc.org changed reviewers: + stefan@webrtc.org
lgtm I think we should think about if rtp_demuxer.h should really live in the root of webrtc/call, but I won't block this CL on that.
On 2017/06/08 09:28:33, stefan-webrtc wrote: > lgtm > > I think we should think about if rtp_demuxer.h should really live in the root of > webrtc/call, but I won't block this CL on that. Should I create a bug for that? (This CL introduced a TODO for moving the tests out of video_engine_tests. I could associate that TODO with the bug, if I create it.)
The CQ bit was checked by eladalon@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from danilchap@webrtc.org, stefan@webrtc.org Link to the patchset: https://codereview.webrtc.org/2920993002/#ps100001 (title: "Missed one cast.")
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": 100001, "attempt_start_ts": 1496919357015630,
"parent_rev": "5c4897fffa0bb3af6651b27a6b04f6e0c5577cae", "commit_rev":
"d0244c21cdb2985df9abec84a0aed6d0d0cbeec0"}
Message was sent while issue was closed.
Description was changed from ========== Add RSID-based demuxing to RtpDemuxer Make RtpDemuxer able to demux RTP packets according to RSID (RTP Stream ID), as well as the (pre-existing) ability to demux according to SSRC. BUG=None ========== to ========== Add RSID-based demuxing to RtpDemuxer Make RtpDemuxer able to demux RTP packets according to RSID (RTP Stream ID), as well as the (pre-existing) ability to demux according to SSRC. BUG=None Review-Url: https://codereview.webrtc.org/2920993002 Cr-Commit-Position: refs/heads/master@{#18495} Committed: https://chromium.googlesource.com/external/webrtc/+/d0244c21cdb2985df9abec84a... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as https://chromium.googlesource.com/external/webrtc/+/d0244c21cdb2985df9abec84a... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
