Create unit tests for RtpDemuxer

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

[elad.alon_webrtc.org, 2017-05-24 12:16:46]

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
==========

[elad.alon_webrtc.org, 2017-05-24 12:16:46]

elad.alon@webrtc.org changed reviewers:
+ danilchap@webrtc.org, nisse@webrtc.org, stefan@webrtc.org

[elad.alon_webrtc.org, 2017-05-24 12:19:35]

PTAL

https://codereview.webrtc.org/2904903002/diff/1/webrtc/call/rtp_demuxer_unitt...
File webrtc/call/rtp_demuxer_unittest.cc (right):

https://codereview.webrtc.org/2904903002/diff/1/webrtc/call/rtp_demuxer_unitt...
webrtc/call/rtp_demuxer_unittest.cc:20: // TODO(elad.alon): This is not
video-specific, and so should not be
Please advise. :-)

[elad.alon_webrtc.org, 2017-05-24 12:21:01]

P.S: Do we have a bug-ID for this? If not, would you like me to file a bug
detailing the intention of adding RTP-stream-ID-demuxing to this class?

[holmer, 2017-05-24 13:10:22]

holmer@google.com changed reviewers:
+ holmer@google.com

[holmer, 2017-05-24 13:10:23]

Would be good with bugs tracking the work of the different semi-large tasks that
we're working on in this project.

Nisse, Danil, could you file those, and maybe an overall
RtpTransportControllerReceive bug which can depend on them?

https://codereview.webrtc.org/2904903002/diff/1/webrtc/call/rtp_demuxer_unitt...
File webrtc/call/rtp_demuxer_unittest.cc (right):

https://codereview.webrtc.org/2904903002/diff/1/webrtc/call/rtp_demuxer_unitt...
webrtc/call/rtp_demuxer_unittest.cc:20: // TODO(elad.alon): This is not
video-specific, and so should not be
On 2017/05/24 12:19:35, eladalon wrote:
> Please advise. :-)

How is this currently a video engine test? Because it's included in a video test
executable? I think what we may want to do is to rename that executable or break
out common tests into a new exec. Definitely not part of this CL anyway. :)

[danilchap, 2017-05-24 13:45:43]

Why abbreviate UT? (wasn't sure what it mean until read the CL)

Tests are nice!

https://codereview.webrtc.org/2904903002/diff/1/webrtc/call/rtp_demuxer.cc
File webrtc/call/rtp_demuxer.cc (right):

https://codereview.webrtc.org/2904903002/diff/1/webrtc/call/rtp_demuxer.cc#ne...
webrtc/call/rtp_demuxer.cc:15: namespace {
prefer to put anonymous namespace inside namespace webrtc :
namespace webrtc {
namespace {
Helpers
}  // namespace

More code.

}  // namespace webrtc

https://codereview.webrtc.org/2904903002/diff/1/webrtc/call/rtp_demuxer.cc#ne...
webrtc/call/rtp_demuxer.cc:17: template <typename key_t, typename val_t>
may be typename Key, typename Value
"Template parameters should follow the naming style for their category: type
template parameters should follow the rules for type names"
https://google.github.io/styleguide/cppguide.html#General_Naming_Rules

https://codereview.webrtc.org/2904903002/diff/1/webrtc/call/rtp_demuxer.cc#ne...
webrtc/call/rtp_demuxer.cc:18: bool
MultimapAssociationExists(std::multimap<key_t, val_t> multimap,
const mutlimap<...>&

https://codereview.webrtc.org/2904903002/diff/1/webrtc/call/rtp_demuxer_unitt...
File webrtc/call/rtp_demuxer_unittest.cc (right):

https://codereview.webrtc.org/2904903002/diff/1/webrtc/call/rtp_demuxer_unitt...
webrtc/call/rtp_demuxer_unittest.cc:13: #include "webrtc/call/rtp_demuxer.h"
order includes alphabetically, except for the rtp_demuxer.h: include it before
any other header (i.e. before <memory>)

https://google.github.io/styleguide/cppguide.html#Names_and_Order_of_Includes

https://codereview.webrtc.org/2904903002/diff/1/webrtc/call/rtp_demuxer_unitt...
webrtc/call/rtp_demuxer_unittest.cc:20: // TODO(elad.alon): This is not
video-specific, and so should not be
On 2017/05/24 12:19:35, eladalon wrote:
> Please advise. :-)

you put it into call_tests set, which is reasonable.
The fact that it is in turn included into video_engine tests is a different
problem and not subject of this CL.
So I would advise to keep as is.
May move the todo into build.gn file, since it is about organizing dependency,
nothing to do in this file.

https://codereview.webrtc.org/2904903002/diff/1/webrtc/call/rtp_demuxer_unitt...
webrtc/call/rtp_demuxer_unittest.cc:27: constexpr uint32_t kSsrcs[] = {101, 202,
303};
may be prefer same order as recommended for classes, i.e. type then constant

https://google.github.io/styleguide/cppguide.html#Declaration_Order

https://codereview.webrtc.org/2904903002/diff/1/webrtc/call/rtp_demuxer_unitt...
webrtc/call/rtp_demuxer_unittest.cc:38: std::unique_ptr<RtpPacketReceived>
GetRtpPacketReceived(uint32_t ssrc) {
why return unique_ptr instead of copy of the packet?
RVO would likely make it as fast.

https://codereview.webrtc.org/2904903002/diff/1/webrtc/call/rtp_demuxer_unitt...
webrtc/call/rtp_demuxer_unittest.cc:51:
EXPECT_TRUE(packet->Parse(kMinimumPacket, sizeof(kMinimumPacket)));
you do not have to parse packet to create it:
packet->SetPayloadType(kPayloadType);
packet->SetSequenceNumber(kSeqNum);
would create the same packet as parsed (on the other side, since the only field
you use is ssrc, no need to set other fields)

https://codereview.webrtc.org/2904903002/diff/1/webrtc/call/rtp_demuxer_unitt...
webrtc/call/rtp_demuxer_unittest.cc:58: namespace test {
might be better to keep using unnamed namespace.

https://codereview.webrtc.org/2904903002/diff/1/webrtc/call/rtp_demuxer_unitt...
webrtc/call/rtp_demuxer_unittest.cc:65: void SetUp() {
prefer constructor over SetUp when you can

https://codereview.webrtc.org/2904903002/diff/1/webrtc/call/rtp_demuxer_unitt...
webrtc/call/rtp_demuxer_unittest.cc:77: static constexpr size_t kNumOfSinks = 3;
put constants before constructors.
Since you assume kNumOfSinks == arraysize(kSsrcs), make be put it near kSsrcs
array

https://codereview.webrtc.org/2904903002/diff/1/webrtc/call/rtp_demuxer_unitt...
webrtc/call/rtp_demuxer_unittest.cc:86: EXPECT_CALL(sinks[i],
OnRtpPacket(RtpPacketReceivedSsrcMatcher(*packet)));
OnRtpPacket(Property(&RtpPacketReceived::Ssrc, kSsrcs[i])) is not good enough?
using own match looks cleaner, but can you compare error messages when test
fail, which one will be more readable?

btw, why do you test that OnRtpPacket is called with correct packet at all? work
of the demuxer is to forward it to correct sink, so ::testing::_ might be good
enough.

https://codereview.webrtc.org/2904903002/diff/1/webrtc/call/rtp_demuxer_unitt...
webrtc/call/rtp_demuxer_unittest.cc:97: constexpr size_t kSharedSsrc = 404;
use uint32_t for ssrc

https://codereview.webrtc.org/2904903002/diff/1/webrtc/call/rtp_demuxer_unitt...
webrtc/call/rtp_demuxer_unittest.cc:138: sizeof(kSsrcsOfMultiSsrcSink) /
sizeof(kSsrcsOfMultiSsrcSink[0]));
there is arraysize macro in base/arraysize.h exactly for this

https://codereview.webrtc.org/2904903002/diff/1/webrtc/call/rtp_demuxer_unitt...
webrtc/call/rtp_demuxer_unittest.cc:162: #if RTC_DCHECK_IS_ON &&
GTEST_HAS_DEATH_TEST && !defined(WEBRTC_ANDROID)
why exclude android?

[elad.alon_webrtc.org, 2017-05-24 14:12:55]

We're changing my account name to eladalon@webrtc.org, to avoid confusion with
my @google account, which does not feature a dot (elad.alon), unlike this
account. I'm not sure what's going to happen to this CL; heads up.

[eladalon, 2017-05-24 14:41:46]

eladalon@webrtc.org changed reviewers:
+ eladalon@webrtc.org

[eladalon, 2017-05-24 14:41:47]

Please see https://codereview.webrtc.org/2902823004

[eladalon, 2017-05-25 07:59:31]

eladalon@webrtc.org changed reviewers:
- eladalon@webrtc.org