Add RemoteEstimatorProxy for capturing receive times

Add RemoteEstimatorProxy for capturing receive times

For use when send-side bandwidth estimation is enabled.

Receive times need to be captured, buffered and then sent using
TransportFeedback RTCP messaged back to the send side.

BUG=webrtc:4173
Committed: https://crrev.com/233bd87d45bbeeec50d7687e7d98c1cfc7f65562
Cr-Commit-Position: refs/heads/master@{#9898}

[sprang_webrtc, 2015-08-18 09:36:50]

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

[sprang_webrtc, 2015-08-31 13:20:11]

sprang@webrtc.org changed reviewers:
+ stefan@webrtc.org
- mflodman@webrtc.org

[sprang_webrtc, 2015-08-31 13:20:11]

Reassigning to stefan@

[stefan-webrtc, 2015-09-04 07:31:36]

https://codereview.webrtc.org/1290813008/diff/1/webrtc/modules/interface/modu...
File webrtc/modules/interface/module_common_types.h (right):

https://codereview.webrtc.org/1290813008/diff/1/webrtc/modules/interface/modu...
webrtc/modules/interface/module_common_types.h:770: class
SequenceNumberUnwrapper {
We have similar tools for timestamps in base/, but let's put it here to start
with since we already have timestamp and seq num stuff here.

https://codereview.webrtc.org/1290813008/diff/1/webrtc/modules/interface/modu...
webrtc/modules/interface/module_common_types.h:784: } else if (delta > 0 &&
(last_seq_ + delta - (1 << 16)) >= 0) {
Comment on what this case captures.

https://codereview.webrtc.org/1290813008/diff/1/webrtc/modules/pacing/include...
File webrtc/modules/pacing/include/packet_router.h (right):

https://codereview.webrtc.org/1290813008/diff/1/webrtc/modules/pacing/include...
webrtc/modules/pacing/include/packet_router.h:50: virtual bool
SendFeedback(rtcp::TransportFeedback* packet);
Isn't PacketRouter actually living on the send side? Feels a bit confusing to
me. A receive shouldn't need a PacketRouter?

https://codereview.webrtc.org/1290813008/diff/1/webrtc/modules/pacing/packet_...
File webrtc/modules/pacing/packet_router.cc (right):

https://codereview.webrtc.org/1290813008/diff/1/webrtc/modules/pacing/packet_...
webrtc/modules/pacing/packet_router.cc:96:
packet->WithPacketSenderSsrc(rtp_module->SSRC());
Does this replace the ssrc in the feedback packet?

https://codereview.webrtc.org/1290813008/diff/1/webrtc/modules/remote_bitrate...
File webrtc/modules/remote_bitrate_estimator/remote_estimator_proxy.cc (right):

https://codereview.webrtc.org/1290813008/diff/1/webrtc/modules/remote_bitrate...
webrtc/modules/remote_bitrate_estimator/remote_estimator_proxy.cc:77:
time_until_next = (last_process_time_ms_ + kDefaultProcessIntervalMs - now);
remove the parentheses

https://codereview.webrtc.org/1290813008/diff/1/webrtc/modules/remote_bitrate...
webrtc/modules/remote_bitrate_estimator/remote_estimator_proxy.cc:83: //
TODO(sprang): Perhaps we need a dedicated thread here instead?
Could you comment on why?

https://codereview.webrtc.org/1290813008/diff/1/webrtc/modules/remote_bitrate...
webrtc/modules/remote_bitrate_estimator/remote_estimator_proxy.cc:115:
received_packets_.erase(delete_it);
Prefer received_packets_.erase(delete_it++);

https://codereview.webrtc.org/1290813008/diff/1/webrtc/modules/remote_bitrate...
webrtc/modules/remote_bitrate_estimator/remote_estimator_proxy.cc:131: auto it =
received_packets_.find(window_start_seq_);
Shouldn't this be received_packets_.begin()? If so you probably don't need the
window_start_seq_.

https://codereview.webrtc.org/1290813008/diff/1/webrtc/modules/remote_bitrate...
webrtc/modules/remote_bitrate_estimator/remote_estimator_proxy.cc:144:
LOG(LS_WARNING) << "Unable to build feedback packet for #" << it->first;
Should this ever happen? Seems like we have failed unwrapping if this happens.

https://codereview.webrtc.org/1290813008/diff/1/webrtc/modules/remote_bitrate...
File webrtc/modules/remote_bitrate_estimator/remote_estimator_proxy.h (right):

https://codereview.webrtc.org/1290813008/diff/1/webrtc/modules/remote_bitrate...
webrtc/modules/remote_bitrate_estimator/remote_estimator_proxy.h:53: static
const int kBackWindowMs;
Can these be in the .cc file?

https://codereview.webrtc.org/1290813008/diff/1/webrtc/modules/remote_bitrate...
webrtc/modules/remote_bitrate_estimator/remote_estimator_proxy.h:58: bool
BuildFeedbackPacket(rtcp::TransportFeedback* feedback_packetket);
feedback_packet

https://codereview.webrtc.org/1290813008/diff/1/webrtc/modules/remote_bitrate...
webrtc/modules/remote_bitrate_estimator/remote_estimator_proxy.h:71:
std::map<int64_t, int64_t> received_packets_ GUARDED_BY(&lock_);
Maybe a better name would be packet_arrival_times_?

https://codereview.webrtc.org/1290813008/diff/1/webrtc/modules/remote_bitrate...
File webrtc/modules/remote_bitrate_estimator/remote_estimator_proxy_unittest.cc
(right):

https://codereview.webrtc.org/1290813008/diff/1/webrtc/modules/remote_bitrate...
webrtc/modules/remote_bitrate_estimator/remote_estimator_proxy_unittest.cc:143:
.RetiresOnSaturation();
Seems a bit odd to me that this is needed in combination with Times(1) and
WillOnce(). Could you explain this to a mock rookie? :)

[sprang_webrtc, 2015-09-04 13:29:44]

https://codereview.webrtc.org/1290813008/diff/1/webrtc/modules/interface/modu...
File webrtc/modules/interface/module_common_types.h (right):

https://codereview.webrtc.org/1290813008/diff/1/webrtc/modules/interface/modu...
webrtc/modules/interface/module_common_types.h:770: class
SequenceNumberUnwrapper {
On 2015/09/04 07:31:35, stefan-webrtc (holmer) wrote:
> We have similar tools for timestamps in base/, but let's put it here to start
> with since we already have timestamp and seq num stuff here.

That was exactly my reasoning.
Note that this is actually up for review separately.. 
https://codereview.webrtc.org/1209623002/

https://codereview.webrtc.org/1290813008/diff/1/webrtc/modules/interface/modu...
webrtc/modules/interface/module_common_types.h:784: } else if (delta > 0 &&
(last_seq_ + delta - (1 << 16)) >= 0) {
On 2015/09/04 07:31:36, stefan-webrtc (holmer) wrote:
> Comment on what this case captures.

Done.

https://codereview.webrtc.org/1290813008/diff/1/webrtc/modules/pacing/include...
File webrtc/modules/pacing/include/packet_router.h (right):

https://codereview.webrtc.org/1290813008/diff/1/webrtc/modules/pacing/include...
webrtc/modules/pacing/include/packet_router.h:50: virtual bool
SendFeedback(rtcp::TransportFeedback* packet);
On 2015/09/04 07:31:36, stefan-webrtc (holmer) wrote:
> Isn't PacketRouter actually living on the send side? Feels a bit confusing to
> me. A receive shouldn't need a PacketRouter?

Receive times are collected on a call-level, no in the rtp module, so we don't
have a natural way to send the transport feedback rtcp packet back to the send
side. The packet router will find a suitable rtcp sender in a similar manner to
how i finds a suitable rtp sender for the payload on the send side.

https://codereview.webrtc.org/1290813008/diff/1/webrtc/modules/pacing/packet_...
File webrtc/modules/pacing/packet_router.cc (right):

https://codereview.webrtc.org/1290813008/diff/1/webrtc/modules/pacing/packet_...
webrtc/modules/pacing/packet_router.cc:96:
packet->WithPacketSenderSsrc(rtp_module->SSRC());
On 2015/09/04 07:31:36, stefan-webrtc (holmer) wrote:
> Does this replace the ssrc in the feedback packet?

The packet sender ssrc is changed, yes. There is also a media source ssrc, which
is not changed here.

https://codereview.webrtc.org/1290813008/diff/1/webrtc/modules/remote_bitrate...
File webrtc/modules/remote_bitrate_estimator/remote_estimator_proxy.cc (right):

https://codereview.webrtc.org/1290813008/diff/1/webrtc/modules/remote_bitrate...
webrtc/modules/remote_bitrate_estimator/remote_estimator_proxy.cc:77:
time_until_next = (last_process_time_ms_ + kDefaultProcessIntervalMs - now);
On 2015/09/04 07:31:36, stefan-webrtc (holmer) wrote:
> remove the parentheses

Done.

https://codereview.webrtc.org/1290813008/diff/1/webrtc/modules/remote_bitrate...
webrtc/modules/remote_bitrate_estimator/remote_estimator_proxy.cc:83: //
TODO(sprang): Perhaps we need a dedicated thread here instead?
On 2015/09/04 07:31:36, stefan-webrtc (holmer) wrote:
> Could you comment on why?

I no longer think we do, so I'm removing this comment. :)

https://codereview.webrtc.org/1290813008/diff/1/webrtc/modules/remote_bitrate...
webrtc/modules/remote_bitrate_estimator/remote_estimator_proxy.cc:115:
received_packets_.erase(delete_it);
On 2015/09/04 07:31:36, stefan-webrtc (holmer) wrote:
> Prefer received_packets_.erase(delete_it++);

Done.

https://codereview.webrtc.org/1290813008/diff/1/webrtc/modules/remote_bitrate...
webrtc/modules/remote_bitrate_estimator/remote_estimator_proxy.cc:131: auto it =
received_packets_.find(window_start_seq_);
On 2015/09/04 07:31:36, stefan-webrtc (holmer) wrote:
> Shouldn't this be received_packets_.begin()? If so you probably don't need the
> window_start_seq_.

No, window_start_seq_ is the first packet to be included in this feedback
message.
Since packets with lower sequence can arrive later due to reordering, we need to
keep a history of packets which we might need to send again. But probably not.

https://codereview.webrtc.org/1290813008/diff/1/webrtc/modules/remote_bitrate...
webrtc/modules/remote_bitrate_estimator/remote_estimator_proxy.cc:144:
LOG(LS_WARNING) << "Unable to build feedback packet for #" << it->first;
On 2015/09/04 07:31:36, stefan-webrtc (holmer) wrote:
> Should this ever happen? Seems like we have failed unwrapping if this happens.

I'm not sure what kind of error TransportFeedback builder could throw here, but
I think I had one in mind... In any case, if we don't add this clause and that
case happens, we will end up in an infinite loop here...

https://codereview.webrtc.org/1290813008/diff/1/webrtc/modules/remote_bitrate...
File webrtc/modules/remote_bitrate_estimator/remote_estimator_proxy.h (right):

https://codereview.webrtc.org/1290813008/diff/1/webrtc/modules/remote_bitrate...
webrtc/modules/remote_bitrate_estimator/remote_estimator_proxy.h:53: static
const int kBackWindowMs;
On 2015/09/04 07:31:36, stefan-webrtc (holmer) wrote:
> Can these be in the .cc file?

Put them here so the unit tests can access them.

https://codereview.webrtc.org/1290813008/diff/1/webrtc/modules/remote_bitrate...
webrtc/modules/remote_bitrate_estimator/remote_estimator_proxy.h:58: bool
BuildFeedbackPacket(rtcp::TransportFeedback* feedback_packetket);
On 2015/09/04 07:31:36, stefan-webrtc (holmer) wrote:
> feedback_packet

Done.

https://codereview.webrtc.org/1290813008/diff/1/webrtc/modules/remote_bitrate...
webrtc/modules/remote_bitrate_estimator/remote_estimator_proxy.h:71:
std::map<int64_t, int64_t> received_packets_ GUARDED_BY(&lock_);
On 2015/09/04 07:31:36, stefan-webrtc (holmer) wrote:
> Maybe a better name would be packet_arrival_times_?

Done.

https://codereview.webrtc.org/1290813008/diff/1/webrtc/modules/remote_bitrate...
File webrtc/modules/remote_bitrate_estimator/remote_estimator_proxy_unittest.cc
(right):

https://codereview.webrtc.org/1290813008/diff/1/webrtc/modules/remote_bitrate...
webrtc/modules/remote_bitrate_estimator/remote_estimator_proxy_unittest.cc:143:
.RetiresOnSaturation();
On 2015/09/04 07:31:36, stefan-webrtc (holmer) wrote:
> Seems a bit odd to me that this is needed in combination with Times(1) and
> WillOnce(). Could you explain this to a mock rookie? :)

I'm a rookie! Brought this over after following some example. Seems you are
right, not needed in this case. :)

[stefan-webrtc, 2015-09-07 10:12:21]

https://codereview.webrtc.org/1290813008/diff/1/webrtc/modules/pacing/include...
File webrtc/modules/pacing/include/packet_router.h (right):

https://codereview.webrtc.org/1290813008/diff/1/webrtc/modules/pacing/include...
webrtc/modules/pacing/include/packet_router.h:50: virtual bool
SendFeedback(rtcp::TransportFeedback* packet);
On 2015/09/04 13:29:44, språng wrote:
> On 2015/09/04 07:31:36, stefan-webrtc (holmer) wrote:
> > Isn't PacketRouter actually living on the send side? Feels a bit confusing
to
> > me. A receive shouldn't need a PacketRouter?
> 
> Receive times are collected on a call-level, no in the rtp module, so we don't
> have a natural way to send the transport feedback rtcp packet back to the send
> side. The packet router will find a suitable rtcp sender in a similar manner
to
> how i finds a suitable rtp sender for the payload on the send side.

Ok, maybe that is reasonable. How is it set up with the correct modules?

https://codereview.webrtc.org/1290813008/diff/1/webrtc/modules/remote_bitrate...
File webrtc/modules/remote_bitrate_estimator/remote_estimator_proxy.cc (right):

https://codereview.webrtc.org/1290813008/diff/1/webrtc/modules/remote_bitrate...
webrtc/modules/remote_bitrate_estimator/remote_estimator_proxy.cc:131: auto it =
received_packets_.find(window_start_seq_);
On 2015/09/04 13:29:44, språng wrote:
> On 2015/09/04 07:31:36, stefan-webrtc (holmer) wrote:
> > Shouldn't this be received_packets_.begin()? If so you probably don't need
the
> > window_start_seq_.
> 
> No, window_start_seq_ is the first packet to be included in this feedback
> message.
> Since packets with lower sequence can arrive later due to reordering, we need
to
> keep a history of packets which we might need to send again. But probably not.

Right, maybe a comment on that somewhere? Or do you already have one?

https://codereview.webrtc.org/1290813008/diff/1/webrtc/modules/remote_bitrate...
webrtc/modules/remote_bitrate_estimator/remote_estimator_proxy.cc:144:
LOG(LS_WARNING) << "Unable to build feedback packet for #" << it->first;
On 2015/09/04 13:29:44, språng wrote:
> On 2015/09/04 07:31:36, stefan-webrtc (holmer) wrote:
> > Should this ever happen? Seems like we have failed unwrapping if this
happens.
> 
> I'm not sure what kind of error TransportFeedback builder could throw here,
but
> I think I had one in mind... In any case, if we don't add this clause and that
> case happens, we will end up in an infinite loop here...

Right. I mostly want us to be sure that it doesn't fail if it shouldn't be able
to fail. Add a DCHECK too? Or a CHECK?

[sprang_webrtc, 2015-09-07 11:14:35]

https://codereview.webrtc.org/1290813008/diff/1/webrtc/modules/pacing/include...
File webrtc/modules/pacing/include/packet_router.h (right):

https://codereview.webrtc.org/1290813008/diff/1/webrtc/modules/pacing/include...
webrtc/modules/pacing/include/packet_router.h:50: virtual bool
SendFeedback(rtcp::TransportFeedback* packet);
On 2015/09/07 10:12:21, stefan-webrtc (holmer) wrote:
> On 2015/09/04 13:29:44, språng wrote:
> > On 2015/09/04 07:31:36, stefan-webrtc (holmer) wrote:
> > > Isn't PacketRouter actually living on the send side? Feels a bit confusing
> to
> > > me. A receive shouldn't need a PacketRouter?
> > 
> > Receive times are collected on a call-level, no in the rtp module, so we
don't
> > have a natural way to send the transport feedback rtcp packet back to the
send
> > side. The packet router will find a suitable rtcp sender in a similar manner
> to
> > how i finds a suitable rtp sender for the payload on the send side.
> 
> Ok, maybe that is reasonable. How is it set up with the correct modules?

Modules that can send rtcp are added to this router even if they don't send rtp
payload. That change probably ended up in one of the other CL's in the
pipeline...

https://codereview.webrtc.org/1290813008/diff/1/webrtc/modules/remote_bitrate...
File webrtc/modules/remote_bitrate_estimator/remote_estimator_proxy.cc (right):

https://codereview.webrtc.org/1290813008/diff/1/webrtc/modules/remote_bitrate...
webrtc/modules/remote_bitrate_estimator/remote_estimator_proxy.cc:131: auto it =
received_packets_.find(window_start_seq_);
On 2015/09/07 10:12:21, stefan-webrtc (holmer) wrote:
> On 2015/09/04 13:29:44, språng wrote:
> > On 2015/09/04 07:31:36, stefan-webrtc (holmer) wrote:
> > > Shouldn't this be received_packets_.begin()? If so you probably don't need
> the
> > > window_start_seq_.
> > 
> > No, window_start_seq_ is the first packet to be included in this feedback
> > message.
> > Since packets with lower sequence can arrive later due to reordering, we
need
> to
> > keep a history of packets which we might need to send again. But probably
not.
> 
> Right, maybe a comment on that somewhere? Or do you already have one?

Done.

https://codereview.webrtc.org/1290813008/diff/1/webrtc/modules/remote_bitrate...
webrtc/modules/remote_bitrate_estimator/remote_estimator_proxy.cc:144:
LOG(LS_WARNING) << "Unable to build feedback packet for #" << it->first;
On 2015/09/07 10:12:21, stefan-webrtc (holmer) wrote:
> On 2015/09/04 13:29:44, språng wrote:
> > On 2015/09/04 07:31:36, stefan-webrtc (holmer) wrote:
> > > Should this ever happen? Seems like we have failed unwrapping if this
> happens.
> > 
> > I'm not sure what kind of error TransportFeedback builder could throw here,
> but
> > I think I had one in mind... In any case, if we don't add this clause and
that
> > case happens, we will end up in an infinite loop here...
> 
> Right. I mostly want us to be sure that it doesn't fail if it shouldn't be
able
> to fail. Add a DCHECK too? Or a CHECK?

A CHECK is perhaps in order here.

[stefan-webrtc, 2015-09-07 11:18:53]

lgtm

https://codereview.webrtc.org/1290813008/diff/40001/webrtc/modules/remote_bit...
File webrtc/modules/remote_bitrate_estimator/remote_estimator_proxy.cc (right):

https://codereview.webrtc.org/1290813008/diff/40001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/remote_estimator_proxy.cc:142: // If we
can't even add the first seq to the feedback pack, we won't be
feedback packet

[stefan-webrtc, 2015-09-07 11:19:07]

Remember to update the description.

[sprang_webrtc, 2015-09-08 12:11:01]

The CQ bit was checked by sprang@webrtc.org

[sprang_webrtc, 2015-09-08 12:11:01]

The patchset sent to the CQ was uploaded after l-g-t-m from stefan@webrtc.org
Link to the patchset: https://codereview.webrtc.org/1290813008/#ps80001 (title:
"Rebase")

[sprang_webrtc, 2015-09-08 12:11:05]

https://codereview.webrtc.org/1290813008/diff/40001/webrtc/modules/remote_bit...
File webrtc/modules/remote_bitrate_estimator/remote_estimator_proxy.cc (right):

https://codereview.webrtc.org/1290813008/diff/40001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/remote_estimator_proxy.cc:142: // If we
can't even add the first seq to the feedback pack, we won't be
On 2015/09/07 11:18:53, stefan-webrtc (holmer) wrote:
> feedback packet

Done.

[commit-bot: I haz the power, 2015-09-08 12:11:12]

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

[commit-bot: I haz the power, 2015-09-08 12:21:21]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-09-08 12:21:22]

Try jobs failed on following builders:
  win_x64_rel on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/win_x64_rel/builds/9253)

[sprang_webrtc, 2015-09-08 13:06:29]

The CQ bit was checked by sprang@webrtc.org

[sprang_webrtc, 2015-09-08 13:06:30]

The patchset sent to the CQ was uploaded after l-g-t-m from stefan@webrtc.org
Link to the patchset: https://codereview.webrtc.org/1290813008/#ps100001 (title:
"Fixed borked Rebase")

[commit-bot: I haz the power, 2015-09-08 13:06:37]

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

[commit-bot: I haz the power, 2015-09-08 13:38:23]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-09-08 13:38:24]

Try jobs failed on following builders:
  linux_msan on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/linux_msan/builds/4582)

[sprang_webrtc, 2015-09-08 19:40:53]

The CQ bit was checked by sprang@webrtc.org

[commit-bot: I haz the power, 2015-09-08 19:41:05]

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

[commit-bot: I haz the power, 2015-09-08 20:25:20]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2015-09-08 20:25:33]

Patchset 6 (id:??) landed as
https://crrev.com/233bd87d45bbeeec50d7687e7d98c1cfc7f65562
Cr-Commit-Position: refs/heads/master@{#9898}