Add TransportFeedback adapter, adapting remote feedback to bwe estiamtor

Add TransportFeedback adapter, adapting remote feedback to bwe estiamtor

When using send-side bandwidth estimation, the inter-packet delay is
reported back to the sender using RTCP TransportFeedback messages.
Theis data needs to be translated into a format which the bandwidth
estimator (now instantiated on the send side) can use, including looking
up the local absolute send time from the send time history.

BUG=webrtc:4173
Committed: https://crrev.com/5e023eb337eed9746ecea7fc6fbb0fca386f1961
Cr-Commit-Position: refs/heads/master@{#9929}

[sprang_webrtc, 2015-09-08 14:32:20]

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

[sprang_webrtc, 2015-09-08 14:32:20]

Next to last cl for send-side bwe. This ones adds an adapter for the send side.
Next CL will wire it all up and add full-stack tests.

[stefan-webrtc, 2015-09-10 11:10:02]

First pass, will have to take a closer look at the unittests.

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

https://codereview.webrtc.org/1329083005/diff/1/webrtc/modules/remote_bitrate...
webrtc/modules/remote_bitrate_estimator/transport_feedback_adapter.cc:23: const
int64_t kNoTimestamp = std::numeric_limits<int64_t>::min();
Doesn't -1 work?

https://codereview.webrtc.org/1329083005/diff/1/webrtc/modules/remote_bitrate...
webrtc/modules/remote_bitrate_estimator/transport_feedback_adapter.cc:69: if
(labs(delta - kBaseTimestampRangeSizeUs) < labs(delta)) {
std::abs<int64_t> or something like that instead?

https://codereview.webrtc.org/1329083005/diff/1/webrtc/modules/remote_bitrate...
webrtc/modules/remote_bitrate_estimator/transport_feedback_adapter.cc:69: if
(labs(delta - kBaseTimestampRangeSizeUs) < labs(delta)) {
Comment on what lines 69-73 are doing.

https://codereview.webrtc.org/1329083005/diff/1/webrtc/modules/remote_bitrate...
webrtc/modules/remote_bitrate_estimator/transport_feedback_adapter.cc:93:
int64_t ms_timestamp = current_offset_ms_ + (offset_us / 1000);
timestamp_ms

https://codereview.webrtc.org/1329083005/diff/1/webrtc/video_engine/vie_chann...
File webrtc/video_engine/vie_channel_group.cc (right):

https://codereview.webrtc.org/1329083005/diff/1/webrtc/video_engine/vie_chann...
webrtc/video_engine/vie_channel_group.cc:239:
bitrate_controller_->CreateRtcpBandwidthObserver(), nullptr,
This on purpose not wired up yet?

[stefan-webrtc, 2015-09-11 08:15:12]

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

https://codereview.webrtc.org/1329083005/diff/1/webrtc/modules/remote_bitrate...
webrtc/modules/remote_bitrate_estimator/transport_feedback_adapter_unittest.cc:86:
truth[0].arrival_time_ms - input[0].arrival_time_ms;
Just from reading this code I don't fully understand what this method is
checking. Naively, I would expect the delta here to be zero if things are
working well. Then you are adding this delta to input[i] and comparing it to
truth[i] at lines 88-89. Could you explain with a comment maybe?

https://codereview.webrtc.org/1329083005/diff/1/webrtc/modules/remote_bitrate...
webrtc/modules/remote_bitrate_estimator/transport_feedback_adapter_unittest.cc:99:
MockRemoteBitrateEstimator* bitrate_estimator_;
Do you have to allocate it on the heap? Would save you a lot of * in front of
bitrate_estimator_ in the tests.

https://codereview.webrtc.org/1329083005/diff/1/webrtc/modules/remote_bitrate...
webrtc/modules/remote_bitrate_estimator/transport_feedback_adapter_unittest.cc:115:
packet.payload_size, packet.was_paced);
Makes me wonder why we don't pass in a PacketInfo object into this method :)

https://codereview.webrtc.org/1329083005/diff/1/webrtc/modules/remote_bitrate...
webrtc/modules/remote_bitrate_estimator/transport_feedback_adapter_unittest.cc:220:

Should we have a test for the limit on time deltas as well?

[sprang_webrtc, 2015-09-11 12:33:48]

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

https://codereview.webrtc.org/1329083005/diff/1/webrtc/modules/remote_bitrate...
webrtc/modules/remote_bitrate_estimator/transport_feedback_adapter.cc:23: const
int64_t kNoTimestamp = std::numeric_limits<int64_t>::min();
On 2015/09/10 11:10:01, stefan-webrtc (holmer) wrote:
> Doesn't -1 work?

Yes. Initially I allowed slightly negative timestamps, but have since changed
that...

https://codereview.webrtc.org/1329083005/diff/1/webrtc/modules/remote_bitrate...
webrtc/modules/remote_bitrate_estimator/transport_feedback_adapter.cc:69: if
(labs(delta - kBaseTimestampRangeSizeUs) < labs(delta)) {
On 2015/09/10 11:10:01, stefan-webrtc (holmer) wrote:
> std::abs<int64_t> or something like that instead?

Done.

https://codereview.webrtc.org/1329083005/diff/1/webrtc/modules/remote_bitrate...
webrtc/modules/remote_bitrate_estimator/transport_feedback_adapter.cc:69: if
(labs(delta - kBaseTimestampRangeSizeUs) < labs(delta)) {
On 2015/09/10 11:10:01, stefan-webrtc (holmer) wrote:
> Comment on what lines 69-73 are doing.

Done.

https://codereview.webrtc.org/1329083005/diff/1/webrtc/modules/remote_bitrate...
webrtc/modules/remote_bitrate_estimator/transport_feedback_adapter.cc:93:
int64_t ms_timestamp = current_offset_ms_ + (offset_us / 1000);
On 2015/09/10 11:10:01, stefan-webrtc (holmer) wrote:
> timestamp_ms

Done.

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

https://codereview.webrtc.org/1329083005/diff/1/webrtc/modules/remote_bitrate...
webrtc/modules/remote_bitrate_estimator/transport_feedback_adapter_unittest.cc:86:
truth[0].arrival_time_ms - input[0].arrival_time_ms;
On 2015/09/11 08:15:12, stefan-webrtc (holmer) wrote:
> Just from reading this code I don't fully understand what this method is
> checking. Naively, I would expect the delta here to be zero if things are
> working well. Then you are adding this delta to input[i] and comparing it to
> truth[i] at lines 88-89. Could you explain with a comment maybe?

Done.

https://codereview.webrtc.org/1329083005/diff/1/webrtc/modules/remote_bitrate...
webrtc/modules/remote_bitrate_estimator/transport_feedback_adapter_unittest.cc:99:
MockRemoteBitrateEstimator* bitrate_estimator_;
On 2015/09/11 08:15:12, stefan-webrtc (holmer) wrote:
> Do you have to allocate it on the heap? Would save you a lot of * in front of
> bitrate_estimator_ in the tests.

Yes, the instance will be owned by TransportFeedbackAdapter...

https://codereview.webrtc.org/1329083005/diff/1/webrtc/modules/remote_bitrate...
webrtc/modules/remote_bitrate_estimator/transport_feedback_adapter_unittest.cc:115:
packet.payload_size, packet.was_paced);
On 2015/09/11 08:15:12, stefan-webrtc (holmer) wrote:
> Makes me wonder why we don't pass in a PacketInfo object into this method :)

Done.

https://codereview.webrtc.org/1329083005/diff/1/webrtc/modules/remote_bitrate...
webrtc/modules/remote_bitrate_estimator/transport_feedback_adapter_unittest.cc:220:

On 2015/09/11 08:15:12, stefan-webrtc (holmer) wrote:
> Should we have a test for the limit on time deltas as well?

Done.

https://codereview.webrtc.org/1329083005/diff/1/webrtc/video_engine/vie_chann...
File webrtc/video_engine/vie_channel_group.cc (right):

https://codereview.webrtc.org/1329083005/diff/1/webrtc/video_engine/vie_chann...
webrtc/video_engine/vie_channel_group.cc:239:
bitrate_controller_->CreateRtcpBandwidthObserver(), nullptr,
On 2015/09/10 11:10:02, stefan-webrtc (holmer) wrote:
> This on purpose not wired up yet?

Yep. Will be in the follow-up cl.

[stefan-webrtc, 2015-09-14 08:48:51]

LG, but could you please further clarify the test code?

https://codereview.webrtc.org/1329083005/diff/20001/webrtc/modules/remote_bit...
File
webrtc/modules/remote_bitrate_estimator/transport_feedback_adapter_unittest.cc
(right):

https://codereview.webrtc.org/1329083005/diff/20001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/transport_feedback_adapter_unittest.cc:87:
// difference when comparing.
This is still a bit weird to me. Both seem to be arrival timestamps, so what
difference are we talking about? The difference between two succeeding arrivals
or the difference between an arrival timestamp and a send timestamp for the same
packet? I assume the latter?

[sprang_webrtc, 2015-09-14 09:03:01]

https://codereview.webrtc.org/1329083005/diff/20001/webrtc/modules/remote_bit...
File
webrtc/modules/remote_bitrate_estimator/transport_feedback_adapter_unittest.cc
(right):

https://codereview.webrtc.org/1329083005/diff/20001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/transport_feedback_adapter_unittest.cc:87:
// difference when comparing.
On 2015/09/14 08:48:50, stefan-webrtc (holmer) wrote:
> This is still a bit weird to me. Both seem to be arrival timestamps, so what
> difference are we talking about? The difference between two succeeding
arrivals
> or the difference between an arrival timestamp and a send timestamp for the
same
> packet? I assume the latter?

truth[i].arrival_time_ms is the timestamp given as input to the transport
feedback rtcp packets. These may be subject to loss of precision in the encoding
(not a problem right now so we're using ms in this test).
Mainly though, the difference is caused by the offset added by
TransportFeedbackAdapter::OnTransportFeedback. At the first received feedback
packet it will calibrate the base time to be current time ms. We could of course
get around this by setting the fake clock to the timestamp of the first packet
and update the offset time to also add the first delta of the first feedback
packet...

Not sure if it is worth it?

[stefan-webrtc, 2015-09-14 09:18:54]

lgtm

https://codereview.webrtc.org/1329083005/diff/20001/webrtc/modules/remote_bit...
File
webrtc/modules/remote_bitrate_estimator/transport_feedback_adapter_unittest.cc
(right):

https://codereview.webrtc.org/1329083005/diff/20001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/transport_feedback_adapter_unittest.cc:87:
// difference when comparing.
On 2015/09/14 09:03:01, språng wrote:
> On 2015/09/14 08:48:50, stefan-webrtc (holmer) wrote:
> > This is still a bit weird to me. Both seem to be arrival timestamps, so what
> > difference are we talking about? The difference between two succeeding
> arrivals
> > or the difference between an arrival timestamp and a send timestamp for the
> same
> > packet? I assume the latter?
> 
> truth[i].arrival_time_ms is the timestamp given as input to the transport
> feedback rtcp packets. These may be subject to loss of precision in the
encoding
> (not a problem right now so we're using ms in this test).
> Mainly though, the difference is caused by the offset added by
> TransportFeedbackAdapter::OnTransportFeedback. At the first received feedback
> packet it will calibrate the base time to be current time ms. We could of
course
> get around this by setting the fake clock to the timestamp of the first packet
> and update the offset time to also add the first delta of the first feedback
> packet...
> 
> Not sure if it is worth it?

Ah, ok. Maybe you can just comment on that it's correcting for that offset? That
wasn't clear to me.

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

The CQ bit was checked by sprang@webrtc.org

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

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/1329083005/#ps40001 (title:
"Extended comment, rebase")

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

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

[commit-bot: I haz the power, 2015-09-14 11:11:24]

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

[commit-bot: I haz the power, 2015-09-14 11:11:25]

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

[sprang_webrtc, 2015-09-14 12:28:42]

The CQ bit was checked by sprang@webrtc.org

[sprang_webrtc, 2015-09-14 12:28:42]

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/1329083005/#ps60001 (title:
"Bad merge, test issue")

[commit-bot: I haz the power, 2015-09-14 12:28:48]

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

[sprang_webrtc, 2015-09-14 13:26:03]

https://codereview.webrtc.org/1329083005/diff/20001/webrtc/modules/remote_bit...
File
webrtc/modules/remote_bitrate_estimator/transport_feedback_adapter_unittest.cc
(right):

https://codereview.webrtc.org/1329083005/diff/20001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/transport_feedback_adapter_unittest.cc:87:
// difference when comparing.
On 2015/09/14 09:18:54, stefan-webrtc (holmer) wrote:
> On 2015/09/14 09:03:01, språng wrote:
> > On 2015/09/14 08:48:50, stefan-webrtc (holmer) wrote:
> > > This is still a bit weird to me. Both seem to be arrival timestamps, so
what
> > > difference are we talking about? The difference between two succeeding
> > arrivals
> > > or the difference between an arrival timestamp and a send timestamp for
the
> > same
> > > packet? I assume the latter?
> > 
> > truth[i].arrival_time_ms is the timestamp given as input to the transport
> > feedback rtcp packets. These may be subject to loss of precision in the
> encoding
> > (not a problem right now so we're using ms in this test).
> > Mainly though, the difference is caused by the offset added by
> > TransportFeedbackAdapter::OnTransportFeedback. At the first received
feedback
> > packet it will calibrate the base time to be current time ms. We could of
> course
> > get around this by setting the fake clock to the timestamp of the first
packet
> > and update the offset time to also add the first delta of the first feedback
> > packet...
> > 
> > Not sure if it is worth it?
> 
> Ah, ok. Maybe you can just comment on that it's correcting for that offset?
That
> wasn't clear to me.

Done.

[commit-bot: I haz the power, 2015-09-14 13:42:48]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-09-14 13:42:57]

Patchset 4 (id:??) landed as
https://crrev.com/5e023eb337eed9746ecea7fc6fbb0fca386f1961
Cr-Commit-Position: refs/heads/master@{#9929}

[kjellander_webrtc, 2016-09-29 05:15:50]

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

[kjellander_webrtc, 2016-09-29 05:15:51]

Added comment regarding log spam

https://codereview.webrtc.org/1329083005/diff/60001/webrtc/modules/remote_bit...
File webrtc/modules/remote_bitrate_estimator/transport_feedback_adapter.cc
(right):

https://codereview.webrtc.org/1329083005/diff/60001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/transport_feedback_adapter.cc:105:
LOG(LS_WARNING) << "Failed to lookup send time for " << failed_lookups
This warning can spam the logs a lot during a call. Can you implement some of
kind of throttling for it?