Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2902823004:
Create unit tests for RtpDemuxer (Closed)
DescriptionCreate unit tests for RtpDemuxer
1. Create unit tests for RtpDemuxer.
2. Add an RTC_DCHECK in RtpDemuxer that makes sure that the sink<->ssrc multimap does not allow multiple instances of the same association.
BUG=None
Review-Url: https://codereview.webrtc.org/2902823004
Cr-Commit-Position: refs/heads/master@{#18357}
Committed: https://chromium.googlesource.com/external/webrtc/+/760a076a5293b65d70c4f3cd171e148439b3608a
Patch Set 1 #Patch Set 2 : . #
Total comments: 29
Patch Set 3 : CR response #
Total comments: 6
Patch Set 4 : Response to CR #
Total comments: 2
Patch Set 5 : . #Patch Set 6 : MakeUnique #Patch Set 7 : Might as well remove the flaky death-test. #
Messages
Total messages: 38 (18 generated)
Description was changed from ========== Create UTs for RtpDemuxer 1. Create UTs for RtpDemuxer. 2. Add an RTC_DCHECK in RtpDemuxer that makes sure that the sink<->ssrc multimap does not allow multiple instances of the same association. BUG=None ========== to ========== Create UTs for RtpDemuxer 1. Create UTs for RtpDemuxer. 2. Add an RTC_DCHECK in RtpDemuxer that makes sure that the sink<->ssrc multimap does not allow multiple instances of the same association. BUG=None ==========
eladalon@webrtc.org changed reviewers: + danilchap@webrtc.org, nisse@webrtc.org, stefan@webrtc.org
Description was changed from ========== Create UTs for RtpDemuxer 1. Create UTs for RtpDemuxer. 2. Add an RTC_DCHECK in RtpDemuxer that makes sure that the sink<->ssrc multimap does not allow multiple instances of the same association. BUG=None ========== to ========== Create unit tests for RtpDemuxer 1. Create unit tests for RtpDemuxer. 2. Add an RTC_DCHECK in RtpDemuxer that makes sure that the sink<->ssrc multimap does not allow multiple instances of the same association. BUG=None ==========
https://codereview.webrtc.org/2902823004/diff/20001/webrtc/call/BUILD.gn File webrtc/call/BUILD.gn (right): https://codereview.webrtc.org/2902823004/diff/20001/webrtc/call/BUILD.gn#newc... webrtc/call/BUILD.gn:92: "rtp_demuxer_unittest.cc", # TODO(eladalon): Not really video-engine specific. same can be said about all other unittests in this source_set. so may be move this comment closer to the part that need changing. also, it is better to phrase TODOs in a form of what should be done. https://codereview.webrtc.org/2902823004/diff/20001/webrtc/call/rtp_demuxer.cc File webrtc/call/rtp_demuxer.cc (right): https://codereview.webrtc.org/2902823004/diff/20001/webrtc/call/rtp_demuxer.c... webrtc/call/rtp_demuxer.cc:24: using ref_t = typename std::multimap<Key, Value>::const_reference; name ref_t as a type too https://codereview.webrtc.org/2902823004/diff/20001/webrtc/call/rtp_demuxer_u... File webrtc/call/rtp_demuxer_unittest.cc (right): https://codereview.webrtc.org/2902823004/diff/20001/webrtc/call/rtp_demuxer_u... webrtc/call/rtp_demuxer_unittest.cc:15: #include "webrtc/modules/rtp_rtcp/source/rtp_packet_received.h" put it inbetween webrtc/base and webrtc/test (alphabetical order) https://codereview.webrtc.org/2902823004/diff/20001/webrtc/call/rtp_demuxer_u... webrtc/call/rtp_demuxer_unittest.cc:37: std::unique_ptr<RtpPacketReceived> GetRtpPacketReceived(uint32_t ssrc) { may be call it Create... https://codereview.webrtc.org/2902823004/diff/20001/webrtc/call/rtp_demuxer_u... webrtc/call/rtp_demuxer_unittest.cc:38: std::unique_ptr<RtpPacketReceived> packet(new RtpPacketReceived()); may be to avoid mentioning RtpPacketReceived twice: auto packet = rtc::MakeUnique<RtpPacketReceived>(); (using relatively new base/ptr_util.h) https://codereview.webrtc.org/2902823004/diff/20001/webrtc/call/rtp_demuxer_u... webrtc/call/rtp_demuxer_unittest.cc:51: virtual ~RtpDemuxerTest() { override instead of virtual https://codereview.webrtc.org/2902823004/diff/20001/webrtc/call/rtp_demuxer_u... webrtc/call/rtp_demuxer_unittest.cc:58: static constexpr size_t kNumOfSinks = 3; prefer constants above constructor. https://google.github.io/styleguide/cppguide.html#Declaration_Order if you want to keep it here, add a comment why break common order. https://codereview.webrtc.org/2902823004/diff/20001/webrtc/call/rtp_demuxer_u... webrtc/call/rtp_demuxer_unittest.cc:65: EXPECT_CALL(sinks[i], OnRtpPacket(RtpPacketReceivedSsrcMatcher(*packet))); which line you think would be faster to understand?: EXPECT_CALL(sinks[i], OnRtpPacket(RtpPacketReceivedSsrcMatcher(*packet))); EXPECT_CALL(sinks[i], OnRtpPacket(SameSsrc(*packet))); EXPECT_CALL(sinks[i], OnRtpPacket(SsrcSameAsIn(*packet))); https://codereview.webrtc.org/2902823004/diff/20001/webrtc/call/rtp_demuxer_u... webrtc/call/rtp_demuxer_unittest.cc:120: TEST_F(RtpDemuxerTest, SinkRemovalSanity) { prefer to name test describing what it is tesing, i.e. both scenario and what would happen, e.g. AfterSinkRemovedDoesNotForwardPacketsToIt https://testing.googleblog.com/2014/10/testing-on-toilet-writing-descriptive.... https://codereview.webrtc.org/2902823004/diff/20001/webrtc/call/rtp_demuxer_u... webrtc/call/rtp_demuxer_unittest.cc:134: // with which it was previously associated. check it with StrictMock<MockRtpPacketSink> or with EXPECT_CALL(multi_ssrc_sink, OnRtpPacket(_)).Times(0);
https://codereview.webrtc.org/2902823004/diff/20001/webrtc/call/BUILD.gn File webrtc/call/BUILD.gn (right): https://codereview.webrtc.org/2902823004/diff/20001/webrtc/call/BUILD.gn#newc... webrtc/call/BUILD.gn:92: "rtp_demuxer_unittest.cc", # TODO(eladalon): Not really video-engine specific. On 2017/05/24 17:14:02, danilchap wrote: > same can be said about all other unittests in this source_set. > so may be move this comment closer to the part that need changing. > > also, it is better to phrase TODOs in a form of what should be done. Moved. Would we normally also file a bug for this, or is a TODO sufficient? https://codereview.webrtc.org/2902823004/diff/20001/webrtc/call/rtp_demuxer.cc File webrtc/call/rtp_demuxer.cc (right): https://codereview.webrtc.org/2902823004/diff/20001/webrtc/call/rtp_demuxer.c... webrtc/call/rtp_demuxer.cc:24: using ref_t = typename std::multimap<Key, Value>::const_reference; On 2017/05/24 17:14:02, danilchap wrote: > name ref_t as a type too Done. https://codereview.webrtc.org/2902823004/diff/20001/webrtc/call/rtp_demuxer_u... File webrtc/call/rtp_demuxer_unittest.cc (right): https://codereview.webrtc.org/2902823004/diff/20001/webrtc/call/rtp_demuxer_u... webrtc/call/rtp_demuxer_unittest.cc:15: #include "webrtc/modules/rtp_rtcp/source/rtp_packet_received.h" On 2017/05/24 17:14:02, danilchap wrote: > put it inbetween webrtc/base and webrtc/test (alphabetical order) I will explain my reasoning, and if you still prefer this to be moved, I will move, and take note of the preference in the future. My reasoning was that this section had the test-specific headers - what we test (RtpPacket, etc.) - whereas the next section had the headers for test infrastructure - implementation details of *how* we test. Wdyt? (I realize my approach collides with putting rtp_demuxer.h first. I'd rather move it back here, then.) https://codereview.webrtc.org/2902823004/diff/20001/webrtc/call/rtp_demuxer_u... webrtc/call/rtp_demuxer_unittest.cc:37: std::unique_ptr<RtpPacketReceived> GetRtpPacketReceived(uint32_t ssrc) { On 2017/05/24 17:14:02, danilchap wrote: > may be call it Create... Done. https://codereview.webrtc.org/2902823004/diff/20001/webrtc/call/rtp_demuxer_u... webrtc/call/rtp_demuxer_unittest.cc:38: std::unique_ptr<RtpPacketReceived> packet(new RtpPacketReceived()); On 2017/05/24 17:14:02, danilchap wrote: > may be to avoid mentioning RtpPacketReceived twice: > auto packet = rtc::MakeUnique<RtpPacketReceived>(); > (using relatively new base/ptr_util.h) Thank you for bringing this utility function to my attention. I wonder if it might not be better to use it in cases that require more verbose code (pointer to an iterator over a map of vectors of iterators...), and in this case keep it simple? The benefit of this method is that we only construct the unique_ptr once; with MakeUnique, we construct two, albeit the second one with a simple move. Wdyt? https://codereview.webrtc.org/2902823004/diff/20001/webrtc/call/rtp_demuxer_u... webrtc/call/rtp_demuxer_unittest.cc:51: virtual ~RtpDemuxerTest() { On 2017/05/24 17:14:02, danilchap wrote: > override instead of virtual Done. https://codereview.webrtc.org/2902823004/diff/20001/webrtc/call/rtp_demuxer_u... webrtc/call/rtp_demuxer_unittest.cc:58: static constexpr size_t kNumOfSinks = 3; On 2017/05/24 17:14:02, danilchap wrote: > prefer constants above constructor. > https://google.github.io/styleguide/cppguide.html#Declaration_Order > > if you want to keep it here, add a comment why break common order. I thought you were okay with putting it here? https://codereview.webrtc.org/2904903002/diff/1/webrtc/call/rtp_demuxer_unitt... IMHO, the usage makes it clear enough why this is here. I'd prefer avoiding a comment about order, to avoid distracting from comments about functionality. Wdyt? https://codereview.webrtc.org/2902823004/diff/20001/webrtc/call/rtp_demuxer_u... webrtc/call/rtp_demuxer_unittest.cc:65: EXPECT_CALL(sinks[i], OnRtpPacket(RtpPacketReceivedSsrcMatcher(*packet))); On 2017/05/24 17:14:02, danilchap wrote: > which line you think would be faster to understand?: > EXPECT_CALL(sinks[i], OnRtpPacket(RtpPacketReceivedSsrcMatcher(*packet))); > EXPECT_CALL(sinks[i], OnRtpPacket(SameSsrc(*packet))); > EXPECT_CALL(sinks[i], OnRtpPacket(SsrcSameAsIn(*packet))); I see what you mean. Sure, let's go with SsrcSameAsIn. https://codereview.webrtc.org/2902823004/diff/20001/webrtc/call/rtp_demuxer_u... webrtc/call/rtp_demuxer_unittest.cc:120: TEST_F(RtpDemuxerTest, SinkRemovalSanity) { On 2017/05/24 17:14:02, danilchap wrote: > prefer to name test describing what it is tesing, i.e. both scenario and what > would happen, e.g. AfterSinkRemovedDoesNotForwardPacketsToIt > > https://testing.googleblog.com/2014/10/testing-on-toilet-writing-descriptive.... Good point. I've gone with "OnRtpPacketNotCalledOnRemovedSinks". https://codereview.webrtc.org/2902823004/diff/20001/webrtc/call/rtp_demuxer_u... webrtc/call/rtp_demuxer_unittest.cc:134: // with which it was previously associated. On 2017/05/24 17:14:03, danilchap wrote: > check it with StrictMock<MockRtpPacketSink> > or with > EXPECT_CALL(multi_ssrc_sink, OnRtpPacket(_)).Times(0); I'm not sure why this would be needed. If you move the removal of the sink to after this block, you'll see that the test fails already. As for Times(0), although not strictly necessary, I guess it could be seen as code self-documentation. (I wonder if it would fail any of the mutations I've seen on testing-on-the-toilet recently, though, because removing the line would not make any changes to the test's results.) In the trade-off between code brevity and code self-documentation, what do you prefer here?
https://codereview.webrtc.org/2902823004/diff/20001/webrtc/call/BUILD.gn File webrtc/call/BUILD.gn (right): https://codereview.webrtc.org/2902823004/diff/20001/webrtc/call/BUILD.gn#newc... webrtc/call/BUILD.gn:92: "rtp_demuxer_unittest.cc", # TODO(eladalon): Not really video-engine specific. On 2017/05/25 08:00:05, eladalon wrote: > On 2017/05/24 17:14:02, danilchap wrote: > > same can be said about all other unittests in this source_set. > > so may be move this comment closer to the part that need changing. > > > > also, it is better to phrase TODOs in a form of what should be done. > > Moved. Would we normally also file a bug for this, or is a TODO sufficient? Probably are no hard rules. Speaking generally, in one way, TODO is a light-weight tool to use when for some reason one can't be bothered to file a bug. And it can be used to link to a relevant bug, if there is one. To file a bug, I think one would need to have a wider scope than just moving a file or two around, and figure out what's the desired organization of these build targets. Looking at this case, to me, it seems appropriate that the call_tests target includes unit tests for whatever code happens to be located in the call/ directory. Now the rtp demuxer should be moved out of this directory at some point, together with the rest of the rtp transport controller code which is under development, but that's not really a problem with this build target or this BUILD.gn file. https://codereview.webrtc.org/2902823004/diff/40001/webrtc/call/rtp_demuxer_u... File webrtc/call/rtp_demuxer_unittest.cc (right): https://codereview.webrtc.org/2902823004/diff/40001/webrtc/call/rtp_demuxer_u... webrtc/call/rtp_demuxer_unittest.cc:58: static constexpr size_t kNumOfSinks = 3; I'm no real c++ guru, but does static really make sense together with constexpr, and if so, what does it mean? https://codereview.webrtc.org/2902823004/diff/40001/webrtc/call/rtp_demuxer_u... webrtc/call/rtp_demuxer_unittest.cc:65: EXPECT_CALL(sinks[i], OnRtpPacket(SsrcSameAsIn(*packet))); You could compare the address of the packet, which would arguably be stricter than comparing ssrc. I think it would work with just testing::Ref(*packet). https://codereview.webrtc.org/2902823004/diff/40001/webrtc/call/rtp_demuxer_u... webrtc/call/rtp_demuxer_unittest.cc:67: testing::Mock::VerifyAndClearExpectations(&sinks[i]); Why do you need to call this explicitly? If it's really beneficial, I think it deserves a comment explaining why.
https://codereview.webrtc.org/2902823004/diff/20001/webrtc/call/rtp_demuxer_u... File webrtc/call/rtp_demuxer_unittest.cc (right): https://codereview.webrtc.org/2902823004/diff/20001/webrtc/call/rtp_demuxer_u... webrtc/call/rtp_demuxer_unittest.cc:15: #include "webrtc/modules/rtp_rtcp/source/rtp_packet_received.h" On 2017/05/25 08:00:05, eladalon wrote: > On 2017/05/24 17:14:02, danilchap wrote: > > put it inbetween webrtc/base and webrtc/test (alphabetical order) > > I will explain my reasoning, and if you still prefer this to be moved, I will > move, and take note of the preference in the future. > > My reasoning was that this section had the test-specific headers - what we test > (RtpPacket, etc.) - whereas the next section had the headers for test > infrastructure - implementation details of *how* we test. > Wdyt? > > (I realize my approach collides with putting rtp_demuxer.h first. I'd rather > move it back here, then.) I'm not sure why introduce own convention when there is already one. 1st refactoring will reorder headers alphabetically anyway. It might be even done without looking, using a script. https://codereview.webrtc.org/2902823004/diff/20001/webrtc/call/rtp_demuxer_u... webrtc/call/rtp_demuxer_unittest.cc:38: std::unique_ptr<RtpPacketReceived> packet(new RtpPacketReceived()); On 2017/05/25 08:00:05, eladalon wrote: > On 2017/05/24 17:14:02, danilchap wrote: > > may be to avoid mentioning RtpPacketReceived twice: > > auto packet = rtc::MakeUnique<RtpPacketReceived>(); > > (using relatively new base/ptr_util.h) > > Thank you for bringing this utility function to my attention. I wonder if it > might not be better to use it in cases that require more verbose code (pointer > to an iterator over a map of vectors of iterators...), and in this case keep it > simple? The benefit of this method is that we only construct the unique_ptr > once; with MakeUnique, we construct two, albeit the second one with a simple > move. Wdyt? The way I see it, MakeUnique tells 'nothing odd going here, just creating a smart pointer using public and explicit constructor, no need to worry'. 'new' is more flexible than MakeUnique and so require extra attention: need to check if it is properly deallocated; sometimes it mean it is not public constructor that is used. Personally - I always use MakeUnique when I can, try to use WrapUnique when I can't and use new only when neither of those two work. (Move of the unique_ptr is so cheap, I always assume it is free) In this case I'm fine with current code: it is simple enough any way. https://codereview.webrtc.org/2902823004/diff/20001/webrtc/call/rtp_demuxer_u... webrtc/call/rtp_demuxer_unittest.cc:58: static constexpr size_t kNumOfSinks = 3; On 2017/05/25 08:00:05, eladalon wrote: > On 2017/05/24 17:14:02, danilchap wrote: > > prefer constants above constructor. > > https://google.github.io/styleguide/cppguide.html#Declaration_Order > > > > if you want to keep it here, add a comment why break common order. > > I thought you were okay with putting it here? > > https://codereview.webrtc.org/2904903002/diff/1/webrtc/call/rtp_demuxer_unitt... > IMHO, the usage makes it clear enough why this is here. I'd prefer avoiding a > comment about order, to avoid distracting from comments about functionality. > Wdyt? One of the worries: in your tests you rely on fact kNumOfSinks = arraysize(kSsrc), but those two constants are far away. if you would use arraysize(kSsrc) instead of 3, the link would be explicit and wouldn't require any other comment. Another problem - is mixing together constants and regular members. My only concern is that it goes against style and thus alarms and ask for extra attention. https://codereview.webrtc.org/2902823004/diff/20001/webrtc/call/rtp_demuxer_u... webrtc/call/rtp_demuxer_unittest.cc:134: // with which it was previously associated. On 2017/05/25 08:00:05, eladalon wrote: > On 2017/05/24 17:14:03, danilchap wrote: > > check it with StrictMock<MockRtpPacketSink> > > or with > > EXPECT_CALL(multi_ssrc_sink, OnRtpPacket(_)).Times(0); > > I'm not sure why this would be needed. If you move the removal of the sink to > after this block, you'll see that the test fails already. I tried to move RemoveSink after the loop - test still pass. (Sure, there are few extra message about unexpected calls, but by default they are just warning, not errors.) > In the trade-off between code > brevity and code self-documentation, what do you prefer here? Personally - code self-documentation. Preliminary brevity imho is as bad as preliminary optimization: it hurts readability [and thus reliability] and later refactoring more than it helps. [often you need to refactor in a different direction than initial brevity] Also, personally, I prefer comments written in c++ than comments written in English: those are often checked by compiler and tools and thus more up to date. https://codereview.webrtc.org/2902823004/diff/40001/webrtc/call/rtp_demuxer_u... File webrtc/call/rtp_demuxer_unittest.cc (right): https://codereview.webrtc.org/2902823004/diff/40001/webrtc/call/rtp_demuxer_u... webrtc/call/rtp_demuxer_unittest.cc:58: static constexpr size_t kNumOfSinks = 3; On 2017/05/29 07:58:22, nisse-webrtc wrote: > I'm no real c++ guru, but does static really make sense together with constexpr, > and if so, what does it mean? afaik it is a compile error to use constexpr without static for member variable. static still mean same: the variable is bound to class, not to object.
https://codereview.webrtc.org/2902823004/diff/20001/webrtc/call/rtp_demuxer_u... File webrtc/call/rtp_demuxer_unittest.cc (right): https://codereview.webrtc.org/2902823004/diff/20001/webrtc/call/rtp_demuxer_u... webrtc/call/rtp_demuxer_unittest.cc:15: #include "webrtc/modules/rtp_rtcp/source/rtp_packet_received.h" On 2017/05/29 08:59:53, danilchap wrote: > On 2017/05/25 08:00:05, eladalon wrote: > > On 2017/05/24 17:14:02, danilchap wrote: > > > put it inbetween webrtc/base and webrtc/test (alphabetical order) > > > > I will explain my reasoning, and if you still prefer this to be moved, I will > > move, and take note of the preference in the future. > > > > My reasoning was that this section had the test-specific headers - what we > test > > (RtpPacket, etc.) - whereas the next section had the headers for test > > infrastructure - implementation details of *how* we test. > > Wdyt? > > > > (I realize my approach collides with putting rtp_demuxer.h first. I'd rather > > move it back here, then.) > > I'm not sure why introduce own convention when there is already one. > 1st refactoring will reorder headers alphabetically anyway. It might be even > done without looking, using a script. Done. https://codereview.webrtc.org/2902823004/diff/20001/webrtc/call/rtp_demuxer_u... webrtc/call/rtp_demuxer_unittest.cc:38: std::unique_ptr<RtpPacketReceived> packet(new RtpPacketReceived()); On 2017/05/29 08:59:53, danilchap wrote: > On 2017/05/25 08:00:05, eladalon wrote: > > On 2017/05/24 17:14:02, danilchap wrote: > > > may be to avoid mentioning RtpPacketReceived twice: > > > auto packet = rtc::MakeUnique<RtpPacketReceived>(); > > > (using relatively new base/ptr_util.h) > > > > Thank you for bringing this utility function to my attention. I wonder if it > > might not be better to use it in cases that require more verbose code (pointer > > to an iterator over a map of vectors of iterators...), and in this case keep > it > > simple? The benefit of this method is that we only construct the unique_ptr > > once; with MakeUnique, we construct two, albeit the second one with a simple > > move. Wdyt? > > The way I see it, MakeUnique tells 'nothing odd going here, just creating a > smart pointer using public and explicit constructor, no need to worry'. > 'new' is more flexible than MakeUnique and so require extra attention: need to > check if it is properly deallocated; sometimes it mean it is not public > constructor that is used. > Personally - I always use MakeUnique when I can, try to use WrapUnique when I > can't and use new only when neither of those two work. > > (Move of the unique_ptr is so cheap, I always assume it is free) > > In this case I'm fine with current code: it is simple enough any way. Thanks for explaining your rationale for preferring MakeUnique. I've searched the codebase and only found 103 references to it, so it's probably still being adopted. For now, I'll only use it for complicated types, where the gain in readability is enough to offset reader unfamiliarity and the slight associated cost. If it seems to be gaining popularity as the standard way of doing things, I'll join the adopters. https://codereview.webrtc.org/2902823004/diff/20001/webrtc/call/rtp_demuxer_u... webrtc/call/rtp_demuxer_unittest.cc:134: // with which it was previously associated. On 2017/05/29 08:59:53, danilchap wrote: > On 2017/05/25 08:00:05, eladalon wrote: > > On 2017/05/24 17:14:03, danilchap wrote: > > > check it with StrictMock<MockRtpPacketSink> > > > or with > > > EXPECT_CALL(multi_ssrc_sink, OnRtpPacket(_)).Times(0); > > > > I'm not sure why this would be needed. If you move the removal of the sink to > > after this block, you'll see that the test fails already. > > I tried to move RemoveSink after the loop - test still pass. > (Sure, there are few extra message about unexpected calls, but by default they > are just warning, not errors.) > > > In the trade-off between code > > brevity and code self-documentation, what do you prefer here? > Personally - code self-documentation. > Preliminary brevity imho is as bad as preliminary optimization: it hurts > readability [and thus reliability] and later refactoring more than it helps. > [often you need to refactor in a different direction than initial brevity] > > Also, personally, I prefer comments written in c++ than comments written in > English: those are often checked by compiler and tools and thus more up to date. > > Sorry, I mistook a warning for an error. Thank you for your alertness! https://codereview.webrtc.org/2902823004/diff/40001/webrtc/call/rtp_demuxer_u... File webrtc/call/rtp_demuxer_unittest.cc (right): https://codereview.webrtc.org/2902823004/diff/40001/webrtc/call/rtp_demuxer_u... webrtc/call/rtp_demuxer_unittest.cc:65: EXPECT_CALL(sinks[i], OnRtpPacket(SsrcSameAsIn(*packet))); On 2017/05/29 07:58:22, nisse-webrtc wrote: > You could compare the address of the packet, which would arguably be stricter > than comparing ssrc. I think it would work with just testing::Ref(*packet). Thanks for the tip, but I think it would introduce an assumption that RtpDemuxer does not replicate the data, as it might wish to do if it were, for example, multi-threaded, in which case it might wish to buffer the replicated object for the other thread. https://codereview.webrtc.org/2902823004/diff/40001/webrtc/call/rtp_demuxer_u... webrtc/call/rtp_demuxer_unittest.cc:67: testing::Mock::VerifyAndClearExpectations(&sinks[i]); On 2017/05/29 07:58:22, nisse-webrtc wrote: > Why do you need to call this explicitly? If it's really beneficial, I think it > deserves a comment explaining why. I'll remove, then.
lgtm https://codereview.webrtc.org/2902823004/diff/60001/webrtc/call/rtp_demuxer_u... File webrtc/call/rtp_demuxer_unittest.cc (right): https://codereview.webrtc.org/2902823004/diff/60001/webrtc/call/rtp_demuxer_u... webrtc/call/rtp_demuxer_unittest.cc:25: using testing::_; nit: prefer full qualified named when writing using: using ::testing::_; (this way you can be sure it will not collide with webrtc::testing)
https://codereview.webrtc.org/2902823004/diff/60001/webrtc/call/rtp_demuxer_u... File webrtc/call/rtp_demuxer_unittest.cc (right): https://codereview.webrtc.org/2902823004/diff/60001/webrtc/call/rtp_demuxer_u... webrtc/call/rtp_demuxer_unittest.cc:25: using testing::_; On 2017/05/30 14:32:35, danilchap wrote: > nit: prefer full qualified named when writing using: > using ::testing::_; > (this way you can be sure it will not collide with webrtc::testing) Done.
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/2902823004/#ps80001 (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: presubmit on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/17567)
lgtm https://codereview.webrtc.org/2902823004/diff/20001/webrtc/call/rtp_demuxer_u... File webrtc/call/rtp_demuxer_unittest.cc (right): https://codereview.webrtc.org/2902823004/diff/20001/webrtc/call/rtp_demuxer_u... webrtc/call/rtp_demuxer_unittest.cc:38: std::unique_ptr<RtpPacketReceived> packet(new RtpPacketReceived()); On 2017/05/30 14:23:37, eladalon wrote: > On 2017/05/29 08:59:53, danilchap wrote: > > On 2017/05/25 08:00:05, eladalon wrote: > > > On 2017/05/24 17:14:02, danilchap wrote: > > > > may be to avoid mentioning RtpPacketReceived twice: > > > > auto packet = rtc::MakeUnique<RtpPacketReceived>(); > > > > (using relatively new base/ptr_util.h) > > > > > > Thank you for bringing this utility function to my attention. I wonder if it > > > might not be better to use it in cases that require more verbose code > (pointer > > > to an iterator over a map of vectors of iterators...), and in this case keep > > it > > > simple? The benefit of this method is that we only construct the unique_ptr > > > once; with MakeUnique, we construct two, albeit the second one with a simple > > > move. Wdyt? > > > > The way I see it, MakeUnique tells 'nothing odd going here, just creating a > > smart pointer using public and explicit constructor, no need to worry'. > > 'new' is more flexible than MakeUnique and so require extra attention: need to > > check if it is properly deallocated; sometimes it mean it is not public > > constructor that is used. > > Personally - I always use MakeUnique when I can, try to use WrapUnique when I > > can't and use new only when neither of those two work. > > > > (Move of the unique_ptr is so cheap, I always assume it is free) > > > > In this case I'm fine with current code: it is simple enough any way. > > Thanks for explaining your rationale for preferring MakeUnique. I've searched > the codebase and only found 103 references to it, so it's probably still being > adopted. For now, I'll only use it for complicated types, where the gain in > readability is enough to offset reader unfamiliarity and the slight associated > cost. If it seems to be gaining popularity as the standard way of doing things, > I'll join the adopters. MakeUnique was added recently, and I think the intention is to use it everywhere possible.
holmer@google.com changed reviewers: + holmer@google.com
lgtm
The CQ bit was checked by eladalon@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from holmer@google.com, nisse@webrtc.org, danilchap@webrtc.org Link to the patchset: https://codereview.webrtc.org/2902823004/#ps100001 (title: "MakeUnique")
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: presubmit on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/17578)
lgtm
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: linux_memcheck on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_memcheck/builds/8368)
The CQ bit was checked by eladalon@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from holmer@google.com, nisse@webrtc.org, danilchap@webrtc.org, stefan@webrtc.org Link to the patchset: https://codereview.webrtc.org/2902823004/#ps120001 (title: "Might as well remove the flaky death-test.")
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: linux_memcheck on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_memcheck/builds/8393)
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/...
CQ is committing da patch.
Bot data: {"patchset_id": 120001, "attempt_start_ts": 1496246379438670,
"parent_rev": "6d3e866ab1ab67c0f9e228a4975ed262c3327bca", "commit_rev":
"760a076a5293b65d70c4f3cd171e148439b3608a"}
Message was sent while issue was closed.
Description was changed from ========== Create unit tests for RtpDemuxer 1. Create unit tests for RtpDemuxer. 2. Add an RTC_DCHECK in RtpDemuxer that makes sure that the sink<->ssrc multimap does not allow multiple instances of the same association. BUG=None ========== to ========== Create unit tests for RtpDemuxer 1. Create unit tests for RtpDemuxer. 2. Add an RTC_DCHECK in RtpDemuxer that makes sure that the sink<->ssrc multimap does not allow multiple instances of the same association. BUG=None Review-Url: https://codereview.webrtc.org/2902823004 Cr-Commit-Position: refs/heads/master@{#18357} Committed: https://chromium.googlesource.com/external/webrtc/+/760a076a5293b65d70c4f3cd1... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001) as https://chromium.googlesource.com/external/webrtc/+/760a076a5293b65d70c4f3cd1... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
