Switch voice transport to use Call and Stream instead of VoENetwork.

Switch voice transport to use Call and Stream instead of VoENetwork.

VoENetwork is kept for now, but is not really used anylonger.

webrtcvoiceengine is changed to have the same behavior for unsignaled
ssrc as video has, which is reflected by disabling one test case and
this will be discussed and followed up.

BUG=webrtc:5079
TBR=tommi

Committed: https://crrev.com/3d7db263b902f319b5b0c828ccff59cd8664b937
Cr-Commit-Position: refs/heads/master@{#12555}

[mflodman, 2016-04-22 09:02:18]

mflodman@webrtc.org changed reviewers:
+ solenberg@webrtc.org

[mflodman, 2016-04-22 09:02:19]

Fredrik,
Please take a look.

https://codereview.webrtc.org/1909333002/diff/1/webrtc/audio/audio_send_strea...
File webrtc/audio/audio_send_stream_unittest.cc (right):

https://codereview.webrtc.org/1909333002/diff/1/webrtc/audio/audio_send_strea...
webrtc/audio/audio_send_stream_unittest.cc:159:
testing::NiceMock<MockVoEChannelProxy>* channel_proxy_ = nullptr;
To avoid errors for unnecessary calls to DeRegisterExternalTansport.

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

https://codereview.webrtc.org/1909333002/diff/1/webrtc/call/call.cc#newcode753
webrtc/call/call.cc:753: // TODO(pbos): Make sure it's a valid packet.
Changing comments, we might get receive blocks for multiple receivers and do
want to broadcast.

[stefan-webrtc, 2016-04-22 09:16:19]

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

[stefan-webrtc, 2016-04-22 09:16:19]

drive-by review of call_perf_tests

https://codereview.webrtc.org/1909333002/diff/1/webrtc/call/call_perf_tests.cc
File webrtc/call/call_perf_tests.cc (right):

https://codereview.webrtc.org/1909333002/diff/1/webrtc/call/call_perf_tests.c...
webrtc/call/call_perf_tests.cc:157: VoENetwork* voe_network =
VoENetwork::GetInterface(voice_engine);
No longer used.

If you feel like it, it should be straightforward to build this test upon
EndToEndTest now, which uses CallTest which sets up legacy voice engine objects,
calls, send/receive streams and fake devices for you.

https://codereview.webrtc.org/1909333002/diff/1/webrtc/call/call_perf_tests.c...
webrtc/call/call_perf_tests.cc:226: AudioSendStream::Config
audio_send_config(&audio_send_transport);
I'm not sure this actually works in practice. All packets go to the same pacer
which will send them out on the same transport. Have you verified that the
streams aren't in sync already at the beginning?

[mflodman, 2016-04-22 09:41:48]

https://codereview.webrtc.org/1909333002/diff/1/webrtc/call/call_perf_tests.cc
File webrtc/call/call_perf_tests.cc (right):

https://codereview.webrtc.org/1909333002/diff/1/webrtc/call/call_perf_tests.c...
webrtc/call/call_perf_tests.cc:157: VoENetwork* voe_network =
VoENetwork::GetInterface(voice_engine);
On 2016/04/22 09:16:19, stefan-webrtc (holmer) wrote:
> No longer used.
> 
> If you feel like it, it should be straightforward to build this test upon
> EndToEndTest now, which uses CallTest which sets up legacy voice engine
objects,
> calls, send/receive streams and fake devices for you.

Removed.

I'd prefer to followup with the refactoring to avoid more changes, if you're ok
with that?

https://codereview.webrtc.org/1909333002/diff/1/webrtc/call/call_perf_tests.c...
webrtc/call/call_perf_tests.cc:226: AudioSendStream::Config
audio_send_config(&audio_send_transport);
On 2016/04/22 09:16:19, stefan-webrtc (holmer) wrote:
> I'm not sure this actually works in practice. All packets go to the same pacer
> which will send them out on the same transport. Have you verified that the
> streams aren't in sync already at the beginning?

If that is the case, our API is wrong if we can set one transport per send
stream.

The test starts by printing:
"RESULT stream_offset: synchronization= -553 ms"

to slowly go down to:
"RESULT stream_offset: synchronization= -14 ms"

I'll look at the code though.

[stefan-webrtc, 2016-04-22 10:35:05]

lgtm for call_perf_tests

https://codereview.webrtc.org/1909333002/diff/1/webrtc/call/call_perf_tests.cc
File webrtc/call/call_perf_tests.cc (right):

https://codereview.webrtc.org/1909333002/diff/1/webrtc/call/call_perf_tests.c...
webrtc/call/call_perf_tests.cc:157: VoENetwork* voe_network =
VoENetwork::GetInterface(voice_engine);
On 2016/04/22 09:41:48, mflodman wrote:
> On 2016/04/22 09:16:19, stefan-webrtc (holmer) wrote:
> > No longer used.
> > 
> > If you feel like it, it should be straightforward to build this test upon
> > EndToEndTest now, which uses CallTest which sets up legacy voice engine
> objects,
> > calls, send/receive streams and fake devices for you.
> 
> Removed.
> 
> I'd prefer to followup with the refactoring to avoid more changes, if you're
ok
> with that?

Sure, no problem.

https://codereview.webrtc.org/1909333002/diff/1/webrtc/call/call_perf_tests.c...
webrtc/call/call_perf_tests.cc:226: AudioSendStream::Config
audio_send_config(&audio_send_transport);
On 2016/04/22 09:41:48, mflodman wrote:
> On 2016/04/22 09:16:19, stefan-webrtc (holmer) wrote:
> > I'm not sure this actually works in practice. All packets go to the same
pacer
> > which will send them out on the same transport. Have you verified that the
> > streams aren't in sync already at the beginning?
> 
> If that is the case, our API is wrong if we can set one transport per send
> stream.
> 
> The test starts by printing:
> "RESULT stream_offset: synchronization= -553 ms"
> 
> to slowly go down to:
> "RESULT stream_offset: synchronization= -14 ms"
> 
> I'll look at the code though.

You're right, it does go over separate transports, my mistake.

[the sun, 2016-04-22 12:40:32]

Very nice!

All small stuff, except that we need to continue support changing default SSRC
on the fly. See comment in webrtcvoicengine_unittest.cc,

https://codereview.webrtc.org/1909333002/diff/20001/webrtc/audio/audio_receiv...
File webrtc/audio/audio_receive_stream.cc (right):

https://codereview.webrtc.org/1909333002/diff/20001/webrtc/audio/audio_receiv...
webrtc/audio/audio_receive_stream.cc:153: return
channel_proxy_->ReceivedRTCPPacket(static_cast<const uint8_t*>(packet),
Looks like this cast is not needed anymore?

https://codereview.webrtc.org/1909333002/diff/20001/webrtc/audio/audio_receiv...
webrtc/audio/audio_receive_stream.cc:178:
remote_bitrate_estimator_->IncomingPacket(arrival_time_ms, payload_size,
stefan: is it still appropriate to fork packets off for BWE here or should that
happen some other way (e.g. through the Channel's RtpRtcpModule)?

https://codereview.webrtc.org/1909333002/diff/20001/webrtc/audio/audio_receiv...
webrtc/audio/audio_receive_stream.cc:182: return
channel_proxy_->ReceivedRTPPacket(packet, length, packet_time);
VoE::ReceivedRTPPacket() will:
  if ((length < 12) || (length > 1292)) {
    return -1;
  }
but it seems to me that we shouldn't make such a decision at that level, and I
can't find anything in the code that looks like it will break if the packet is <
12 bytes or > 1292 bytes. However, we should be aware that were potentially
opening up for abuse.

https://codereview.webrtc.org/1909333002/diff/20001/webrtc/audio/audio_receiv...
File webrtc/audio/audio_receive_stream_unittest.cc (right):

https://codereview.webrtc.org/1909333002/diff/20001/webrtc/audio/audio_receiv...
webrtc/audio/audio_receive_stream_unittest.cc:239: TEST(AudioReceiveStreamTest,
AudioPacketUpdatesBweFeedback) {
Rename test case to "ReceiveRtpPacket"

https://codereview.webrtc.org/1909333002/diff/20001/webrtc/audio/audio_receiv...
webrtc/audio/audio_receive_stream_unittest.cc:266: 
Add simple test case for "ReceiveRtcpPacket". Yes, it is just a call-through
right now, but it won't always be like that. With the infrastructure in place it
is more likely the test won't be forgotten down the line.

https://codereview.webrtc.org/1909333002/diff/20001/webrtc/audio/audio_send_s...
File webrtc/audio/audio_send_stream.cc (right):

https://codereview.webrtc.org/1909333002/diff/20001/webrtc/audio/audio_send_s...
webrtc/audio/audio_send_stream.cc:128: return
channel_proxy_->ReceivedRTCPPacket(static_cast<const uint8_t*>(packet),
Looks like this cast is not needed anymore?

https://codereview.webrtc.org/1909333002/diff/20001/webrtc/audio/audio_send_s...
File webrtc/audio/audio_send_stream_unittest.cc (right):

https://codereview.webrtc.org/1909333002/diff/20001/webrtc/audio/audio_send_s...
webrtc/audio/audio_send_stream_unittest.cc:73: channel_proxy_ = new
testing::NiceMock<MockVoEChannelProxy>();
Add EXPECT_CALL for [De]RegisterExternalTransport instead.

https://codereview.webrtc.org/1909333002/diff/20001/webrtc/call/call.cc
File webrtc/call/call.cc (right):

https://codereview.webrtc.org/1909333002/diff/20001/webrtc/call/call.cc#newco...
webrtc/call/call.cc:758: if (media_type == MediaType::ANY || media_type ==
MediaType::VIDEO) {
In Call::DeliverRtp, packets are forwarded to audio streams first - is there any
point in using the same order in both methods?

https://codereview.webrtc.org/1909333002/diff/20001/webrtc/call/call.cc#newco...
webrtc/call/call.cc:765: if (media_type == MediaType::ANY || media_type ==
MediaType::AUDIO) {
if media_type == ANY, is it ok that packets *may* be forwarded to both audio and
video streams?

https://codereview.webrtc.org/1909333002/diff/20001/webrtc/call/call_perf_tes...
File webrtc/call/call_perf_tests.cc (right):

https://codereview.webrtc.org/1909333002/diff/20001/webrtc/call/call_perf_tes...
webrtc/call/call_perf_tests.cc:194: const MediaType media_type_;
RTC_DISALLOW_IMPLICIT_CONSTRUCTORS

https://codereview.webrtc.org/1909333002/diff/20001/webrtc/media/engine/fakew...
File webrtc/media/engine/fakewebrtccall.cc (right):

https://codereview.webrtc.org/1909333002/diff/20001/webrtc/media/engine/fakew...
webrtc/media/engine/fakewebrtccall.cc:72: return last_packet_ ==
std::string(static_cast<const char*>(data), length);
CppReference for std::string::operator==() say it shall point at a
null-terminated sequence for char* strings:
http://www.cplusplus.com/reference/string/string/operators/

To me that indicates that the comparison may not actually compare the whole
contents of the string.

I suggest you use rtc::Buffer here and for last_packet_, as that has the
required functionality, and more accurately conveys the intent.

https://codereview.webrtc.org/1909333002/diff/20001/webrtc/media/engine/fakew...
File webrtc/media/engine/fakewebrtccall.h (right):

https://codereview.webrtc.org/1909333002/diff/20001/webrtc/media/engine/fakew...
webrtc/media/engine/fakewebrtccall.h:77: bool VerifyLastPacket(const void* data,
size_t length) const;
uint8_t*

https://codereview.webrtc.org/1909333002/diff/20001/webrtc/media/engine/webrt...
File webrtc/media/engine/webrtcvoiceengine.cc (left):

https://codereview.webrtc.org/1909333002/diff/20001/webrtc/media/engine/webrt...
webrtc/media/engine/webrtcvoiceengine.cc:2314: // Sending channels need all RTCP
packets with feedback information.
You're certain that the proper filtering of SR/RR will happen in the
RtpRtcpModule?

https://codereview.webrtc.org/1909333002/diff/20001/webrtc/media/engine/webrt...
File webrtc/media/engine/webrtcvoiceengine.cc (right):

https://codereview.webrtc.org/1909333002/diff/20001/webrtc/media/engine/webrt...
webrtc/media/engine/webrtcvoiceengine.cc:2256: 
nit: delete line

https://codereview.webrtc.org/1909333002/diff/20001/webrtc/media/engine/webrt...
webrtc/media/engine/webrtcvoiceengine.cc:2289:
call_->Receiver()->DeliverPacket(webrtc::MediaType::AUDIO,
RTC_DCHECK_NE(webrtc::PacketReceiver::DELIVERY_UNKNOWN_SSRC, delivery_result);

https://codereview.webrtc.org/1909333002/diff/20001/webrtc/media/engine/webrt...
File webrtc/media/engine/webrtcvoiceengine_unittest.cc (right):

https://codereview.webrtc.org/1909333002/diff/20001/webrtc/media/engine/webrt...
webrtc/media/engine/webrtcvoiceengine_unittest.cc:2607:
call_.GetAudioReceiveStreams().front();
Use GetRecvStream(1) instead

https://codereview.webrtc.org/1909333002/diff/20001/webrtc/media/engine/webrt...
webrtc/media/engine/webrtcvoiceengine_unittest.cc:2631:
call_.GetAudioReceiveStream(ssrc1);
Use GetRecvStream(..)

https://codereview.webrtc.org/1909333002/diff/20001/webrtc/media/engine/webrt...
webrtc/media/engine/webrtcvoiceengine_unittest.cc:2678: const
cricket::FakeAudioReceiveStream* s =
Use GetRecvStream(1) instead

https://codereview.webrtc.org/1909333002/diff/20001/webrtc/media/engine/webrt...
webrtc/media/engine/webrtcvoiceengine_unittest.cc:2687: // by webrtc::Call.
Shall this be fixed or dropped?
I believe we must support the behavior. :(
See: https://bugs.chromium.org/p/webrtc/issues/detail?id=5208

https://codereview.webrtc.org/1909333002/diff/20001/webrtc/test/mock_voe_chan...
File webrtc/test/mock_voe_channel_proxy.h (right):

https://codereview.webrtc.org/1909333002/diff/20001/webrtc/test/mock_voe_chan...
webrtc/test/mock_voe_channel_proxy.h:49:
MOCK_METHOD3(ReceivedRTPPacket,bool(const uint8_t* packet,
nit: space after ,

https://codereview.webrtc.org/1909333002/diff/20001/webrtc/voice_engine/chann...
File webrtc/voice_engine/channel_proxy.cc (right):

https://codereview.webrtc.org/1909333002/diff/20001/webrtc/voice_engine/chann...
webrtc/voice_engine/channel_proxy.cc:161: void
ChannelProxy::RegisterExternalTransport(Transport* transport) {
Add:
RTC_DCHECK(thread_checker_.CalledOnValidThread());

https://codereview.webrtc.org/1909333002/diff/20001/webrtc/voice_engine/chann...
webrtc/voice_engine/channel_proxy.cc:162:
channel()->RegisterExternalTransport(*transport);
Change Channel::RegisterExternalTransport() to take a Transport* instead. Looks
like the code in Channel will work if we supply a nullptr without fuss, and we
don't crash on the deref here.

Also, add 
int error = ...
RTC_DCHECK_EQ(0, error);
(here and below)

https://codereview.webrtc.org/1909333002/diff/20001/webrtc/voice_engine/chann...
webrtc/voice_engine/channel_proxy.cc:165: void
ChannelProxy::DeRegisterExternalTransport() {
Add:
RTC_DCHECK(thread_checker_.CalledOnValidThread());

https://codereview.webrtc.org/1909333002/diff/20001/webrtc/voice_engine/chann...
webrtc/voice_engine/channel_proxy.cc:171: const PacketTime& packet_time) {
Add:
// May be called on either worker thread or network thread.
(and below)

[tommi, 2016-04-23 16:00:08]

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

[tommi, 2016-04-23 16:00:09]

drive by

https://codereview.webrtc.org/1909333002/diff/20001/webrtc/media/engine/fakew...
File webrtc/media/engine/fakewebrtccall.cc (right):

https://codereview.webrtc.org/1909333002/diff/20001/webrtc/media/engine/fakew...
webrtc/media/engine/fakewebrtccall.cc:72: return last_packet_ ==
std::string(static_cast<const char*>(data), length);
On 2016/04/22 12:40:31, the sun wrote:
> CppReference for std::string::operator==() say it shall point at a
> null-terminated sequence for char* strings:
> http://www.cplusplus.com/reference/string/string/operators/
> 
> To me that indicates that the comparison may not actually compare the whole
> contents of the string.
> 
> I suggest you use rtc::Buffer here and for last_packet_, as that has the
> required functionality, and more accurately conveys the intent.

+1.  string is a misleading type for binary data and in some implementations
comes with an extra inline allocated buffer which isn't necessary for this
class.

Creating a copy of the data just for comparison is also something I'd prefer to
never do in our code base (since code can get copy pasted).

https://codereview.webrtc.org/1909333002/diff/20001/webrtc/media/engine/fakew...
webrtc/media/engine/fakewebrtccall.cc:78: received_packets_++;
++foo

https://codereview.webrtc.org/1909333002/diff/20001/webrtc/media/engine/fakew...
webrtc/media/engine/fakewebrtccall.cc:79: last_packet_ =
std::string(reinterpret_cast<const char*>(packet), length);
What about using a movable type such as rtc::Buffer instead?

https://codereview.webrtc.org/1909333002/diff/20001/webrtc/media/engine/webrt...
File webrtc/media/engine/webrtcvoiceengine.cc (right):

https://codereview.webrtc.org/1909333002/diff/20001/webrtc/media/engine/webrt...
webrtc/media/engine/webrtcvoiceengine.cc:2254: packet->cdata(), packet->size(),
I'd really like DeliverPacket to use rtc::CopyOnWriteBuffer.  As is, we lose the
benefits of it here and force the caller to do a deep copy.

https://codereview.webrtc.org/1909333002/diff/20001/webrtc/voice_engine/chann...
File webrtc/voice_engine/channel_proxy.h (right):

https://codereview.webrtc.org/1909333002/diff/20001/webrtc/voice_engine/chann...
webrtc/voice_engine/channel_proxy.h:78: virtual bool ReceivedRTPPacket(const
uint8_t* packet,
prefer using CopyOnWriteBuffer here instead of forcing buffer copies due to the
interface.

[mflodman, 2016-04-27 13:42:18]

So a few patch sets fighting tests, but comments should be addressed now.

PTAL

https://codereview.webrtc.org/1909333002/diff/20001/webrtc/audio/audio_receiv...
File webrtc/audio/audio_receive_stream.cc (right):

https://codereview.webrtc.org/1909333002/diff/20001/webrtc/audio/audio_receiv...
webrtc/audio/audio_receive_stream.cc:153: return
channel_proxy_->ReceivedRTCPPacket(static_cast<const uint8_t*>(packet),
On 2016/04/22 12:40:31, the sun wrote:
> Looks like this cast is not needed anymore?

Doh!

https://codereview.webrtc.org/1909333002/diff/20001/webrtc/audio/audio_receiv...
webrtc/audio/audio_receive_stream.cc:178:
remote_bitrate_estimator_->IncomingPacket(arrival_time_ms, payload_size,
On 2016/04/22 12:40:31, the sun wrote:
> stefan: is it still appropriate to fork packets off for BWE here or should
that
> happen some other way (e.g. through the Channel's RtpRtcpModule)?

For video this is done in ViEReciever, that we're refactoring and this might end
up in VideoReceiveStream too. So I'd prefer to keep it here at lest for now
until this has been settled.

ok?

https://codereview.webrtc.org/1909333002/diff/20001/webrtc/audio/audio_receiv...
webrtc/audio/audio_receive_stream.cc:182: return
channel_proxy_->ReceivedRTPPacket(packet, length, packet_time);
On 2016/04/22 12:40:31, the sun wrote:
> VoE::ReceivedRTPPacket() will:
>   if ((length < 12) || (length > 1292)) {
>     return -1;
>   }
> but it seems to me that we shouldn't make such a decision at that level, and I
> can't find anything in the code that looks like it will break if the packet is
<
> 12 bytes or > 1292 bytes. However, we should be aware that were potentially
> opening up for abuse.

Channel::ReceivedRTPPacket is calling rtp_header_parser_->Parse(received_packet,
length, &header) right away and this will catch length < 12.

I can add back a check for too large packets if you think this is needed for
audio, I'm not very familiar with the potential risks there.

In that case, I can add that check in Channel::ReceivedRTPPacket rather than
here.

WDYT?

https://codereview.webrtc.org/1909333002/diff/20001/webrtc/audio/audio_receiv...
File webrtc/audio/audio_receive_stream_unittest.cc (right):

https://codereview.webrtc.org/1909333002/diff/20001/webrtc/audio/audio_receiv...
webrtc/audio/audio_receive_stream_unittest.cc:239: TEST(AudioReceiveStreamTest,
AudioPacketUpdatesBweFeedback) {
On 2016/04/22 12:40:31, the sun wrote:
> Rename test case to "ReceiveRtpPacket"

Done.

https://codereview.webrtc.org/1909333002/diff/20001/webrtc/audio/audio_receiv...
webrtc/audio/audio_receive_stream_unittest.cc:266: 
On 2016/04/22 12:40:31, the sun wrote:
> Add simple test case for "ReceiveRtcpPacket". Yes, it is just a call-through
> right now, but it won't always be like that. With the infrastructure in place
it
> is more likely the test won't be forgotten down the line.

Done. And caught a stupid mistake done when changing the signature of channel
proxy DeliverRTCPPacket.

https://codereview.webrtc.org/1909333002/diff/20001/webrtc/audio/audio_send_s...
File webrtc/audio/audio_send_stream.cc (right):

https://codereview.webrtc.org/1909333002/diff/20001/webrtc/audio/audio_send_s...
webrtc/audio/audio_send_stream.cc:128: return
channel_proxy_->ReceivedRTCPPacket(static_cast<const uint8_t*>(packet),
On 2016/04/22 12:40:31, the sun wrote:
> Looks like this cast is not needed anymore?

Done.

https://codereview.webrtc.org/1909333002/diff/20001/webrtc/audio/audio_send_s...
File webrtc/audio/audio_send_stream_unittest.cc (right):

https://codereview.webrtc.org/1909333002/diff/20001/webrtc/audio/audio_send_s...
webrtc/audio/audio_send_stream_unittest.cc:73: channel_proxy_ = new
testing::NiceMock<MockVoEChannelProxy>();
On 2016/04/22 12:40:31, the sun wrote:
> Add EXPECT_CALL for [De]RegisterExternalTransport instead.

Done.

https://codereview.webrtc.org/1909333002/diff/20001/webrtc/call/call.cc
File webrtc/call/call.cc (right):

https://codereview.webrtc.org/1909333002/diff/20001/webrtc/call/call.cc#newco...
webrtc/call/call.cc:758: if (media_type == MediaType::ANY || media_type ==
MediaType::VIDEO) {
On 2016/04/22 12:40:31, the sun wrote:
> In Call::DeliverRtp, packets are forwarded to audio streams first - is there
any
> point in using the same order in both methods?

I don't think it really matters, but my assumption is that BWE related RTCP are
currently sent to video streams and it makes sense to process them asap. In
practice it might not really make a difference, so I can change if you'd like.

https://codereview.webrtc.org/1909333002/diff/20001/webrtc/call/call.cc#newco...
webrtc/call/call.cc:765: if (media_type == MediaType::ANY || media_type ==
MediaType::AUDIO) {
On 2016/04/22 12:40:31, the sun wrote:
> if media_type == ANY, is it ok that packets *may* be forwarded to both audio
and
> video streams?

I thought about this and I'm not sure what the standard really says in this
case, if it's ok to bundle report blocks for different media types in the same
RTCP RR.

I prefer it this way, talk with people that knows and then restrict if that is
not allowed.

https://codereview.webrtc.org/1909333002/diff/20001/webrtc/call/call_perf_tes...
File webrtc/call/call_perf_tests.cc (right):

https://codereview.webrtc.org/1909333002/diff/20001/webrtc/call/call_perf_tes...
webrtc/call/call_perf_tests.cc:194: const MediaType media_type_;
On 2016/04/22 12:40:31, the sun wrote:
> RTC_DISALLOW_IMPLICIT_CONSTRUCTORS

Done.

https://codereview.webrtc.org/1909333002/diff/20001/webrtc/media/engine/fakew...
File webrtc/media/engine/fakewebrtccall.cc (right):

https://codereview.webrtc.org/1909333002/diff/20001/webrtc/media/engine/fakew...
webrtc/media/engine/fakewebrtccall.cc:72: return last_packet_ ==
std::string(static_cast<const char*>(data), length);
On 2016/04/22 12:40:31, the sun wrote:
> CppReference for std::string::operator==() say it shall point at a
> null-terminated sequence for char* strings:
> http://www.cplusplus.com/reference/string/string/operators/
> 
> To me that indicates that the comparison may not actually compare the whole
> contents of the string.
> 
> I suggest you use rtc::Buffer here and for last_packet_, as that has the
> required functionality, and more accurately conveys the intent.

Done. This was just copied from elsewhere in these tests.

And good suggestion, I'm still in the mental mode of not using stuff in the base
folder :(

https://codereview.webrtc.org/1909333002/diff/20001/webrtc/media/engine/fakew...
webrtc/media/engine/fakewebrtccall.cc:78: received_packets_++;
On 2016/04/23 16:00:09, tommi-webrtc wrote:
> ++foo

Done.

https://codereview.webrtc.org/1909333002/diff/20001/webrtc/media/engine/fakew...
webrtc/media/engine/fakewebrtccall.cc:79: last_packet_ =
std::string(reinterpret_cast<const char*>(packet), length);
On 2016/04/23 16:00:09, tommi-webrtc wrote:
> What about using a movable type such as rtc::Buffer instead?

Done.

https://codereview.webrtc.org/1909333002/diff/20001/webrtc/media/engine/fakew...
File webrtc/media/engine/fakewebrtccall.h (right):

https://codereview.webrtc.org/1909333002/diff/20001/webrtc/media/engine/fakew...
webrtc/media/engine/fakewebrtccall.h:77: bool VerifyLastPacket(const void* data,
size_t length) const;
On 2016/04/22 12:40:31, the sun wrote:
> uint8_t*

Done.

https://codereview.webrtc.org/1909333002/diff/20001/webrtc/media/engine/webrt...
File webrtc/media/engine/webrtcvoiceengine.cc (left):

https://codereview.webrtc.org/1909333002/diff/20001/webrtc/media/engine/webrt...
webrtc/media/engine/webrtcvoiceengine.cc:2314: // Sending channels need all RTCP
packets with feedback information.
On 2016/04/22 12:40:31, the sun wrote:
> You're certain that the proper filtering of SR/RR will happen in the
> RtpRtcpModule?

That's how it's always been done fore video, so the RTP module should handle
that as is.

https://codereview.webrtc.org/1909333002/diff/20001/webrtc/media/engine/webrt...
File webrtc/media/engine/webrtcvoiceengine.cc (right):

https://codereview.webrtc.org/1909333002/diff/20001/webrtc/media/engine/webrt...
webrtc/media/engine/webrtcvoiceengine.cc:2254: packet->cdata(), packet->size(),
On 2016/04/23 16:00:09, tommi-webrtc wrote:
> I'd really like DeliverPacket to use rtc::CopyOnWriteBuffer.  As is, we lose
the
> benefits of it here and force the caller to do a deep copy.

SG without really looking into it. That should be done in another CL though and
not here, it's potentially a big change to get it right. This API was written
when we couldn't use classes in webrtc/base, so the call API should get an
overhaul now to see what can be improved.

https://codereview.webrtc.org/1909333002/diff/20001/webrtc/media/engine/webrt...
webrtc/media/engine/webrtcvoiceengine.cc:2256: 
On 2016/04/22 12:40:31, the sun wrote:
> nit: delete line

Done.

https://codereview.webrtc.org/1909333002/diff/20001/webrtc/media/engine/webrt...
webrtc/media/engine/webrtcvoiceengine.cc:2289:
call_->Receiver()->DeliverPacket(webrtc::MediaType::AUDIO,
On 2016/04/22 12:40:31, the sun wrote:
> RTC_DCHECK_NE(webrtc::PacketReceiver::DELIVERY_UNKNOWN_SSRC, delivery_result);

Done.

https://codereview.webrtc.org/1909333002/diff/20001/webrtc/media/engine/webrt...
File webrtc/media/engine/webrtcvoiceengine_unittest.cc (right):

https://codereview.webrtc.org/1909333002/diff/20001/webrtc/media/engine/webrt...
webrtc/media/engine/webrtcvoiceengine_unittest.cc:2607:
call_.GetAudioReceiveStreams().front();
On 2016/04/22 12:40:32, the sun wrote:
> Use GetRecvStream(1) instead

Done.

https://codereview.webrtc.org/1909333002/diff/20001/webrtc/media/engine/webrt...
webrtc/media/engine/webrtcvoiceengine_unittest.cc:2631:
call_.GetAudioReceiveStream(ssrc1);
On 2016/04/22 12:40:31, the sun wrote:
> Use GetRecvStream(..)

Done.

https://codereview.webrtc.org/1909333002/diff/20001/webrtc/media/engine/webrt...
webrtc/media/engine/webrtcvoiceengine_unittest.cc:2678: const
cricket::FakeAudioReceiveStream* s =
On 2016/04/22 12:40:32, the sun wrote:
> Use GetRecvStream(1) instead

Done.

https://codereview.webrtc.org/1909333002/diff/20001/webrtc/media/engine/webrt...
webrtc/media/engine/webrtcvoiceengine_unittest.cc:2687: // by webrtc::Call.
Shall this be fixed or dropped?
On 2016/04/22 12:40:32, the sun wrote:
> I believe we must support the behavior. :(
> See: https://bugs.chromium.org/p/webrtc/issues/detail?id=5208

Changed and supported now. A potential previously created default stream will be
deleted though, to not have more than one at the time and to not add _a lot_ of
code complexity to support changing ssrc for an existing default receive stream

https://codereview.webrtc.org/1909333002/diff/20001/webrtc/test/mock_voe_chan...
File webrtc/test/mock_voe_channel_proxy.h (right):

https://codereview.webrtc.org/1909333002/diff/20001/webrtc/test/mock_voe_chan...
webrtc/test/mock_voe_channel_proxy.h:49:
MOCK_METHOD3(ReceivedRTPPacket,bool(const uint8_t* packet,
On 2016/04/22 12:40:32, the sun wrote:
> nit: space after ,

Done.

https://codereview.webrtc.org/1909333002/diff/20001/webrtc/voice_engine/chann...
File webrtc/voice_engine/channel_proxy.cc (right):

https://codereview.webrtc.org/1909333002/diff/20001/webrtc/voice_engine/chann...
webrtc/voice_engine/channel_proxy.cc:161: void
ChannelProxy::RegisterExternalTransport(Transport* transport) {
On 2016/04/22 12:40:32, the sun wrote:
> Add:
> RTC_DCHECK(thread_checker_.CalledOnValidThread());

Done.

https://codereview.webrtc.org/1909333002/diff/20001/webrtc/voice_engine/chann...
webrtc/voice_engine/channel_proxy.cc:162:
channel()->RegisterExternalTransport(*transport);
On 2016/04/22 12:40:32, the sun wrote:
> Change Channel::RegisterExternalTransport() to take a Transport* instead.
Looks
> like the code in Channel will work if we supply a nullptr without fuss, and we
> don't crash on the deref here.
> 
> Also, add 
> int error = ...
> RTC_DCHECK_EQ(0, error);
> (here and below)

Done. And agree is should be safe, but the logic could be optimized there since
I assume we're always using external transport.

https://codereview.webrtc.org/1909333002/diff/20001/webrtc/voice_engine/chann...
webrtc/voice_engine/channel_proxy.cc:165: void
ChannelProxy::DeRegisterExternalTransport() {
On 2016/04/22 12:40:32, the sun wrote:
> Add:
> RTC_DCHECK(thread_checker_.CalledOnValidThread());

Done.

https://codereview.webrtc.org/1909333002/diff/20001/webrtc/voice_engine/chann...
webrtc/voice_engine/channel_proxy.cc:171: const PacketTime& packet_time) {
On 2016/04/22 12:40:32, the sun wrote:
> Add:
> // May be called on either worker thread or network thread.
> (and below)

Done.

https://codereview.webrtc.org/1909333002/diff/20001/webrtc/voice_engine/chann...
File webrtc/voice_engine/channel_proxy.h (right):

https://codereview.webrtc.org/1909333002/diff/20001/webrtc/voice_engine/chann...
webrtc/voice_engine/channel_proxy.h:78: virtual bool ReceivedRTPPacket(const
uint8_t* packet,
On 2016/04/23 16:00:09, tommi-webrtc wrote:
> prefer using CopyOnWriteBuffer here instead of forcing buffer copies due to
the
> interface.

I think that as something we should look at, but not in this CL. The contract
has been to copy the incoming data to store it in a buffer used by the decoder
anyway and I'm not how much we really would gain by doing this change. But we
can take a look at it.

[the sun, 2016-04-28 09:15:57]

lgtm % minor comments

https://codereview.webrtc.org/1909333002/diff/20001/webrtc/audio/audio_receiv...
File webrtc/audio/audio_receive_stream.cc (right):

https://codereview.webrtc.org/1909333002/diff/20001/webrtc/audio/audio_receiv...
webrtc/audio/audio_receive_stream.cc:178:
remote_bitrate_estimator_->IncomingPacket(arrival_time_ms, payload_size,
On 2016/04/27 13:42:17, mflodman wrote:
> On 2016/04/22 12:40:31, the sun wrote:
> > stefan: is it still appropriate to fork packets off for BWE here or should
> that
> > happen some other way (e.g. through the Channel's RtpRtcpModule)?
> 
> For video this is done in ViEReciever, that we're refactoring and this might
end
> up in VideoReceiveStream too. So I'd prefer to keep it here at lest for now
> until this has been settled.
> 
> ok?

ok, just wanted to make sure we won't magically send packets twice to the RBE or
something.

https://codereview.webrtc.org/1909333002/diff/20001/webrtc/audio/audio_receiv...
webrtc/audio/audio_receive_stream.cc:182: return
channel_proxy_->ReceivedRTPPacket(packet, length, packet_time);
On 2016/04/27 13:42:17, mflodman wrote:
> On 2016/04/22 12:40:31, the sun wrote:
> > VoE::ReceivedRTPPacket() will:
> >   if ((length < 12) || (length > 1292)) {
> >     return -1;
> >   }
> > but it seems to me that we shouldn't make such a decision at that level, and
I
> > can't find anything in the code that looks like it will break if the packet
is
> <
> > 12 bytes or > 1292 bytes. However, we should be aware that were potentially
> > opening up for abuse.
> 
> Channel::ReceivedRTPPacket is calling
rtp_header_parser_->Parse(received_packet,
> length, &header) right away and this will catch length < 12.
> 
> I can add back a check for too large packets if you think this is needed for
> audio, I'm not very familiar with the potential risks there.
> 
> In that case, I can add that check in Channel::ReceivedRTPPacket rather than
> here.
> 
> WDYT?

Yes, please add the check in Channel::ReceivedRTPPacket().

https://codereview.webrtc.org/1909333002/diff/20001/webrtc/call/call.cc
File webrtc/call/call.cc (right):

https://codereview.webrtc.org/1909333002/diff/20001/webrtc/call/call.cc#newco...
webrtc/call/call.cc:758: if (media_type == MediaType::ANY || media_type ==
MediaType::VIDEO) {
On 2016/04/27 13:42:17, mflodman wrote:
> On 2016/04/22 12:40:31, the sun wrote:
> > In Call::DeliverRtp, packets are forwarded to audio streams first - is there
> any
> > point in using the same order in both methods?
> 
> I don't think it really matters, but my assumption is that BWE related RTCP
are
> currently sent to video streams and it makes sense to process them asap. In
> practice it might not really make a difference, so I can change if you'd like.

No, but add a comment about that.

https://codereview.webrtc.org/1909333002/diff/20001/webrtc/call/call.cc#newco...
webrtc/call/call.cc:765: if (media_type == MediaType::ANY || media_type ==
MediaType::AUDIO) {
On 2016/04/27 13:42:17, mflodman wrote:
> On 2016/04/22 12:40:31, the sun wrote:
> > if media_type == ANY, is it ok that packets *may* be forwarded to both audio
> and
> > video streams?
> 
> I thought about this and I'm not sure what the standard really says in this
> case, if it's ok to bundle report blocks for different media types in the same
> RTCP RR.
> 
> I prefer it this way, talk with people that knows and then restrict if that is
> not allowed.

Acknowledged.

https://codereview.webrtc.org/1909333002/diff/100001/webrtc/media/engine/fake...
File webrtc/media/engine/fakewebrtccall.h (right):

https://codereview.webrtc.org/1909333002/diff/100001/webrtc/media/engine/fake...
webrtc/media/engine/fakewebrtccall.h:84: bool DeliverRtcp(const uint8_t* packet,
size_t length) override;
You can just leave this method as it was (see:
https://codereview.webrtc.org/1924793002/)

https://codereview.webrtc.org/1909333002/diff/100001/webrtc/media/engine/webr...
File webrtc/media/engine/webrtcvoiceengine.cc (right):

https://codereview.webrtc.org/1909333002/diff/100001/webrtc/media/engine/webr...
webrtc/media/engine/webrtcvoiceengine.cc:2256: 
nit: remove blank line

https://codereview.webrtc.org/1909333002/diff/100001/webrtc/media/engine/webr...
webrtc/media/engine/webrtcvoiceengine.cc:2261: // Create a default receive
stream for this unsignalled annd previously not
s/annd/and

https://codereview.webrtc.org/1909333002/diff/100001/webrtc/media/engine/webr...
webrtc/media/engine/webrtcvoiceengine.cc:2262: // received ssrc. If there
already is a default receive stream, delete it.
Add:
// See: https://bugs.chromium.org/p/webrtc/issues/detail?id=5208

https://codereview.webrtc.org/1909333002/diff/100001/webrtc/media/engine/webr...
webrtc/media/engine/webrtcvoiceengine.cc:2268: if (default_recv_ssrc_ != -1) {
Thanks! This logic is actually easier to follow now.

https://codereview.webrtc.org/1909333002/diff/100001/webrtc/media/engine/webr...
File webrtc/media/engine/webrtcvoiceengine_unittest.cc (right):

https://codereview.webrtc.org/1909333002/diff/100001/webrtc/media/engine/webr...
webrtc/media/engine/webrtcvoiceengine_unittest.cc:2687: // creating receive
streams for SSRC!=0.
Note that the behavior is now changed and this comment does not apply. We will
never support unsignalled SSRC=0 now.

https://codereview.webrtc.org/1909333002/diff/100001/webrtc/media/engine/webr...
webrtc/media/engine/webrtcvoiceengine_unittest.cc:2693: // Verify the old
default stream is deleted.
// Verify we only have one default stream.
would be better to say

https://codereview.webrtc.org/1909333002/diff/100001/webrtc/test/call_test.h
File webrtc/test/call_test.h (left):

https://codereview.webrtc.org/1909333002/diff/100001/webrtc/test/call_test.h#...
webrtc/test/call_test.h:123: VoENetwork* network;
Remove?

[stefan-webrtc, 2016-04-28 09:27:48]

lgtm

https://codereview.webrtc.org/1909333002/diff/20001/webrtc/audio/audio_receiv...
File webrtc/audio/audio_receive_stream.cc (right):

https://codereview.webrtc.org/1909333002/diff/20001/webrtc/audio/audio_receiv...
webrtc/audio/audio_receive_stream.cc:178:
remote_bitrate_estimator_->IncomingPacket(arrival_time_ms, payload_size,
On 2016/04/28 09:15:56, the sun wrote:
> On 2016/04/27 13:42:17, mflodman wrote:
> > On 2016/04/22 12:40:31, the sun wrote:
> > > stefan: is it still appropriate to fork packets off for BWE here or should
> > that
> > > happen some other way (e.g. through the Channel's RtpRtcpModule)?
> > 
> > For video this is done in ViEReciever, that we're refactoring and this might
> end
> > up in VideoReceiveStream too. So I'd prefer to keep it here at lest for now
> > until this has been settled.
> > 
> > ok?
> 
> ok, just wanted to make sure we won't magically send packets twice to the RBE
or
> something.

Should be fine, and I also prefer it being done this way.

[mflodman, 2016-04-29 05:54:01]

Tommi,
Since you "only" had comments on old test behavior not changed in this CL and
done in the same way elsewhere, I'll go ahead and press commit and we can
continue the discussion.

https://codereview.webrtc.org/1909333002/diff/100001/webrtc/media/engine/fake...
File webrtc/media/engine/fakewebrtccall.h (right):

https://codereview.webrtc.org/1909333002/diff/100001/webrtc/media/engine/fake...
webrtc/media/engine/fakewebrtccall.h:84: bool DeliverRtcp(const uint8_t* packet,
size_t length) override;
On 2016/04/28 09:15:56, the sun wrote:
> You can just leave this method as it was (see:
> https://codereview.webrtc.org/1924793002/)

Acknowledged.

https://codereview.webrtc.org/1909333002/diff/100001/webrtc/media/engine/webr...
File webrtc/media/engine/webrtcvoiceengine.cc (right):

https://codereview.webrtc.org/1909333002/diff/100001/webrtc/media/engine/webr...
webrtc/media/engine/webrtcvoiceengine.cc:2256: 
On 2016/04/28 09:15:56, the sun wrote:
> nit: remove blank line

Done.

https://codereview.webrtc.org/1909333002/diff/100001/webrtc/media/engine/webr...
webrtc/media/engine/webrtcvoiceengine.cc:2261: // Create a default receive
stream for this unsignalled annd previously not
On 2016/04/28 09:15:56, the sun wrote:
> s/annd/and

Done.

https://codereview.webrtc.org/1909333002/diff/100001/webrtc/media/engine/webr...
webrtc/media/engine/webrtcvoiceengine.cc:2262: // received ssrc. If there
already is a default receive stream, delete it.
On 2016/04/28 09:15:56, the sun wrote:
> Add:
> // See: https://bugs.chromium.org/p/webrtc/issues/detail?id=5208

Done.

https://codereview.webrtc.org/1909333002/diff/100001/webrtc/media/engine/webr...
webrtc/media/engine/webrtcvoiceengine.cc:2268: if (default_recv_ssrc_ != -1) {
On 2016/04/28 09:15:56, the sun wrote:
> Thanks! This logic is actually easier to follow now.

Agree, thanks!

https://codereview.webrtc.org/1909333002/diff/100001/webrtc/media/engine/webr...
File webrtc/media/engine/webrtcvoiceengine_unittest.cc (right):

https://codereview.webrtc.org/1909333002/diff/100001/webrtc/media/engine/webr...
webrtc/media/engine/webrtcvoiceengine_unittest.cc:2687: // creating receive
streams for SSRC!=0.
On 2016/04/28 09:15:57, the sun wrote:
> Note that the behavior is now changed and this comment does not apply. We will
> never support unsignalled SSRC=0 now.

Roght, comment updated.

https://codereview.webrtc.org/1909333002/diff/100001/webrtc/media/engine/webr...
webrtc/media/engine/webrtcvoiceengine_unittest.cc:2693: // Verify the old
default stream is deleted.
On 2016/04/28 09:15:57, the sun wrote:
> // Verify we only have one default stream.
> would be better to say

Agree, changed.

https://codereview.webrtc.org/1909333002/diff/100001/webrtc/test/call_test.h
File webrtc/test/call_test.h (left):

https://codereview.webrtc.org/1909333002/diff/100001/webrtc/test/call_test.h#...
webrtc/test/call_test.h:123: VoENetwork* network;
On 2016/04/28 09:15:57, the sun wrote:
> Remove?

Done here and in cc-file.

[mflodman, 2016-04-29 05:54:32]

Description was changed from

==========
Switch voice transport to use Call and Stream instead of VoENetwork.

VoENetwork is kept for now, but is not really used anylonger.

webrtcvoiceengine is changed to have the same behavior for unsignaled
ssrc as video has, which is reflected by disabling one test case and
this will be discussed and followed up.

BUG=webrtc:5079
==========

to

==========
Switch voice transport to use Call and Stream instead of VoENetwork.

VoENetwork is kept for now, but is not really used anylonger.

webrtcvoiceengine is changed to have the same behavior for unsignaled
ssrc as video has, which is reflected by disabling one test case and
this will be discussed and followed up.

BUG=webrtc:5079

TBR=tommi
==========

[mflodman, 2016-04-29 05:55:02]

The CQ bit was checked by mflodman@webrtc.org

[mflodman, 2016-04-29 05:55:02]

The patchset sent to the CQ was uploaded after l-g-t-m from
solenberg@webrtc.org, stefan@webrtc.org
Link to the patchset: https://codereview.webrtc.org/1909333002/#ps120001 (title:
"Addressed coments on ps#6")

[commit-bot: I haz the power, 2016-04-29 05:55:06]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1909333002/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1909333002/120001

[commit-bot: I haz the power, 2016-04-29 07:57:18]

Description was changed from

==========
Switch voice transport to use Call and Stream instead of VoENetwork.

VoENetwork is kept for now, but is not really used anylonger.

webrtcvoiceengine is changed to have the same behavior for unsignaled
ssrc as video has, which is reflected by disabling one test case and
this will be discussed and followed up.

BUG=webrtc:5079

TBR=tommi
==========

to

==========
Switch voice transport to use Call and Stream instead of VoENetwork.

VoENetwork is kept for now, but is not really used anylonger.

webrtcvoiceengine is changed to have the same behavior for unsignaled
ssrc as video has, which is reflected by disabling one test case and
this will be discussed and followed up.

BUG=webrtc:5079

TBR=tommi
==========

[commit-bot: I haz the power, 2016-04-29 07:57:19]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-05-01 22:01:37]

Description was changed from

==========
Switch voice transport to use Call and Stream instead of VoENetwork.

VoENetwork is kept for now, but is not really used anylonger.

webrtcvoiceengine is changed to have the same behavior for unsignaled
ssrc as video has, which is reflected by disabling one test case and
this will be discussed and followed up.

BUG=webrtc:5079

TBR=tommi
==========

to

==========
Switch voice transport to use Call and Stream instead of VoENetwork.

VoENetwork is kept for now, but is not really used anylonger.

webrtcvoiceengine is changed to have the same behavior for unsignaled
ssrc as video has, which is reflected by disabling one test case and
this will be discussed and followed up.

BUG=webrtc:5079

TBR=tommi

Committed: https://crrev.com/3d7db263b902f319b5b0c828ccff59cd8664b937
Cr-Commit-Position: refs/heads/master@{#12555}
==========

[commit-bot: I haz the power, 2016-05-01 22:01:39]

Patchset 7 (id:??) landed as
https://crrev.com/3d7db263b902f319b5b0c828ccff59cd8664b937
Cr-Commit-Position: refs/heads/master@{#12555}