Refactored incoming bitrate estimator.

Refactored incoming bitrate estimator.

BUG=webrtc:7746
Review-Url: https://codereview.webrtc.org/2917873002
Cr-Commit-Position: refs/heads/master@{#18478}
Committed: https://chromium.googlesource.com/external/webrtc/+/5fc8bf8b87495c225f5060760fa9d967b8dc9f9e

[tschumi, 2017-06-01 13:47:44]

Description was changed from

==========
Refactored incoming bitrate estimator.

BUG=webrtc:7746
==========

to

==========
Refactored incoming bitrate estimator.

BUG=webrtc:7746
==========

[tschumi, 2017-06-01 13:47:44]

tschumim@webrtc.org changed reviewers:
+ philipel@webrtc.org, terelius@webrtc.org

[tschumi, 2017-06-01 13:49:14]

Hi,
Here the prefix refactoring cl.

[terelius, 2017-06-01 15:15:04]

https://codereview.webrtc.org/2917873002/diff/1/webrtc/modules/congestion_con...
File webrtc/modules/congestion_controller/incoming_bitrate_estimator.cc (right):

https://codereview.webrtc.org/2917873002/diff/1/webrtc/modules/congestion_con...
webrtc/modules/congestion_controller/incoming_bitrate_estimator.cc:27: void
IncomingBitrateEstimator::Update(int64_t now_ms, int bytes) {
Has any part of this function changed?

https://codereview.webrtc.org/2917873002/diff/1/webrtc/modules/congestion_con...
webrtc/modules/congestion_controller/incoming_bitrate_estimator.cc:61: // Reset
if time moves backwards.
Has any part of this function changed?

https://codereview.webrtc.org/2917873002/diff/1/webrtc/modules/congestion_con...
File webrtc/modules/congestion_controller/incoming_bitrate_estimator.h (right):

https://codereview.webrtc.org/2917873002/diff/1/webrtc/modules/congestion_con...
webrtc/modules/congestion_controller/incoming_bitrate_estimator.h:23: class
IncomingBitrateEstimator {
I'd prefer not calling it incoming bitrate, since it isn't related to the our
incoming bitrate. I think the "incoming" name goes back to when bandwidth
estimation was done on the receive side.

Maybe AcknowledgedBitrateEstimator?

https://codereview.webrtc.org/2917873002/diff/1/webrtc/modules/congestion_con...
File webrtc/modules/congestion_controller/send_side_congestion_controller.cc
(right):

https://codereview.webrtc.org/2917873002/diff/1/webrtc/modules/congestion_con...
webrtc/modules/congestion_controller/send_side_congestion_controller.cc:153:
event_log_, incoming_bitrate_estimator_.get(), clock_));
Could we pass the actual bitrate estimate to the delay based detector instead of
passing a reference/pointer to the object? I think it would be nice to decouple
the objects instead of having implicit lifetime assumptions.

[tschumi, 2017-06-02 11:48:44]

Hi,
I decided not to move out the loop to send_side_congestion_controller. The CL
would have grow to much.

https://codereview.webrtc.org/2917873002/diff/1/webrtc/modules/congestion_con...
File webrtc/modules/congestion_controller/incoming_bitrate_estimator.cc (right):

https://codereview.webrtc.org/2917873002/diff/1/webrtc/modules/congestion_con...
webrtc/modules/congestion_controller/incoming_bitrate_estimator.cc:27: void
IncomingBitrateEstimator::Update(int64_t now_ms, int bytes) {
On 2017/06/01 15:15:04, terelius wrote:
> Has any part of this function changed?

Nope.
That is just a pure copy.

https://codereview.webrtc.org/2917873002/diff/1/webrtc/modules/congestion_con...
webrtc/modules/congestion_controller/incoming_bitrate_estimator.cc:61: // Reset
if time moves backwards.
On 2017/06/01 15:15:04, terelius wrote:
> Has any part of this function changed?

Nope.
That is just a pure copy.

https://codereview.webrtc.org/2917873002/diff/1/webrtc/modules/congestion_con...
File webrtc/modules/congestion_controller/incoming_bitrate_estimator.h (right):

https://codereview.webrtc.org/2917873002/diff/1/webrtc/modules/congestion_con...
webrtc/modules/congestion_controller/incoming_bitrate_estimator.h:23: class
IncomingBitrateEstimator {
On 2017/06/01 15:15:04, terelius wrote:
> I'd prefer not calling it incoming bitrate, since it isn't related to the our
> incoming bitrate. I think the "incoming" name goes back to when bandwidth
> estimation was done on the receive side.
> 
> Maybe AcknowledgedBitrateEstimator?

Acknowledged.

https://codereview.webrtc.org/2917873002/diff/1/webrtc/modules/congestion_con...
File webrtc/modules/congestion_controller/send_side_congestion_controller.cc
(right):

https://codereview.webrtc.org/2917873002/diff/1/webrtc/modules/congestion_con...
webrtc/modules/congestion_controller/send_side_congestion_controller.cc:153:
event_log_, incoming_bitrate_estimator_.get(), clock_));
Changed according offline discussions.

[tschumi, 2017-06-02 12:08:04]

https://codereview.webrtc.org/2917873002/diff/40001/webrtc/modules/congestion...
File webrtc/modules/congestion_controller/send_side_congestion_controller.cc
(right):

https://codereview.webrtc.org/2917873002/diff/40001/webrtc/modules/congestion...
webrtc/modules/congestion_controller/send_side_congestion_controller.cc:182:
acknowledged_bitrate_estimator_.reset(
Should we write a test for this ?

[terelius, 2017-06-02 12:14:06]

https://codereview.webrtc.org/2917873002/diff/20001/webrtc/modules/congestion...
File webrtc/modules/congestion_controller/delay_based_bwe_unittest.cc (right):

https://codereview.webrtc.org/2917873002/diff/20001/webrtc/modules/congestion...
webrtc/modules/congestion_controller/delay_based_bwe_unittest.cc:31:
rtc::Optional<uint32_t>());
Do we have test coverage for the "typical" case where we actually do have an
acknowledged bitrate?

https://codereview.webrtc.org/2917873002/diff/20001/webrtc/modules/congestion...
File
webrtc/modules/congestion_controller/include/send_side_congestion_controller.h
(right):

https://codereview.webrtc.org/2917873002/diff/20001/webrtc/modules/congestion...
webrtc/modules/congestion_controller/include/send_side_congestion_controller.h:56:
int64_t probinbweg_interval_ms) = 0;
Please fix :)

https://codereview.webrtc.org/2917873002/diff/20001/webrtc/modules/remote_bit...
File webrtc/modules/remote_bitrate_estimator/test/estimators/send_side.cc
(right):

https://codereview.webrtc.org/2917873002/diff/20001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/test/estimators/send_side.cc:78:
acknowledged_bitrate_estimator_->IncomingPacketFeedbackVector(
Should we sort the packet vectors here as well? I don't think we have any
packet_processor in the current simulations that reorder packets, but if we did
we might run into trouble here. Maybe it would be sufficient to DCHECK that the
vector is sorted?

https://codereview.webrtc.org/2917873002/diff/40001/webrtc/modules/congestion...
File webrtc/modules/congestion_controller/send_side_congestion_controller.cc
(right):

https://codereview.webrtc.org/2917873002/diff/40001/webrtc/modules/congestion...
webrtc/modules/congestion_controller/send_side_congestion_controller.cc:183:
rtc::MakeUnique<AcknowledgedBitrateEstimator>()),
Should this be a semicolon?

[philipel, 2017-06-02 12:32:41]

https://codereview.webrtc.org/2917873002/diff/20001/webrtc/modules/congestion...
File webrtc/modules/congestion_controller/acknowledge_bitrate_estimator.cc
(right):

https://codereview.webrtc.org/2917873002/diff/20001/webrtc/modules/congestion...
webrtc/modules/congestion_controller/acknowledge_bitrate_estimator.cc:29: for
(const auto& packet : packet_feedback_vector) {
remove {}

https://codereview.webrtc.org/2917873002/diff/20001/webrtc/modules/congestion...
File webrtc/modules/congestion_controller/acknowledge_bitrate_estimator.h
(right):

https://codereview.webrtc.org/2917873002/diff/20001/webrtc/modules/congestion...
webrtc/modules/congestion_controller/acknowledge_bitrate_estimator.h:30: const
std::vector<PacketFeedback>& packet_feedback_vector);
Isn't it possible to forward declare PacketFeedback and move the include to the
.cc file?

https://codereview.webrtc.org/2917873002/diff/20001/webrtc/modules/congestion...
File webrtc/modules/congestion_controller/send_side_congestion_controller.cc
(right):

https://codereview.webrtc.org/2917873002/diff/20001/webrtc/modules/congestion...
webrtc/modules/congestion_controller/send_side_congestion_controller.cc:63:
return packet_feedback.arrival_time_ms !=
How do we get feedback that does not have the arrival time set? It feels like a
thing that should never happen.

[tschumi, 2017-06-02 13:13:05]

https://codereview.webrtc.org/2917873002/diff/20001/webrtc/modules/congestion...
File webrtc/modules/congestion_controller/acknowledge_bitrate_estimator.cc
(right):

https://codereview.webrtc.org/2917873002/diff/20001/webrtc/modules/congestion...
webrtc/modules/congestion_controller/acknowledge_bitrate_estimator.cc:29: for
(const auto& packet : packet_feedback_vector) {
On 2017/06/02 12:32:40, philipel wrote:
> remove {}

Done.

https://codereview.webrtc.org/2917873002/diff/20001/webrtc/modules/congestion...
File webrtc/modules/congestion_controller/acknowledge_bitrate_estimator.h
(right):

https://codereview.webrtc.org/2917873002/diff/20001/webrtc/modules/congestion...
webrtc/modules/congestion_controller/acknowledge_bitrate_estimator.h:30: const
std::vector<PacketFeedback>& packet_feedback_vector);
On 2017/06/02 12:32:40, philipel wrote:
> Isn't it possible to forward declare PacketFeedback and move the include to
the
> .cc file?

Sure.
Done

https://codereview.webrtc.org/2917873002/diff/20001/webrtc/modules/congestion...
File webrtc/modules/congestion_controller/delay_based_bwe_unittest.cc (right):

https://codereview.webrtc.org/2917873002/diff/20001/webrtc/modules/congestion...
webrtc/modules/congestion_controller/delay_based_bwe_unittest.cc:31:
rtc::Optional<uint32_t>());
On 2017/06/02 12:14:05, terelius wrote:
> Do we have test coverage for the "typical" case where we actually do have an
> acknowledged bitrate?

Yes its done with the help of delay_based_bwe_unittest_helper

https://codereview.webrtc.org/2917873002/diff/20001/webrtc/modules/congestion...
File
webrtc/modules/congestion_controller/include/send_side_congestion_controller.h
(right):

https://codereview.webrtc.org/2917873002/diff/20001/webrtc/modules/congestion...
webrtc/modules/congestion_controller/include/send_side_congestion_controller.h:56:
int64_t probinbweg_interval_ms) = 0;
On 2017/06/02 12:14:05, terelius wrote:
> Please fix :)

Done.

https://codereview.webrtc.org/2917873002/diff/20001/webrtc/modules/congestion...
File webrtc/modules/congestion_controller/send_side_congestion_controller.cc
(right):

https://codereview.webrtc.org/2917873002/diff/20001/webrtc/modules/congestion...
webrtc/modules/congestion_controller/send_side_congestion_controller.cc:63:
return packet_feedback.arrival_time_ms !=
On 2017/06/02 12:32:40, philipel wrote:
> How do we get feedback that does not have the arrival time set? It feels like
a
> thing that should never happen.

If the packet was not lost, we would receive this. This logic is just copied
form delay_based_bwe.

https://codereview.webrtc.org/2917873002/diff/20001/webrtc/modules/remote_bit...
File webrtc/modules/remote_bitrate_estimator/test/estimators/send_side.cc
(right):

https://codereview.webrtc.org/2917873002/diff/20001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/test/estimators/send_side.cc:78:
acknowledged_bitrate_estimator_->IncomingPacketFeedbackVector(
Yea a DCHECK is a great idea, will have to move the PacketFeedbackComparator for
this.

https://codereview.webrtc.org/2917873002/diff/40001/webrtc/modules/congestion...
File webrtc/modules/congestion_controller/send_side_congestion_controller.cc
(right):

https://codereview.webrtc.org/2917873002/diff/40001/webrtc/modules/congestion...
webrtc/modules/congestion_controller/send_side_congestion_controller.cc:183:
rtc::MakeUnique<AcknowledgedBitrateEstimator>()),
Yes

[terelius, 2017-06-05 14:52:51]

lgtm with one minor update:

https://codereview.webrtc.org/2917873002/diff/80001/webrtc/modules/rtp_rtcp/i...
File webrtc/modules/rtp_rtcp/include/rtp_rtcp_defines.h (right):

https://codereview.webrtc.org/2917873002/diff/80001/webrtc/modules/rtp_rtcp/i...
webrtc/modules/rtp_rtcp/include/rtp_rtcp_defines.h:350: class
PacketFeedbackComparator {
Could this be reused in analyzer.cc too?

[philipel, 2017-06-05 15:07:07]

lgtm

[tschumi, 2017-06-05 15:13:54]

https://codereview.webrtc.org/2917873002/diff/80001/webrtc/modules/rtp_rtcp/i...
File webrtc/modules/rtp_rtcp/include/rtp_rtcp_defines.h (right):

https://codereview.webrtc.org/2917873002/diff/80001/webrtc/modules/rtp_rtcp/i...
webrtc/modules/rtp_rtcp/include/rtp_rtcp_defines.h:350: class
PacketFeedbackComparator {
On 2017/06/05 14:52:51, terelius wrote:
> Could this be reused in analyzer.cc too?

Done.

[tschumi, 2017-06-05 15:23:48]

The CQ bit was checked by tschumim@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-05 15:23:54]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2017-06-05 15:28:41]

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

[commit-bot: I haz the power, 2017-06-05 15:28:42]

Dry run: Try jobs failed on following builders:
  ios32_sim_ios9_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios32_sim_ios9_dbg/buil...)

[tschumi, 2017-06-07 06:27:33]

The CQ bit was checked by tschumim@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-07 06:27:39]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2017-06-07 06:59:23]

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

[commit-bot: I haz the power, 2017-06-07 06:59:24]

Dry run: This issue passed the CQ dry run.

[tschumi, 2017-06-07 07:01:30]

The CQ bit was checked by tschumim@webrtc.org

[tschumi, 2017-06-07 07:01:31]

The patchset sent to the CQ was uploaded after l-g-t-m from philipel@webrtc.org,
terelius@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2917873002/#ps120001 (title:
"Fix for iso build")

[commit-bot: I haz the power, 2017-06-07 07:01:40]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2017-06-07 07:05:40]

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

[commit-bot: I haz the power, 2017-06-07 07:05:41]

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

[tschumi, 2017-06-07 07:20:54]

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

[tschumi, 2017-06-07 07:22:17]

Hi Stefan,
I need a owner approval for this cl, because of the change in
rtp_rtcp_defines.h. 
Pleas take a look.

[stefan-webrtc, 2017-06-07 09:56:32]

https://codereview.webrtc.org/2917873002/diff/120001/webrtc/modules/congestio...
File webrtc/modules/congestion_controller/acknowledge_bitrate_estimator.h
(right):

https://codereview.webrtc.org/2917873002/diff/120001/webrtc/modules/congestio...
webrtc/modules/congestion_controller/acknowledge_bitrate_estimator.h:30: void
IncomingPacketFeedbackVector(
Empty line above to have ctors/dtors separated from regular methods. Instead
feel free to group this method with bitrate_bps().

https://codereview.webrtc.org/2917873002/diff/120001/webrtc/modules/congestio...
webrtc/modules/congestion_controller/acknowledge_bitrate_estimator.h:38: int
sum_;
Empty line above to separate methods and members.

https://codereview.webrtc.org/2917873002/diff/120001/webrtc/modules/congestio...
File webrtc/modules/congestion_controller/send_side_congestion_controller.cc
(right):

https://codereview.webrtc.org/2917873002/diff/120001/webrtc/modules/congestio...
webrtc/modules/congestion_controller/send_side_congestion_controller.cc:56:
return sorted_packet_feedback_vector;
If this is already sorted here, why do we sort it again in
SortPacketFeedbackVector? Or is this actually not sorting, and we should rename
this variable?

[tschumi, 2017-06-07 15:26:19]

https://codereview.webrtc.org/2917873002/diff/120001/webrtc/modules/congestio...
File webrtc/modules/congestion_controller/acknowledge_bitrate_estimator.h
(right):

https://codereview.webrtc.org/2917873002/diff/120001/webrtc/modules/congestio...
webrtc/modules/congestion_controller/acknowledge_bitrate_estimator.h:30: void
IncomingPacketFeedbackVector(
On 2017/06/07 09:56:31, stefan-webrtc wrote:
> Empty line above to have ctors/dtors separated from regular methods. Instead
> feel free to group this method with bitrate_bps().

Done.

https://codereview.webrtc.org/2917873002/diff/120001/webrtc/modules/congestio...
webrtc/modules/congestion_controller/acknowledge_bitrate_estimator.h:38: int
sum_;
On 2017/06/07 09:56:31, stefan-webrtc wrote:
> Empty line above to separate methods and members.

Done.

https://codereview.webrtc.org/2917873002/diff/120001/webrtc/modules/congestio...
File webrtc/modules/congestion_controller/send_side_congestion_controller.cc
(right):

https://codereview.webrtc.org/2917873002/diff/120001/webrtc/modules/congestio...
webrtc/modules/congestion_controller/send_side_congestion_controller.cc:56:
return sorted_packet_feedback_vector;
On 2017/06/07 09:56:31, stefan-webrtc wrote:
> If this is already sorted here, why do we sort it again in
> SortPacketFeedbackVector? Or is this actually not sorting, and we should
rename
> this variable?
You are right there is no sorting. Will renamed the vector.

[stefan-webrtc, 2017-06-07 15:27:18]

lgtm

[tschumi, 2017-06-07 15:49:40]

The CQ bit was checked by tschumim@webrtc.org

[tschumi, 2017-06-07 15:49:40]

The patchset sent to the CQ was uploaded after l-g-t-m from philipel@webrtc.org,
terelius@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2917873002/#ps140001 (title:
"Respond to comments")

[commit-bot: I haz the power, 2017-06-07 15:49:46]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2017-06-07 16:48:21]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1496850580325970,
"parent_rev": "20ebf4ede803cd4f628ef9378700f60b72f2eab0", "commit_rev":
"5fc8bf8b87495c225f5060760fa9d967b8dc9f9e"}

[commit-bot: I haz the power, 2017-06-07 16:48:25]

Description was changed from

==========
Refactored incoming bitrate estimator.

BUG=webrtc:7746
==========

to

==========
Refactored incoming bitrate estimator.

BUG=webrtc:7746

Review-Url: https://codereview.webrtc.org/2917873002
Cr-Commit-Position: refs/heads/master@{#18478}
Committed:
https://chromium.googlesource.com/external/webrtc/+/5fc8bf8b87495c225f5060760...
==========

[commit-bot: I haz the power, 2017-06-07 16:48:26]

Committed patchset #8 (id:140001) as
https://chromium.googlesource.com/external/webrtc/+/5fc8bf8b87495c225f5060760...

[tschumim, 2017-06-07 18:48:36]

A revert of this CL (patchset #8 id:140001) has been created in
https://codereview.webrtc.org/2924093002/ by tschumim@google.com.

The reason for reverting is: Caused failure in vice test.

[tschumi, 2017-06-08 07:10:12]

A revert of this CL (patchset #8 id:140001) has been created in
https://codereview.webrtc.org/2924243002/ by tschumim@webrtc.org.

The reason for reverting is: Breaks Vice tests.