Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2943693003:
Create RtcpDemuxer (Closed)
DescriptionCreate RtcpDemuxer. Capabilities:
1. Demux RTCP messages according to the sender-SSRC.
2. Demux RTCP messages according to the RSID (resolved to an SSRC, then compared to the sender-RTCP).
3. Allow listening in on all RTCP messages passing through the demuxer ("broadcast sinks").
BUG=webrtc:7135
Review-Url: https://codereview.webrtc.org/2943693003
Cr-Commit-Position: refs/heads/master@{#18763}
Committed: https://chromium.googlesource.com/external/webrtc/+/cb83bdf01f2ec8b9ed254991edc2be053c9eed24
Patch Set 1 #Patch Set 2 : git cl format #
Total comments: 15
Patch Set 3 : CR response #
Total comments: 21
Patch Set 4 : CR response and some cleanup. #
Total comments: 5
Patch Set 5 : Get rid of ArrayView in rtp_rtcp_demuxer_helper_unittest.cc, too. #
Total comments: 22
Patch Set 6 : . #
Total comments: 14
Patch Set 7 : . #
Total comments: 10
Patch Set 8 : . #
Total comments: 2
Patch Set 9 : . #Patch Set 10 : . #Patch Set 11 : dependencies #Patch Set 12 : . #Patch Set 13 : Rebased #
Messages
Total messages: 62 (23 generated)
eladalon@webrtc.org changed reviewers: + danilchap@webrtc.org, nisse@webrtc.org, stefan@webrtc.org
PTAL
One initial question. I haven't looked into the implementation yet. https://codereview.webrtc.org/2943693003/diff/20001/webrtc/call/rsid_ssrc_ass... File webrtc/call/rsid_ssrc_association_resolution_observer.h (right): https://codereview.webrtc.org/2943693003/diff/20001/webrtc/call/rsid_ssrc_ass... webrtc/call/rsid_ssrc_association_resolution_observer.h:21: class RsidResolutionObserver { For consistency, also rename the file to "rsid_resolution_observer.h". (And the include guards). https://codereview.webrtc.org/2943693003/diff/20001/webrtc/call/rtcp_demuxer.h File webrtc/call/rtcp_demuxer.h (right): https://codereview.webrtc.org/2943693003/diff/20001/webrtc/call/rtcp_demuxer.... webrtc/call/rtcp_demuxer.h:73: std::multimap<uint32_t, RtcpPacketSinkInterface*> ssrc_sinks_; I'm not sure what the use cases are, but do we really need multimaps here and below?
https://codereview.webrtc.org/2943693003/diff/20001/webrtc/call/rsid_ssrc_ass... File webrtc/call/rsid_ssrc_association_resolution_observer.h (right): https://codereview.webrtc.org/2943693003/diff/20001/webrtc/call/rsid_ssrc_ass... webrtc/call/rsid_ssrc_association_resolution_observer.h:21: class RsidResolutionObserver { On 2017/06/16 13:18:34, nisse-webrtc wrote: > For consistency, also rename the file to "rsid_resolution_observer.h". (And the > include guards). Done. https://codereview.webrtc.org/2943693003/diff/20001/webrtc/call/rtcp_demuxer.h File webrtc/call/rtcp_demuxer.h (right): https://codereview.webrtc.org/2943693003/diff/20001/webrtc/call/rtcp_demuxer.... webrtc/call/rtcp_demuxer.h:73: std::multimap<uint32_t, RtcpPacketSinkInterface*> ssrc_sinks_; On 2017/06/16 13:18:34, nisse-webrtc wrote: > I'm not sure what the use cases are, but do we really need multimaps here and > below? For posterity - discussed offline between Dani, Elad and Niels, and the conclusion was to keep this a multimap.
https://codereview.webrtc.org/2943693003/diff/20001/webrtc/call/rtcp_demuxer.h File webrtc/call/rtcp_demuxer.h (right): https://codereview.webrtc.org/2943693003/diff/20001/webrtc/call/rtcp_demuxer.... webrtc/call/rtcp_demuxer.h:73: std::multimap<uint32_t, RtcpPacketSinkInterface*> ssrc_sinks_; On 2017/06/16 15:18:43, eladalon wrote: > On 2017/06/16 13:18:34, nisse-webrtc wrote: > > I'm not sure what the use cases are, but do we really need multimaps here and > > below? > > For posterity - discussed offline between Dani, Elad and Niels, and the > conclusion was to keep this a multimap. *Danil
https://codereview.webrtc.org/2943693003/diff/20001/webrtc/call/rsid_ssrc_ass... File webrtc/call/rsid_ssrc_association_resolution_observer.h (right): https://codereview.webrtc.org/2943693003/diff/20001/webrtc/call/rsid_ssrc_ass... webrtc/call/rsid_ssrc_association_resolution_observer.h:20: // The resolution might either happen during call setup, or during the call. Is there a use case where rsid resolution passed not between rtp_demuxer and rtcp_demuxer? If not, may it will be simpler design to merge rtp_demuxer and rtcp_demuxer into single class and pass resolution internally. https://codereview.webrtc.org/2943693003/diff/20001/webrtc/call/rtp_demuxer.h File webrtc/call/rtp_demuxer.h (right): https://codereview.webrtc.org/2943693003/diff/20001/webrtc/call/rtp_demuxer.h... webrtc/call/rtp_demuxer.h:19: #include "webrtc/call/rsid_ssrc_association_resolution_observer.h" prefer to forward declare over include when it is simple https://codereview.webrtc.org/2943693003/diff/20001/webrtc/call/rtp_demuxer.h... webrtc/call/rtp_demuxer.h:52: // Notifications received from outside are *not* propagated. (The ability may be add this half of the comment when RtpDemuxer will have the ability https://codereview.webrtc.org/2943693003/diff/20001/webrtc/call/rtp_demuxer_u... File webrtc/call/rtp_demuxer_unittest.cc (right): https://codereview.webrtc.org/2943693003/diff/20001/webrtc/call/rtp_demuxer_u... webrtc/call/rtp_demuxer_unittest.cc:128: constexpr uint32_t ssrc = 101; avoid doing small unrated changes in a large CL. While I agree with this test change, I wonder how is it related to rtcp demuxer. https://codereview.webrtc.org/2943693003/diff/20001/webrtc/call/rtp_packet_si... File webrtc/call/rtp_packet_sink_interface.h (right): https://codereview.webrtc.org/2943693003/diff/20001/webrtc/call/rtp_packet_si... webrtc/call/rtp_packet_sink_interface.h:17: // This class represents a receiver of an already parsed RTP packet. why this change? interface designed for classes that will receive lots of packets. https://codereview.webrtc.org/2943693003/diff/40001/webrtc/call/rtcp_demuxer_... File webrtc/call/rtcp_demuxer_unittest.cc (right): https://codereview.webrtc.org/2943693003/diff/40001/webrtc/call/rtcp_demuxer_... webrtc/call/rtcp_demuxer_unittest.cc:39: MATCHER_P(SameAs, other, "") { there is a matcher that checks same (but with a detailed error message): ElementsAreArray https://codereview.webrtc.org/2943693003/diff/40001/webrtc/call/rtcp_demuxer_... webrtc/call/rtcp_demuxer_unittest.cc:60: auto packet = rtc::MakeUnique<rtcp::Bye>(); why create packet on the heap instead of in the stack? i.e. may be rtcp::Bye packet; ... return packet.Build(); https://codereview.webrtc.org/2943693003/diff/40001/webrtc/call/rtcp_demuxer_... webrtc/call/rtcp_demuxer_unittest.cc:74: rtc::ArrayView<const uint8_t> GetArrayView(const rtc::Buffer& buffer) { one of the the key features of ArrayView - it has implicit conversion from anything that has data() and size() functions (including rtc::Buffer) https://codereview.webrtc.org/2943693003/diff/40001/webrtc/call/rtcp_packet_s... File webrtc/call/rtcp_packet_sink_interface.h (right): https://codereview.webrtc.org/2943693003/diff/40001/webrtc/call/rtcp_packet_s... webrtc/call/rtcp_packet_sink_interface.h:21: class RtcpPacketSinkInterface { May be it will be simpler to merge this sink into RtpPacketSinkInterface, but instead of making it pure virtual interface, have default implementations that do nothing (that will be usuful when OnRtcpPacket will be split in many individual rtcp messages - sink will need to implement only those it cares about). That might remove complexity of observers (registering them, ensuring none left at destruction, etc) and remove code duplication between rtp_demuxer and rtcp_demuxer. Do you know a use case that will be missed with that design? https://codereview.webrtc.org/2943693003/diff/40001/webrtc/call/rtp_rtcp_demu... File webrtc/call/rtp_rtcp_demuxer_helper.cc (right): https://codereview.webrtc.org/2943693003/diff/40001/webrtc/call/rtp_rtcp_demu... webrtc/call/rtp_rtcp_demuxer_helper.cc:39: // Sender SSRC at the beginning of of the RTCP payload. of of -> of https://codereview.webrtc.org/2943693003/diff/40001/webrtc/call/rtp_rtcp_demu... File webrtc/call/rtp_rtcp_demuxer_helper.h (right): https://codereview.webrtc.org/2943693003/diff/40001/webrtc/call/rtp_rtcp_demu... webrtc/call/rtp_rtcp_demuxer_helper.h:34: size_t RemoveFromMultimapByValue(Container* multimap, const Value value) { const Value& value or Value value https://codereview.webrtc.org/2943693003/diff/40001/webrtc/call/rtp_rtcp_demu... webrtc/call/rtp_rtcp_demuxer_helper.h:47: template <typename Container, typename Key = typename Container::key_type> why do you need default value for Key? since both template parameters are used as function parameters, compile can find out them automatically. https://codereview.webrtc.org/2943693003/diff/40001/webrtc/call/rtp_rtcp_demu... webrtc/call/rtp_rtcp_demuxer_helper.h:62: bool ParseSenderSsrc(rtc::ArrayView<const uint8_t> packet, Ensure function name mention it extract ssrc from an rtcp packet. consider returning rtc::Optional<uint32_t> instead of having an output parameter. https://codereview.webrtc.org/2943693003/diff/40001/webrtc/call/rtp_rtcp_demu... File webrtc/call/rtp_rtcp_demuxer_helper_unittest.cc (right): https://codereview.webrtc.org/2943693003/diff/40001/webrtc/call/rtp_rtcp_demu... webrtc/call/rtp_rtcp_demuxer_helper_unittest.cc:40: EXPECT_TRUE(ParseSenderSsrc(array, &ssrc)); you should be able to pass buffer directly, without converting it to an array https://codereview.webrtc.org/2943693003/diff/40001/webrtc/call/rtp_rtcp_demu... webrtc/call/rtp_rtcp_demuxer_helper_unittest.cc:97: TEST(RtpRtcpDemuxerHelperTest, ParseSenderSsrc_BadRtcpPacket) { may be test for other kind of bad packets too: e.g. valid header but only 2 bytes of rtcp payload.
https://codereview.webrtc.org/2943693003/diff/40001/webrtc/call/rtcp_packet_s... File webrtc/call/rtcp_packet_sink_interface.h (right): https://codereview.webrtc.org/2943693003/diff/40001/webrtc/call/rtcp_packet_s... webrtc/call/rtcp_packet_sink_interface.h:21: class RtcpPacketSinkInterface { On 2017/06/16 16:46:19, danilchap wrote: > May be it will be simpler to merge this sink into RtpPacketSinkInterface, but > instead of making it pure virtual interface, have default implementations that > do nothing (that will be usuful when OnRtcpPacket will be split in many > individual rtcp messages - sink will need to implement only those it cares > about). > > That might remove complexity of observers (registering them, ensuring none left > at destruction, etc) and remove code duplication between rtp_demuxer and > rtcp_demuxer. > > Do you know a use case that will be missed with that design? I prefer to keep RtpPacketSinkInterface independent of RTCP. I expect RTCP to be terminated at a higher level in the stack than RTP, and that many implementations of RtpPacketSinkInterface wouldn't care at all about RTCP. Sure, default implementations of the methods would work, but I think we'd lose clarity in that it gets less obvious which classes care about RTCP. And we were just able to drop the RTCP callback from the related class RtpData (cl https://codereview.webrtc.org/2886813002)...
https://codereview.webrtc.org/2943693003/diff/20001/webrtc/call/rsid_ssrc_ass... File webrtc/call/rsid_ssrc_association_resolution_observer.h (right): https://codereview.webrtc.org/2943693003/diff/20001/webrtc/call/rsid_ssrc_ass... webrtc/call/rsid_ssrc_association_resolution_observer.h:20: // The resolution might either happen during call setup, or during the call. On 2017/06/16 16:46:18, danilchap wrote: > Is there a use case where rsid resolution passed not between rtp_demuxer and > rtcp_demuxer? > If not, may it will be simpler design to merge rtp_demuxer and rtcp_demuxer into > single class and pass resolution internally. Do you mean have one class that has one RtpDemuxer and one RtcpDemuxer? If so, I'd be open to the idea, although I think maybe the boilerplate we'll need makes it no longer worth it. But if you mean to merge everything into a single class, RtpRtcpDemuxer, where all the logic lies, I'd vote against. I think things are clearer and more maintainable like this; despite the similarity of the two classes, we won't be eliminating any code when merging, except for the Register/Deregister-observer functions. https://codereview.webrtc.org/2943693003/diff/20001/webrtc/call/rtp_demuxer.h File webrtc/call/rtp_demuxer.h (right): https://codereview.webrtc.org/2943693003/diff/20001/webrtc/call/rtp_demuxer.h... webrtc/call/rtp_demuxer.h:19: #include "webrtc/call/rsid_ssrc_association_resolution_observer.h" On 2017/06/16 16:46:18, danilchap wrote: > prefer to forward declare over include when it is simple Done. https://codereview.webrtc.org/2943693003/diff/20001/webrtc/call/rtp_demuxer.h... webrtc/call/rtp_demuxer.h:52: // Notifications received from outside are *not* propagated. (The ability On 2017/06/16 16:46:18, danilchap wrote: > may be add this half of the comment when RtpDemuxer will have the ability Done. https://codereview.webrtc.org/2943693003/diff/20001/webrtc/call/rtp_demuxer_u... File webrtc/call/rtp_demuxer_unittest.cc (right): https://codereview.webrtc.org/2943693003/diff/20001/webrtc/call/rtp_demuxer_u... webrtc/call/rtp_demuxer_unittest.cc:128: constexpr uint32_t ssrc = 101; On 2017/06/16 16:46:18, danilchap wrote: > avoid doing small unrated changes in a large CL. > While I agree with this test change, I wonder how is it related to rtcp demuxer. I understand; I'll try to do so in the future. FYI, the way it's related is - I adapted some of the tests for the RTCP case, and when adapting this one, noticed it could be improved. https://codereview.webrtc.org/2943693003/diff/20001/webrtc/call/rtp_packet_si... File webrtc/call/rtp_packet_sink_interface.h (right): https://codereview.webrtc.org/2943693003/diff/20001/webrtc/call/rtp_packet_si... webrtc/call/rtp_packet_sink_interface.h:17: // This class represents a receiver of an already parsed RTP packet. On 2017/06/16 16:46:18, danilchap wrote: > why this change? > interface designed for classes that will receive lots of packets. It's purely a linguistic correction. We either drop the S from "packets" or the indefinite article ("an") that precedes it. I'll drop the "an", then. https://codereview.webrtc.org/2943693003/diff/40001/webrtc/call/rtcp_demuxer_... File webrtc/call/rtcp_demuxer_unittest.cc (right): https://codereview.webrtc.org/2943693003/diff/40001/webrtc/call/rtcp_demuxer_... webrtc/call/rtcp_demuxer_unittest.cc:39: MATCHER_P(SameAs, other, "") { On 2017/06/16 16:46:19, danilchap wrote: > there is a matcher that checks same (but with a detailed error message): > ElementsAreArray Done. https://codereview.webrtc.org/2943693003/diff/40001/webrtc/call/rtcp_demuxer_... webrtc/call/rtcp_demuxer_unittest.cc:60: auto packet = rtc::MakeUnique<rtcp::Bye>(); On 2017/06/16 16:46:19, danilchap wrote: > why create packet on the heap instead of in the stack? > i.e. may be > rtcp::Bye packet; > ... > return packet.Build(); Right you are. An earlier version of this function returned the rtcp::Bye itself, so that's why it was written like that. After transitioning to returning rtc::Buffer, it's indeed unnecessary. https://codereview.webrtc.org/2943693003/diff/40001/webrtc/call/rtcp_demuxer_... webrtc/call/rtcp_demuxer_unittest.cc:74: rtc::ArrayView<const uint8_t> GetArrayView(const rtc::Buffer& buffer) { On 2017/06/16 16:46:19, danilchap wrote: > one of the the key features of ArrayView - it has implicit conversion from > anything that has data() and size() functions (including rtc::Buffer) Acknowledged (and used). https://codereview.webrtc.org/2943693003/diff/40001/webrtc/call/rtcp_packet_s... File webrtc/call/rtcp_packet_sink_interface.h (right): https://codereview.webrtc.org/2943693003/diff/40001/webrtc/call/rtcp_packet_s... webrtc/call/rtcp_packet_sink_interface.h:21: class RtcpPacketSinkInterface { On 2017/06/19 08:26:51, nisse-webrtc wrote: > On 2017/06/16 16:46:19, danilchap wrote: > > May be it will be simpler to merge this sink into RtpPacketSinkInterface, but > > instead of making it pure virtual interface, have default implementations that > > do nothing (that will be usuful when OnRtcpPacket will be split in many > > individual rtcp messages - sink will need to implement only those it cares > > about). > > > > That might remove complexity of observers (registering them, ensuring none > left > > at destruction, etc) and remove code duplication between rtp_demuxer and > > rtcp_demuxer. > > > > Do you know a use case that will be missed with that design? > > I prefer to keep RtpPacketSinkInterface independent of RTCP. I expect RTCP to be > terminated at a higher level in the stack than RTP, and that many > implementations of RtpPacketSinkInterface wouldn't care at all about RTCP. Sure, > default implementations of the methods would work, but I think we'd lose clarity > in that it gets less obvious which classes care about RTCP. > > And we were just able to drop the RTCP callback from the related class RtpData > (cl https://codereview.webrtc.org/2886813002)... I'll add that multiple calls to a virtual function that ends up doing nothing are nice to avoid. https://codereview.webrtc.org/2943693003/diff/40001/webrtc/call/rtp_rtcp_demu... File webrtc/call/rtp_rtcp_demuxer_helper.cc (right): https://codereview.webrtc.org/2943693003/diff/40001/webrtc/call/rtp_rtcp_demu... webrtc/call/rtp_rtcp_demuxer_helper.cc:39: // Sender SSRC at the beginning of of the RTCP payload. On 2017/06/16 16:46:19, danilchap wrote: > of of -> of Done. https://codereview.webrtc.org/2943693003/diff/40001/webrtc/call/rtp_rtcp_demu... File webrtc/call/rtp_rtcp_demuxer_helper.h (right): https://codereview.webrtc.org/2943693003/diff/40001/webrtc/call/rtp_rtcp_demu... webrtc/call/rtp_rtcp_demuxer_helper.h:34: size_t RemoveFromMultimapByValue(Container* multimap, const Value value) { On 2017/06/16 16:46:19, danilchap wrote: > const Value& value > or > Value value (const Value&) was intended; thanks for catching. https://codereview.webrtc.org/2943693003/diff/40001/webrtc/call/rtp_rtcp_demu... webrtc/call/rtp_rtcp_demuxer_helper.h:47: template <typename Container, typename Key = typename Container::key_type> On 2017/06/16 16:46:19, danilchap wrote: > why do you need default value for Key? > since both template parameters are used as function parameters, compile can find > out them automatically. I was thinking of this as self-documenting code, but okay - removed. https://codereview.webrtc.org/2943693003/diff/40001/webrtc/call/rtp_rtcp_demu... webrtc/call/rtp_rtcp_demuxer_helper.h:62: bool ParseSenderSsrc(rtc::ArrayView<const uint8_t> packet, On 2017/06/16 16:46:19, danilchap wrote: > Ensure function name mention it extract ssrc from an rtcp packet. > > consider returning rtc::Optional<uint32_t> instead of having an output > parameter. Done. https://codereview.webrtc.org/2943693003/diff/40001/webrtc/call/rtp_rtcp_demu... File webrtc/call/rtp_rtcp_demuxer_helper_unittest.cc (right): https://codereview.webrtc.org/2943693003/diff/40001/webrtc/call/rtp_rtcp_demu... webrtc/call/rtp_rtcp_demuxer_helper_unittest.cc:40: EXPECT_TRUE(ParseSenderSsrc(array, &ssrc)); On 2017/06/16 16:46:19, danilchap wrote: > you should be able to pass buffer directly, without converting it to an array Done. https://codereview.webrtc.org/2943693003/diff/40001/webrtc/call/rtp_rtcp_demu... webrtc/call/rtp_rtcp_demuxer_helper_unittest.cc:97: TEST(RtpRtcpDemuxerHelperTest, ParseSenderSsrc_BadRtcpPacket) { On 2017/06/16 16:46:19, danilchap wrote: > may be test for other kind of bad packets too: > e.g. valid header but only 2 bytes of rtcp payload. IMHO, that would be the sink's problem; the demuxer should have as little knowledge about RTCP, and what constitutes a valid RTCP packet, as possible. Also, once we move to passing parsed packets instead of raw, this validation will already have been done before this module is reached. Wdyt?
https://codereview.webrtc.org/2943693003/diff/60001/webrtc/call/BUILD.gn File webrtc/call/BUILD.gn (right): https://codereview.webrtc.org/2943693003/diff/60001/webrtc/call/BUILD.gn#newc... webrtc/call/BUILD.gn:38: # TODO(eladalon): Rename rtc_source_set? Poll reviewers. Any thoughts here?
lgtm https://codereview.webrtc.org/2943693003/diff/60001/webrtc/call/BUILD.gn File webrtc/call/BUILD.gn (right): https://codereview.webrtc.org/2943693003/diff/60001/webrtc/call/BUILD.gn#newc... webrtc/call/BUILD.gn:38: # TODO(eladalon): Rename rtc_source_set? Poll reviewers. On 2017/06/19 11:31:53, eladalon wrote: > Any thoughts here? You should probably ask kjellander@, I don't know the context here.
https://codereview.webrtc.org/2943693003/diff/80001/webrtc/call/rtp_demuxer_u... File webrtc/call/rtp_demuxer_unittest.cc (right): https://codereview.webrtc.org/2943693003/diff/80001/webrtc/call/rtp_demuxer_u... webrtc/call/rtp_demuxer_unittest.cc:524: TEST(RtpDemuxerTest, NotificationSuppressionResetWhenNewObserverAdded) { do you have any (practical) scenario where you want to rely on this feature? https://codereview.webrtc.org/2943693003/diff/80001/webrtc/call/rtp_rtcp_demu... File webrtc/call/rtp_rtcp_demuxer_helper.cc (right): https://codereview.webrtc.org/2943693003/diff/80001/webrtc/call/rtp_rtcp_demu... webrtc/call/rtp_rtcp_demuxer_helper.cc:41: webrtc::ByteReader<uint32_t>::ReadBigEndian(header.payload()); drop webrtc:: prefix https://codereview.webrtc.org/2943693003/diff/80001/webrtc/call/rtp_rtcp_demu... File webrtc/call/rtp_rtcp_demuxer_helper.h (right): https://codereview.webrtc.org/2943693003/diff/80001/webrtc/call/rtp_rtcp_demu... webrtc/call/rtp_rtcp_demuxer_helper.h:26: typename Value = typename Container::value_type> either do not specify default type for Value, or use Container::mapped_type since value_type is something different for the multimap https://codereview.webrtc.org/2943693003/diff/80001/webrtc/call/rtp_rtcp_demu... webrtc/call/rtp_rtcp_demuxer_helper.h:34: template <typename Container, typename Value = typename Container::value_type> ditto https://codereview.webrtc.org/2943693003/diff/80001/webrtc/call/rtp_rtcp_demu... webrtc/call/rtp_rtcp_demuxer_helper.h:55: typename Value = typename Container::value_type> ditto https://codereview.webrtc.org/2943693003/diff/80001/webrtc/call/rtp_rtcp_demu... webrtc/call/rtp_rtcp_demuxer_helper.h:57: auto predicate = [v](const std::pair<Key, Value>& it) { to avoid implicit conversion instead of std::pair<Key, Value> use Container::value_type (which is std::pair<const Key, Value> for map/multimap) https://codereview.webrtc.org/2943693003/diff/80001/webrtc/call/rtp_rtcp_demu... File webrtc/call/rtp_rtcp_demuxer_helper_unittest.cc (right): https://codereview.webrtc.org/2943693003/diff/80001/webrtc/call/rtp_rtcp_demu... webrtc/call/rtp_rtcp_demuxer_helper_unittest.cc:31: TEST(RtpRtcpDemuxerHelperTest, ParseRtcpPacketSenderSsrc_CorrectOutputForBye) { CorrectOutput are probably redundant words. ParseRtcpPacketSenderSsrcForBye looks as descriptive and like a full sentence. https://codereview.webrtc.org/2943693003/diff/80001/webrtc/call/rtp_rtcp_demu... webrtc/call/rtp_rtcp_demuxer_helper_unittest.cc:37: EXPECT_TRUE(ssrc); if you'll keep it, use ASSERT_TRUE may be (to be a bit more explicit) ASSERT_TRUE(ssrc.has_value()); https://codereview.webrtc.org/2943693003/diff/80001/webrtc/call/rtp_rtcp_demu... webrtc/call/rtp_rtcp_demuxer_helper_unittest.cc:38: EXPECT_EQ(*ssrc, kSsrc); you may replace these two EXPECTs with single one: EXPECT_EQ(ssrc, kSsrc);
https://codereview.webrtc.org/2943693003/diff/80001/webrtc/call/rtp_demuxer_u... File webrtc/call/rtp_demuxer_unittest.cc (right): https://codereview.webrtc.org/2943693003/diff/80001/webrtc/call/rtp_demuxer_u... webrtc/call/rtp_demuxer_unittest.cc:524: TEST(RtpDemuxerTest, NotificationSuppressionResetWhenNewObserverAdded) { On 2017/06/19 13:31:56, danilchap wrote: > do you have any (practical) scenario where you want to rely on this feature? The line between practical and theoretical is blurry. Here are some possibilities: 1. RTP packets arriving before negotiation is complete? A callee might be sending RTP messages right after it's produced the ANSWER, before the caller has managed to get it (maybe it got lost, maybe delayed). 2. The Proxy-like class we've discussed for PlType-based demuxing. Besides, I feel things are more correct if we choose either: * Disallow further observer registration after RTP demuxing has started. * Give new observers the full service they expect. The second seems both cleaner as well as more useful. https://codereview.webrtc.org/2943693003/diff/80001/webrtc/call/rtp_rtcp_demu... File webrtc/call/rtp_rtcp_demuxer_helper.cc (right): https://codereview.webrtc.org/2943693003/diff/80001/webrtc/call/rtp_rtcp_demu... webrtc/call/rtp_rtcp_demuxer_helper.cc:41: webrtc::ByteReader<uint32_t>::ReadBigEndian(header.payload()); On 2017/06/19 13:31:56, danilchap wrote: > drop webrtc:: prefix Done. https://codereview.webrtc.org/2943693003/diff/80001/webrtc/call/rtp_rtcp_demu... File webrtc/call/rtp_rtcp_demuxer_helper.h (right): https://codereview.webrtc.org/2943693003/diff/80001/webrtc/call/rtp_rtcp_demu... webrtc/call/rtp_rtcp_demuxer_helper.h:26: typename Value = typename Container::value_type> On 2017/06/19 13:31:56, danilchap wrote: > either do not specify default type for Value, > or use Container::mapped_type since value_type is something different for the > multimap Done. https://codereview.webrtc.org/2943693003/diff/80001/webrtc/call/rtp_rtcp_demu... webrtc/call/rtp_rtcp_demuxer_helper.h:34: template <typename Container, typename Value = typename Container::value_type> On 2017/06/19 13:31:56, danilchap wrote: > ditto Done. https://codereview.webrtc.org/2943693003/diff/80001/webrtc/call/rtp_rtcp_demu... webrtc/call/rtp_rtcp_demuxer_helper.h:55: typename Value = typename Container::value_type> On 2017/06/19 13:31:56, danilchap wrote: > ditto Done. https://codereview.webrtc.org/2943693003/diff/80001/webrtc/call/rtp_rtcp_demu... webrtc/call/rtp_rtcp_demuxer_helper.h:57: auto predicate = [v](const std::pair<Key, Value>& it) { On 2017/06/19 13:31:56, danilchap wrote: > to avoid implicit conversion instead of std::pair<Key, Value> use > Container::value_type > (which is std::pair<const Key, Value> for map/multimap) Done. https://codereview.webrtc.org/2943693003/diff/80001/webrtc/call/rtp_rtcp_demu... File webrtc/call/rtp_rtcp_demuxer_helper_unittest.cc (right): https://codereview.webrtc.org/2943693003/diff/80001/webrtc/call/rtp_rtcp_demu... webrtc/call/rtp_rtcp_demuxer_helper_unittest.cc:31: TEST(RtpRtcpDemuxerHelperTest, ParseRtcpPacketSenderSsrc_CorrectOutputForBye) { On 2017/06/19 13:31:56, danilchap wrote: > CorrectOutput are probably redundant words. > ParseRtcpPacketSenderSsrcForBye > looks as descriptive and like a full sentence. I'll go with ParseRtcpPacketSenderSsrc_ByePacket, etc. https://codereview.webrtc.org/2943693003/diff/80001/webrtc/call/rtp_rtcp_demu... webrtc/call/rtp_rtcp_demuxer_helper_unittest.cc:37: EXPECT_TRUE(ssrc); On 2017/06/19 13:31:56, danilchap wrote: > if you'll keep it, use ASSERT_TRUE > may be (to be a bit more explicit) ASSERT_TRUE(ssrc.has_value()); I prefer without the .has_true(), as that's what's usually used in the part of the codebase I'm currently most familiar with. (I'm not sure what's common elsewhere.) As for the assertion - right you are; it would crash otherwise. https://codereview.webrtc.org/2943693003/diff/80001/webrtc/call/rtp_rtcp_demu... webrtc/call/rtp_rtcp_demuxer_helper_unittest.cc:38: EXPECT_EQ(*ssrc, kSsrc); On 2017/06/19 13:31:56, danilchap wrote: > you may replace these two EXPECTs with single one: > EXPECT_EQ(ssrc, kSsrc); 1. Still need to assert before. 2. I'm used to relying on rtc::Optional's implicit conversion to a bool, but not to anything else; I prefer to keep the *.
https://codereview.webrtc.org/2943693003/diff/80001/webrtc/call/rtp_rtcp_demu... File webrtc/call/rtp_rtcp_demuxer_helper_unittest.cc (right): https://codereview.webrtc.org/2943693003/diff/80001/webrtc/call/rtp_rtcp_demu... webrtc/call/rtp_rtcp_demuxer_helper_unittest.cc:37: EXPECT_TRUE(ssrc); On 2017/06/19 14:35:17, eladalon wrote: > On 2017/06/19 13:31:56, danilchap wrote: > > if you'll keep it, use ASSERT_TRUE > > may be (to be a bit more explicit) ASSERT_TRUE(ssrc.has_value()); > > I prefer without the .has_true(), as that's what's usually used in the part of > the codebase I'm currently most familiar with. (I'm not sure what's common > elsewhere.) .has_value was added only recently, that's one of the reasons why it is rare in the codebase. But if you prefer implicit conversion to bool over .has_value() - I wouldn't object. https://codereview.webrtc.org/2943693003/diff/80001/webrtc/call/rtp_rtcp_demu... webrtc/call/rtp_rtcp_demuxer_helper_unittest.cc:38: EXPECT_EQ(*ssrc, kSsrc); On 2017/06/19 14:35:17, eladalon wrote: > On 2017/06/19 13:31:56, danilchap wrote: > > you may replace these two EXPECTs with single one: > > EXPECT_EQ(ssrc, kSsrc); > > 1. Still need to assert before. > 2. I'm used to relying on rtc::Optional's implicit conversion to a bool, but not > to anything else; I prefer to keep the *. 1. No 2. std::optional<T> allowed to be compared to T without conversions (with empty optional never equal to a value). rtc::Optional has explicit implementation of that feature for equality. If you still prefer to keep * (or use .value() accessor) - I wouldn't mind. Just want you to be aware about the alternative. https://codereview.webrtc.org/2943693003/diff/100001/webrtc/call/BUILD.gn File webrtc/call/BUILD.gn (right): https://codereview.webrtc.org/2943693003/diff/100001/webrtc/call/BUILD.gn#new... webrtc/call/BUILD.gn:38: # TODO(eladalon): Rename rtc_source_set? Poll reviewers. vote: keep as is for now. (remove TODO in favor of the TODO just above) https://codereview.webrtc.org/2943693003/diff/100001/webrtc/call/rtcp_demuxer.h File webrtc/call/rtcp_demuxer.h (right): https://codereview.webrtc.org/2943693003/diff/100001/webrtc/call/rtcp_demuxer... webrtc/call/rtcp_demuxer.h:51: // Sinks may be associated with both an SSRC and an RSID. this line doesn't apply to broadcast sinks https://codereview.webrtc.org/2943693003/diff/100001/webrtc/call/rtcp_demuxer... File webrtc/call/rtcp_demuxer_unittest.cc (right): https://codereview.webrtc.org/2943693003/diff/100001/webrtc/call/rtcp_demuxer... webrtc/call/rtcp_demuxer_unittest.cc:33: using ::webrtc::rtcp::RtcpPacket; used? https://codereview.webrtc.org/2943693003/diff/100001/webrtc/call/rtcp_demuxer... webrtc/call/rtcp_demuxer_unittest.cc:49: if (distinguishing_string != "") { why check? https://codereview.webrtc.org/2943693003/diff/100001/webrtc/call/rtcp_demuxer... webrtc/call/rtcp_demuxer_unittest.cc:285: packets.emplace_back(CreateRtcpPacket(ssrcs[i])); why emplace_back instead of push_back? (packets::value_type is same type CreateRtcpPacket returns) https://codereview.webrtc.org/2943693003/diff/100001/webrtc/call/rtcp_demuxer... webrtc/call/rtcp_demuxer_unittest.cc:344: EXPECT_CALL(sink, OnRtcpPacket(_)).Times(0); // Not called. may be remove the comment - it just repeats '.Times(0)' https://codereview.webrtc.org/2943693003/diff/100001/webrtc/call/rtp_rtcp_dem... File webrtc/call/rtp_rtcp_demuxer_helper.cc (right): https://codereview.webrtc.org/2943693003/diff/100001/webrtc/call/rtp_rtcp_dem... webrtc/call/rtp_rtcp_demuxer_helper.cc:39: // Sender SSRC at the beginning of the RTCP payload. may be add rtcp::ExtendedReports
https://codereview.webrtc.org/2943693003/diff/80001/webrtc/call/rtp_rtcp_demu... File webrtc/call/rtp_rtcp_demuxer_helper_unittest.cc (right): https://codereview.webrtc.org/2943693003/diff/80001/webrtc/call/rtp_rtcp_demu... webrtc/call/rtp_rtcp_demuxer_helper_unittest.cc:37: EXPECT_TRUE(ssrc); On 2017/06/19 16:24:54, danilchap wrote: > On 2017/06/19 14:35:17, eladalon wrote: > > On 2017/06/19 13:31:56, danilchap wrote: > > > if you'll keep it, use ASSERT_TRUE > > > may be (to be a bit more explicit) ASSERT_TRUE(ssrc.has_value()); > > > > I prefer without the .has_true(), as that's what's usually used in the part of > > the codebase I'm currently most familiar with. (I'm not sure what's common > > elsewhere.) > .has_value was added only recently, that's one of the reasons why it is rare in > the codebase. > But if you prefer implicit conversion to bool over .has_value() - I wouldn't > object. > Acknowledged. https://codereview.webrtc.org/2943693003/diff/80001/webrtc/call/rtp_rtcp_demu... webrtc/call/rtp_rtcp_demuxer_helper_unittest.cc:38: EXPECT_EQ(*ssrc, kSsrc); On 2017/06/19 16:24:54, danilchap wrote: > On 2017/06/19 14:35:17, eladalon wrote: > > On 2017/06/19 13:31:56, danilchap wrote: > > > you may replace these two EXPECTs with single one: > > > EXPECT_EQ(ssrc, kSsrc); > > > > 1. Still need to assert before. > > 2. I'm used to relying on rtc::Optional's implicit conversion to a bool, but > not > > to anything else; I prefer to keep the *. > > 1. No > 2. std::optional<T> allowed to be compared to T without conversions (with empty > optional never equal to a value). > rtc::Optional has explicit implementation of that feature for equality. > If you still prefer to keep * (or use .value() accessor) - I wouldn't mind. Just > want you to be aware about the alternative. I see now that it prints "empty optional" when comparing an empty optional with a specific value, so long as the optional is compared directly, and not using operator*. Cool; I'll use that. https://codereview.webrtc.org/2943693003/diff/100001/webrtc/call/BUILD.gn File webrtc/call/BUILD.gn (right): https://codereview.webrtc.org/2943693003/diff/100001/webrtc/call/BUILD.gn#new... webrtc/call/BUILD.gn:38: # TODO(eladalon): Rename rtc_source_set? Poll reviewers. On 2017/06/19 16:24:54, danilchap wrote: > vote: keep as is for now. (remove TODO in favor of the TODO just above) Done. https://codereview.webrtc.org/2943693003/diff/100001/webrtc/call/rtcp_demuxer.h File webrtc/call/rtcp_demuxer.h (right): https://codereview.webrtc.org/2943693003/diff/100001/webrtc/call/rtcp_demuxer... webrtc/call/rtcp_demuxer.h:51: // Sinks may be associated with both an SSRC and an RSID. On 2017/06/19 16:24:54, danilchap wrote: > this line doesn't apply to broadcast sinks Done. https://codereview.webrtc.org/2943693003/diff/100001/webrtc/call/rtcp_demuxer... File webrtc/call/rtcp_demuxer_unittest.cc (right): https://codereview.webrtc.org/2943693003/diff/100001/webrtc/call/rtcp_demuxer... webrtc/call/rtcp_demuxer_unittest.cc:33: using ::webrtc::rtcp::RtcpPacket; Not anymore (I think I was expecting to use this for parsing). Removed. https://codereview.webrtc.org/2943693003/diff/100001/webrtc/call/rtcp_demuxer... webrtc/call/rtcp_demuxer_unittest.cc:49: if (distinguishing_string != "") { On 2017/06/19 16:24:54, danilchap wrote: > why check? I don't want to assume the implementation of Bye does not check against empty reason strings, which, without checking the relevant specifications, sounds like a potentially reasonable (no pun intended) thing to do. https://codereview.webrtc.org/2943693003/diff/100001/webrtc/call/rtcp_demuxer... webrtc/call/rtcp_demuxer_unittest.cc:285: packets.emplace_back(CreateRtcpPacket(ssrcs[i])); On 2017/06/19 16:24:55, danilchap wrote: > why emplace_back instead of push_back? > (packets::value_type is same type CreateRtcpPacket returns) The two should behave the same in this case, but I think you're referring to <sent over chat>, so no worries, I'll change. https://codereview.webrtc.org/2943693003/diff/100001/webrtc/call/rtcp_demuxer... webrtc/call/rtcp_demuxer_unittest.cc:344: EXPECT_CALL(sink, OnRtcpPacket(_)).Times(0); // Not called. On 2017/06/19 16:24:54, danilchap wrote: > may be remove the comment - it just repeats '.Times(0)' Feels slightly clearer to me. Times(0) seems a bit like an error otherwise. But I don't feel strongly about this. https://codereview.webrtc.org/2943693003/diff/100001/webrtc/call/rtp_rtcp_dem... File webrtc/call/rtp_rtcp_demuxer_helper.cc (right): https://codereview.webrtc.org/2943693003/diff/100001/webrtc/call/rtp_rtcp_dem... webrtc/call/rtp_rtcp_demuxer_helper.cc:39: // Sender SSRC at the beginning of the RTCP payload. On 2017/06/19 16:24:55, danilchap wrote: > may be add rtcp::ExtendedReports Done.
kjellander@webrtc.org changed reviewers: + kjellander@webrtc.org
Sorry I forgot about this one yesterday. Please add me as reviewer when you want input, or I'm likely to miss/forget about it. https://codereview.webrtc.org/2943693003/diff/60001/webrtc/call/BUILD.gn File webrtc/call/BUILD.gn (right): https://codereview.webrtc.org/2943693003/diff/60001/webrtc/call/BUILD.gn#newc... webrtc/call/BUILD.gn:38: # TODO(eladalon): Rename rtc_source_set? Poll reviewers. On 2017/06/19 11:43:34, nisse-webrtc wrote: > On 2017/06/19 11:31:53, eladalon wrote: > > Any thoughts here? > > You should probably ask kjellander@, I don't know the context here. Is the question about changing from rtc_source_set to rtc_static library? It's a tricky one for which I don't have a good answer - we have both mixed in our repo, mainly because back in the GYP days everything was static_library. This thread explains some more about the differences: https://groups.google.com/a/chromium.org/forum/#!topic/gn-dev/Jn4TbhOtB9k
Sorry I forgot about this one yesterday. Please add me as reviewer when you want input, or I'm likely to miss/forget about it.
https://codereview.webrtc.org/2943693003/diff/60001/webrtc/call/BUILD.gn File webrtc/call/BUILD.gn (right): https://codereview.webrtc.org/2943693003/diff/60001/webrtc/call/BUILD.gn#newc... webrtc/call/BUILD.gn:38: # TODO(eladalon): Rename rtc_source_set? Poll reviewers. On 2017/06/20 07:47:41, kjellander_webrtc wrote: > On 2017/06/19 11:43:34, nisse-webrtc wrote: > > On 2017/06/19 11:31:53, eladalon wrote: > > > Any thoughts here? > > > > You should probably ask kjellander@, I don't know the context here. > > Is the question about changing from rtc_source_set to rtc_static library? It's a > tricky one for which I don't have a good answer - we have both mixed in our > repo, mainly because back in the GYP days everything was static_library. > This thread explains some more about the differences: > https://groups.google.com/a/chromium.org/forum/#!topic/gn-dev/Jn4TbhOtB9k Sorry for the ambiguity; my question was actually whether the name of the rtc_source_set, the current name being "rtp_interfaces", should be changed to something such as "rtp_rtcp_interfaces". This is a smaller change than that which TODO(nisse) discusses, so I thought we could do that, first.
lgtm https://codereview.webrtc.org/2943693003/diff/120001/webrtc/call/rtcp_demuxer... File webrtc/call/rtcp_demuxer_unittest.cc (right): https://codereview.webrtc.org/2943693003/diff/120001/webrtc/call/rtcp_demuxer... webrtc/call/rtcp_demuxer_unittest.cc:13: #include "webrtc/call/rtcp_demuxer.h" put this include above c++ include <memory> https://codereview.webrtc.org/2943693003/diff/120001/webrtc/call/rtcp_demuxer... webrtc/call/rtcp_demuxer_unittest.cc:70: OnRtcpPacket(ElementsAreArray(packet.cbegin(), packet.cend()))) do you want to test packet is passed without memcpy? or that is not guarantee you want to provide? https://codereview.webrtc.org/2943693003/diff/120001/webrtc/call/rtcp_demuxer... webrtc/call/rtcp_demuxer_unittest.cc:204: for (size_t i = 0; i < 5; i++) { may be for (const auto& packet : packets) { https://codereview.webrtc.org/2943693003/diff/120001/webrtc/call/rtcp_packet_... File webrtc/call/rtcp_packet_sink_interface.h (right): https://codereview.webrtc.org/2943693003/diff/120001/webrtc/call/rtcp_packet_... webrtc/call/rtcp_packet_sink_interface.h:23: virtual ~RtcpPacketSinkInterface() {} may be = default instead (like you did in RsidResolutionObserver interface) https://codereview.webrtc.org/2943693003/diff/120001/webrtc/call/rtp_rtcp_dem... File webrtc/call/rtp_rtcp_demuxer_helper.h (right): https://codereview.webrtc.org/2943693003/diff/120001/webrtc/call/rtp_rtcp_dem... webrtc/call/rtp_rtcp_demuxer_helper.h:54: bool AssociativeContainerHasValue(const Container& c, helpers above use 'Muttimap' instead of 'AssociativeContainer' May be you want to use the same name in this file in all helpers.
https://codereview.webrtc.org/2943693003/diff/60001/webrtc/call/BUILD.gn File webrtc/call/BUILD.gn (right): https://codereview.webrtc.org/2943693003/diff/60001/webrtc/call/BUILD.gn#newc... webrtc/call/BUILD.gn:38: # TODO(eladalon): Rename rtc_source_set? Poll reviewers. On 2017/06/20 07:50:18, eladalon wrote: > On 2017/06/20 07:47:41, kjellander_webrtc wrote: > > On 2017/06/19 11:43:34, nisse-webrtc wrote: > > > On 2017/06/19 11:31:53, eladalon wrote: > > > > Any thoughts here? > > > > > > You should probably ask kjellander@, I don't know the context here. > > > > Is the question about changing from rtc_source_set to rtc_static library? It's > a > > tricky one for which I don't have a good answer - we have both mixed in our > > repo, mainly because back in the GYP days everything was static_library. > > This thread explains some more about the differences: > > https://groups.google.com/a/chromium.org/forum/#!topic/gn-dev/Jn4TbhOtB9k > > Sorry for the ambiguity; my question was actually whether the name of the > rtc_source_set, the current name being "rtp_interfaces", should be changed to > something such as "rtp_rtcp_interfaces". Ah, that wasn't obvious. > This is a smaller change than that > which TODO(nisse) discusses, so I thought we could do that, first. I'd say no. In the big picture, rtcp is just one of the pieces of the rtp transport implementation, and there's no need to mention in the name. And we're aiming for an "RtpTransportController", not an "RtpRtcpTransportController".
nisse - sounds right. I've removed the TODO. https://codereview.webrtc.org/2943693003/diff/120001/webrtc/call/rtcp_demuxer... File webrtc/call/rtcp_demuxer_unittest.cc (right): https://codereview.webrtc.org/2943693003/diff/120001/webrtc/call/rtcp_demuxer... webrtc/call/rtcp_demuxer_unittest.cc:13: #include "webrtc/call/rtcp_demuxer.h" On 2017/06/20 13:04:40, danilchap wrote: > put this include above c++ include <memory> Done. https://codereview.webrtc.org/2943693003/diff/120001/webrtc/call/rtcp_demuxer... webrtc/call/rtcp_demuxer_unittest.cc:70: OnRtcpPacket(ElementsAreArray(packet.cbegin(), packet.cend()))) On 2017/06/20 13:04:40, danilchap wrote: > do you want to test packet is passed without memcpy? or that is not guarantee > you want to provide? I don't think we should provide this guarantee. You? https://codereview.webrtc.org/2943693003/diff/120001/webrtc/call/rtcp_demuxer... webrtc/call/rtcp_demuxer_unittest.cc:204: for (size_t i = 0; i < 5; i++) { On 2017/06/20 13:04:40, danilchap wrote: > may be > for (const auto& packet : packets) { Definitely. https://codereview.webrtc.org/2943693003/diff/120001/webrtc/call/rtcp_packet_... File webrtc/call/rtcp_packet_sink_interface.h (right): https://codereview.webrtc.org/2943693003/diff/120001/webrtc/call/rtcp_packet_... webrtc/call/rtcp_packet_sink_interface.h:23: virtual ~RtcpPacketSinkInterface() {} On 2017/06/20 13:04:40, danilchap wrote: > may be = default instead > (like you did in RsidResolutionObserver interface) Gladly. I'll change the RTP interface, then, too. https://codereview.webrtc.org/2943693003/diff/120001/webrtc/call/rtp_rtcp_dem... File webrtc/call/rtp_rtcp_demuxer_helper.h (right): https://codereview.webrtc.org/2943693003/diff/120001/webrtc/call/rtp_rtcp_dem... webrtc/call/rtp_rtcp_demuxer_helper.h:54: bool AssociativeContainerHasValue(const Container& c, On 2017/06/20 13:04:40, danilchap wrote: > helpers above use 'Muttimap' instead of 'AssociativeContainer' > May be you want to use the same name in this file in all helpers. Done.
holmer@google.com changed reviewers: + holmer@google.com
Please expand the description with more details of what this CL is doing. https://codereview.webrtc.org/2943693003/diff/140001/webrtc/call/rtp_rtcp_dem... File webrtc/call/rtp_rtcp_demuxer_helper.cc (right): https://codereview.webrtc.org/2943693003/diff/140001/webrtc/call/rtp_rtcp_dem... webrtc/call/rtp_rtcp_demuxer_helper.cc:29: if (!header.Parse(next_packet, packet.end() - next_packet)) { CHECK that packet.end() - next_packet is positive?
Description was changed from
==========
Create RtcpDemuxer
BUG=None
==========
to
==========
Create RtcpDemuxer. Capabilities:
1. Demux RTCP messages according to the sender-SSRC.
2. Demux RTCP messages according to the RSID (resolved to an SSRC, then compared
to the sender-RTCP).
3. Allow listening in on all RTCP messages passing through the demuxer
("broadcast sinks").
BUG=None
==========
https://codereview.webrtc.org/2943693003/diff/140001/webrtc/call/rtp_rtcp_dem... File webrtc/call/rtp_rtcp_demuxer_helper.cc (right): https://codereview.webrtc.org/2943693003/diff/140001/webrtc/call/rtp_rtcp_dem... webrtc/call/rtp_rtcp_demuxer_helper.cc:29: if (!header.Parse(next_packet, packet.end() - next_packet)) { On 2017/06/21 06:56:58, holmer wrote: > CHECK that packet.end() - next_packet is positive? Done.
lgtm
Please add a BUG=, even if it's a generic "Rtp transport refactoring" bug.
Description was changed from
==========
Create RtcpDemuxer. Capabilities:
1. Demux RTCP messages according to the sender-SSRC.
2. Demux RTCP messages according to the RSID (resolved to an SSRC, then compared
to the sender-RTCP).
3. Allow listening in on all RTCP messages passing through the demuxer
("broadcast sinks").
BUG=None
==========
to
==========
Create RtcpDemuxer. Capabilities:
1. Demux RTCP messages according to the sender-SSRC.
2. Demux RTCP messages according to the RSID (resolved to an SSRC, then compared
to the sender-RTCP).
3. Allow listening in on all RTCP messages passing through the demuxer
("broadcast sinks").
BUG=webrtc:7135
==========
On 2017/06/26 07:32:14, stefan-webrtc wrote: > Please add a BUG=, even if it's a generic "Rtp transport refactoring" bug. Done.
The CQ bit was checked by eladalon@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from nisse@webrtc.org, danilchap@webrtc.org Link to the patchset: https://codereview.webrtc.org/2943693003/#ps160001 (title: ".")
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: ios32_sim_ios9_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios32_sim_ios9_dbg/buil...) ios_arm64_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_dbg/builds/21443) ios_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_dbg/builds/26761)
The CQ bit was checked by eladalon@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from nisse@webrtc.org, danilchap@webrtc.org, stefan@webrtc.org Link to the patchset: https://codereview.webrtc.org/2943693003/#ps180001 (title: ".")
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: ios32_sim_ios9_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios32_sim_ios9_dbg/buil...) ios64_sim_ios10_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios64_sim_ios10_dbg/bui...) ios_arm64_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_dbg/builds/21445) ios_arm64_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_rel/builds/21235)
The CQ bit was checked by eladalon@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from nisse@webrtc.org, danilchap@webrtc.org, stefan@webrtc.org Link to the patchset: https://codereview.webrtc.org/2943693003/#ps200001 (title: "dependencies")
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: ios32_sim_ios9_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios32_sim_ios9_dbg/buil...) ios_arm64_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_dbg/builds/21446) ios_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_dbg/builds/26764)
The CQ bit was checked by eladalon@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from nisse@webrtc.org, danilchap@webrtc.org, stefan@webrtc.org Link to the patchset: https://codereview.webrtc.org/2943693003/#ps220001 (title: ".")
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: android_arm64_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_arm64_rel/build...)
The CQ bit was checked by eladalon@webrtc.org
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: android_arm64_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_arm64_rel/build...)
Three times the same flaky test; I'll disable it.
The CQ bit was checked by eladalon@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from nisse@webrtc.org, danilchap@webrtc.org, stefan@webrtc.org Link to the patchset: https://codereview.webrtc.org/2943693003/#ps240001 (title: "Rebased")
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": 240001, "attempt_start_ts": 1498478770123730,
"parent_rev": "3ac91c7580982ae4f6fd792d6b20cedb3106bae9", "commit_rev":
"cb83bdf01f2ec8b9ed254991edc2be053c9eed24"}
Message was sent while issue was closed.
Description was changed from
==========
Create RtcpDemuxer. Capabilities:
1. Demux RTCP messages according to the sender-SSRC.
2. Demux RTCP messages according to the RSID (resolved to an SSRC, then compared
to the sender-RTCP).
3. Allow listening in on all RTCP messages passing through the demuxer
("broadcast sinks").
BUG=webrtc:7135
==========
to
==========
Create RtcpDemuxer. Capabilities:
1. Demux RTCP messages according to the sender-SSRC.
2. Demux RTCP messages according to the RSID (resolved to an SSRC, then compared
to the sender-RTCP).
3. Allow listening in on all RTCP messages passing through the demuxer
("broadcast sinks").
BUG=webrtc:7135
Review-Url: https://codereview.webrtc.org/2943693003
Cr-Commit-Position: refs/heads/master@{#18763}
Committed:
https://chromium.googlesource.com/external/webrtc/+/cb83bdf01f2ec8b9ed254991e...
==========
Message was sent while issue was closed.
Committed patchset #13 (id:240001) as https://chromium.googlesource.com/external/webrtc/+/cb83bdf01f2ec8b9ed254991e...
Message was sent while issue was closed.
A revert of this CL (patchset #13 id:240001) has been created in https://codereview.webrtc.org/2957763002/ by guidou@webrtc.org. The reason for reverting is: Breaks Chromium FYI bots. The problem is in the BUILD.gn file. Sample failure: https://build.chromium.org/p/chromium.webrtc.fyi/builders/Linux%20Builder/bui... Sample logs: use_goma = true """ to /b/c/b/Linux_Builder/src/out/Release/args.gn. /b/c/b/Linux_Builder/src/buildtools/linux64/gn gen //out/Release --check -> returned 1 ERROR at //third_party/webrtc/call/BUILD.gn:46:5: Can't load input file. "//webrtc/base:rtc_base_approved", ^--------------------------------.
Message was sent while issue was closed.
On 2017/06/26 13:28:28, guidou wrote: > A revert of this CL (patchset #13 id:240001) has been created in > https://codereview.webrtc.org/2957763002/ by mailto:guidou@webrtc.org. > > The reason for reverting is: Breaks Chromium FYI bots. > > The problem is in the BUILD.gn file. > > Sample failure: > https://build.chromium.org/p/chromium.webrtc.fyi/builders/Linux%20Builder/bui... > > Sample logs: > use_goma = true > """ to /b/c/b/Linux_Builder/src/out/Release/args.gn. > > /b/c/b/Linux_Builder/src/buildtools/linux64/gn gen //out/Release --check > -> returned 1 > ERROR at //third_party/webrtc/call/BUILD.gn:46:5: Can't load input file. > "//webrtc/base:rtc_base_approved", > ^--------------------------------. Yes. I now realize why relative paths are usually used; chromium probably sees webrtc under //third_party/webrtc/. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
