Propagate probing cluster id to SendTimeHistory.

Propagate probing cluster id to SendTimeHistory, both for packets and padding.

BUG=webrtc:5859
Committed: https://crrev.com/5be28c848b91bc6e4800eac07a3f5ac09a32ad70
Cr-Commit-Position: refs/heads/master@{#12985}

[philipel, 2016-05-25 11:25:38]

philipel@webrtc.org changed reviewers:
+ danilchap@webrtc.org, stefan@webrtc.org

[danilchap, 2016-05-25 13:00:54]

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

https://codereview.webrtc.org/2005313003/diff/1/webrtc/modules/remote_bitrate...
webrtc/modules/remote_bitrate_estimator/remote_bitrate_estimator_abs_send_time.cc:214:
packet_info.probe_cluster_id);
IncomingPacketInfo here still takes (bool was_paced) as last parameter.

You definitely want to update RemoteBitrateEstimatorAbsSendTime more: replace
was_paced with probe_cluster_id.

Or at least convert it to was_paced correctly: 'packet_info.probe_cluster_id >=
0'

https://codereview.webrtc.org/2005313003/diff/1/webrtc/modules/rtp_rtcp/inclu...
File webrtc/modules/rtp_rtcp/include/rtp_rtcp_defines.h (right):

https://codereview.webrtc.org/2005313003/diff/1/webrtc/modules/rtp_rtcp/inclu...
webrtc/modules/rtp_rtcp/include/rtp_rtcp_defines.h:288: // Which probing cluster
this packet belongs to, -1 means no cluster.
may be something like '-1 means packet is not used for probing'
and/or add a constant like 'constexpr int kNotAProbe = -1;' and use that one
instead of mystical value -1. (this struct looks like a good place to hold this
constant)

https://codereview.webrtc.org/2005313003/diff/1/webrtc/modules/rtp_rtcp/sourc...
File webrtc/modules/rtp_rtcp/source/rtp_sender.cc (right):

https://codereview.webrtc.org/2005313003/diff/1/webrtc/modules/rtp_rtcp/sourc...
webrtc/modules/rtp_rtcp/source/rtp_sender.cc:1068: true);
AddPacket already take probe_cluster_id as last parameter, not bool.
may be change AddPacket signature so that old code wouldn't compile to help find
all places that should be changed.
(true look wrong here anyway: this is code path when pacer is absent)

[philipel, 2016-05-26 09:59:12]

New strategy: completely annihilate was_paced.

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

https://codereview.webrtc.org/2005313003/diff/1/webrtc/modules/remote_bitrate...
webrtc/modules/remote_bitrate_estimator/remote_bitrate_estimator_abs_send_time.cc:214:
packet_info.probe_cluster_id);
On 2016/05/25 13:00:54, danilchap wrote:
> IncomingPacketInfo here still takes (bool was_paced) as last parameter.
> 
> You definitely want to update RemoteBitrateEstimatorAbsSendTime more: replace
> was_paced with probe_cluster_id.
> 
> Or at least convert it to was_paced correctly: 'packet_info.probe_cluster_id
>=
> 0'

I have now annihilated was_paced.

https://codereview.webrtc.org/2005313003/diff/1/webrtc/modules/rtp_rtcp/inclu...
File webrtc/modules/rtp_rtcp/include/rtp_rtcp_defines.h (right):

https://codereview.webrtc.org/2005313003/diff/1/webrtc/modules/rtp_rtcp/inclu...
webrtc/modules/rtp_rtcp/include/rtp_rtcp_defines.h:288: // Which probing cluster
this packet belongs to, -1 means no cluster.
On 2016/05/25 13:00:54, danilchap wrote:
> may be something like '-1 means packet is not used for probing'
> and/or add a constant like 'constexpr int kNotAProbe = -1;' and use that one
> instead of mystical value -1. (this struct looks like a good place to hold
this
> constant)

Defined kNotAProbe in this class.

https://codereview.webrtc.org/2005313003/diff/1/webrtc/modules/rtp_rtcp/sourc...
File webrtc/modules/rtp_rtcp/source/rtp_sender.cc (right):

https://codereview.webrtc.org/2005313003/diff/1/webrtc/modules/rtp_rtcp/sourc...
webrtc/modules/rtp_rtcp/source/rtp_sender.cc:1068: true);
On 2016/05/25 13:00:54, danilchap wrote:
> AddPacket already take probe_cluster_id as last parameter, not bool.
> may be change AddPacket signature so that old code wouldn't compile to help
find
> all places that should be changed.
> (true look wrong here anyway: this is code path when pacer is absent)

The definition was already changed, but since bool is automatically casted to an
int it compiled fine. An ugly hack to find all the places where we call this
function still using a bool instead of an int is to declare the old function and
mark it with:

__attribute__((deprecated("Old Version!")))

:)

Also, changed from true to PacketInfo::kNotAProbe here.

[danilchap, 2016-05-26 11:22:42]

This CL starting to look big. May be split it into several steps, e.g.: 
1) Fixed bug in paced_sender (bitrate_prober actually) - small CL that will not
be blocked by reviewing other changes.
2) propagate cluster_id to SentHistory, but not further (i.e. do not touch BWE
and was_paced flag)

3) create follow-up CLS that would propagate and use cluster_id in BWE.

4) remove was_paced flag as no longer used (this change spans across multiply
modules so it would be easier to review if it is 'remove-only' CL).

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

https://codereview.webrtc.org/2005313003/diff/20001/webrtc/audio/audio_receiv...
webrtc/audio/audio_receive_stream.cc:238: header, false);
This looks strange (may be this packet is a probe: code here can't know that)
that I wonder if it is needed.
Looking through code got an impression last parameter (was_paced) could be
removed instead of replaced.
i.e. code here should probably look like that:
remote_bitrate_estimator_->IncomingPacket(arrival_time_ms, payload_size,
header);

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

https://codereview.webrtc.org/2005313003/diff/20001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/remote_bitrate_estimator_unittest_helper.cc:225:
int probe_cluster_id) {
May be also update calls to this function?
true is not a proper integer.
In fact, because it is was always called with was_paced=true, this parameter
likely is not needed.

https://codereview.webrtc.org/2005313003/diff/20001/webrtc/video/rtp_stream_r...
File webrtc/video/rtp_stream_receiver.cc (left):

https://codereview.webrtc.org/2005313003/diff/20001/webrtc/video/rtp_stream_r...
webrtc/video/rtp_stream_receiver.cc:312: header, true);
this change looks even stranger than similar code for incoming audio packet:
true (that means 'many be a probe') replaced with value that tells 'not a
probe'. Again, look like it could be simply removed.

[stefan-webrtc, 2016-05-26 16:29:50]

Since the RemoteBitrateEstimator is used both on the send-side and the
receive-side depending on configuration, and we can't know the probe cluster id
on the receiver, I see two options:

1. Fork the RemoteBitrateEstimatorAbsSendTime so that we have a separate
implementation for the send-side, and keep the receive-side version intact.

2. Break out the probe cluster detection from RemoteBitrateEstimator and use it
to give ids to the packets before they are passed in.

#2 seems pretty complicated, so I'd prefer #1. We will anyway want to fork it
later since we'll be doing other improvements which will be send-side only. I'd
suggest forking it in a separate CL though.

Also,

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

https://codereview.webrtc.org/2005313003/diff/20001/webrtc/modules/congestion...
webrtc/modules/congestion_controller/congestion_controller.cc:55:
probe_cluster_id);
Note that this code is running on the receive-side, so you won't know the
cluster id here.

https://codereview.webrtc.org/2005313003/diff/20001/webrtc/modules/pacing/bit...
File webrtc/modules/pacing/bitrate_prober.cc (right):

https://codereview.webrtc.org/2005313003/diff/20001/webrtc/modules/pacing/bit...
webrtc/modules/pacing/bitrate_prober.cc:158: probing_state_ = kWait;
Could you add a unittest which covers this issue?

https://codereview.webrtc.org/2005313003/diff/20001/webrtc/modules/pacing/pac...
File webrtc/modules/pacing/paced_sender.cc (right):

https://codereview.webrtc.org/2005313003/diff/20001/webrtc/modules/pacing/pac...
webrtc/modules/pacing/paced_sender.cc:395: is_probing ?
prober_->CurrentClusterId() : PacketInfo::kNotAProbe;
Any reason for doing this here instead of on line 429? Better to do it close to
when it's actually being used.

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

https://codereview.webrtc.org/2005313003/diff/20001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/remote_bitrate_estimator_abs_send_time.cc:271:
if (include_probe &&
Shouldn't we also make use of the fact that we know what cluster the probe
packet belongs to below? For instance, if we're sending two probes, one at 500
kbps and one at 1000 kbps, we want to measure the incoming bitrate of probe 1
separately from the bitrate of probe 2.

Or maybe this is still WIP and will be improved in a follow-up? In that case
state that in the CL description.

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

https://codereview.webrtc.org/2005313003/diff/20001/webrtc/modules/rtp_rtcp/i...
webrtc/modules/rtp_rtcp/include/rtp_rtcp_defines.h:306:
__attribute__((deprecated("Old Version!"))) {}
Use RTC_DEPRECATED.

https://codereview.webrtc.org/2005313003/diff/20001/webrtc/modules/rtp_rtcp/s...
File webrtc/modules/rtp_rtcp/source/rtp_sender.cc (left):

https://codereview.webrtc.org/2005313003/diff/20001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtp_sender.cc:680: true);
Was this a bug? Should it have been covered by a test?

https://codereview.webrtc.org/2005313003/diff/20001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtp_sender.cc:936: true);
Same here

https://codereview.webrtc.org/2005313003/diff/20001/webrtc/video/rtp_stream_r...
File webrtc/video/rtp_stream_receiver.cc (left):

https://codereview.webrtc.org/2005313003/diff/20001/webrtc/video/rtp_stream_r...
webrtc/video/rtp_stream_receiver.cc:312: header, true);
On 2016/05/26 11:22:42, danilchap wrote:
> this change looks even stranger than similar code for incoming audio packet:
> true (that means 'many be a probe') replaced with value that tells 'not a
> probe'. Again, look like it could be simply removed.

Yes, this looks wrong. If anything it should be assumed to always be a probe as
before. Not sure how that would work though since we don't know what probe
cluster it belongs to at the receiver.

[danilchap, 2016-05-31 08:58:24]

https://codereview.webrtc.org/2005313003/diff/20001/webrtc/modules/rtp_rtcp/s...
File webrtc/modules/rtp_rtcp/source/rtp_sender.cc (right):

https://codereview.webrtc.org/2005313003/diff/20001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtp_sender.cc:1011: bytes_sent +=
SendPadData(bytes - bytes_sent, false, 0, 0);
Padding can use cluster_id too.

[philipel, 2016-05-31 11:57:07]

Description was changed from

==========
Propagate probing cluster id to SendTimeHistory.

- Propagate probing cluster id to SendTimeHistory, both for packets and padding.
- Replaced |was_paced| with |probing_cluster_id| in PacketInfo.
- Fixed bug in paced_sender which caused padding to be sent after the prober
  changed state to kWait.

BUG=webrtc:5859
==========

to

==========
Propagate probing cluster id to SendTimeHistory, both for packets and padding.


BUG=webrtc:5859
==========

[philipel, 2016-05-31 11:58:10]

Right, the current strategy for this CL is to only save the probe_cluster_id in
the SendTimeHistory.

[danilchap, 2016-05-31 13:06:50]

functional change looks good, but simple propagation tests looks missing.

https://codereview.webrtc.org/2005313003/diff/40001/webrtc/modules/pacing/pac...
File webrtc/modules/pacing/paced_sender.cc (right):

https://codereview.webrtc.org/2005313003/diff/40001/webrtc/modules/pacing/pac...
webrtc/modules/pacing/paced_sender.cc:428:
SendPadding(static_cast<size_t>(padding_needed), probe_cluster_id);
since you are modifying this line anyway, remove static_cast from size_t to
size_t

https://codereview.webrtc.org/2005313003/diff/40001/webrtc/modules/pacing/pac...
File webrtc/modules/pacing/paced_sender_unittest.cc (right):

https://codereview.webrtc.org/2005313003/diff/40001/webrtc/modules/pacing/pac...
webrtc/modules/pacing/paced_sender_unittest.cc:873: TEST_F(PacedSenderTest,
DISABLED_ProbeClusterId) {
Add a comment why this test is disabled if it should be.
Does adding unused cluster_id breaks it?

https://codereview.webrtc.org/2005313003/diff/40001/webrtc/modules/pacing/pac...
File webrtc/modules/pacing/packet_router_unittest.cc (right):

https://codereview.webrtc.org/2005313003/diff/40001/webrtc/modules/pacing/pac...
webrtc/modules/pacing/packet_router_unittest.cc:51: retransmission,
PacketInfo::kNotAProbe))
may be instead of constant 'not a probe' use some local constant kClusterProbeId
= 7 (similar to kSsrc/timestamp/sequence_number values). Though this tests do
not use cluster_id value, they should work with any passed value.

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

https://codereview.webrtc.org/2005313003/diff/40001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/send_time_history_unittest.cc:39:
PacketInfo::kNotAProbe);
better propagate and test probe_cluster_id field too.

https://codereview.webrtc.org/2005313003/diff/40001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/send_time_history_unittest.cc:134:
sent_packets[i].was_paced, PacketInfo::kNotAProbe);
sent_packets[i].probe_cluster_id

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

https://codereview.webrtc.org/2005313003/diff/40001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/test/estimators/send_side.cc:97:
packet->paced(), PacketInfo::kNotAProbe);
This look like the place to test how cluster id affects send-side BWE.
Can you add at least add a TODO comment to extend this test suit to use
propagated cluster_id.

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

https://codereview.webrtc.org/2005313003/diff/40001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/transport_feedback_adapter_unittest.cc:96:
}
check probe_cluster_id too.

https://codereview.webrtc.org/2005313003/diff/40001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/transport_feedback_adapter_unittest.cc:103:
PacketInfo::kNotAProbe);
probably info.probe_cluster_id here.

https://codereview.webrtc.org/2005313003/diff/40001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/transport_feedback_adapter_unittest.cc:118:
PacketInfo(100, 200, 0, 1500, true, PacketInfo::kNotAProbe));
May be helpful to check probe_cluster_id here too, e.g. use '0' for 1st three
packets, '1' for last two.

https://codereview.webrtc.org/2005313003/diff/40001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/transport_feedback_adapter_unittest.cc:154:
PacketInfo(100, 200, 0, 1500, true, PacketInfo::kNotAProbe));
ditto

https://codereview.webrtc.org/2005313003/diff/40001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/transport_feedback_adapter_unittest.cc:204:
PacketInfo::kNotAProbe));
ditto

https://codereview.webrtc.org/2005313003/diff/40001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/transport_feedback_adapter_unittest.cc:242:
PacketInfo(120, 200, 0, 1500, true, PacketInfo::kNotAProbe));
ditto

[philipel, 2016-05-31 14:34:59]

https://codereview.webrtc.org/2005313003/diff/40001/webrtc/modules/pacing/pac...
File webrtc/modules/pacing/paced_sender.cc (right):

https://codereview.webrtc.org/2005313003/diff/40001/webrtc/modules/pacing/pac...
webrtc/modules/pacing/paced_sender.cc:428:
SendPadding(static_cast<size_t>(padding_needed), probe_cluster_id);
On 2016/05/31 13:06:49, danilchap wrote:
> since you are modifying this line anyway, remove static_cast from size_t to
> size_t

Done.

https://codereview.webrtc.org/2005313003/diff/40001/webrtc/modules/pacing/pac...
File webrtc/modules/pacing/paced_sender_unittest.cc (right):

https://codereview.webrtc.org/2005313003/diff/40001/webrtc/modules/pacing/pac...
webrtc/modules/pacing/paced_sender_unittest.cc:873: TEST_F(PacedSenderTest,
DISABLED_ProbeClusterId) {
On 2016/05/31 13:06:49, danilchap wrote:
> Add a comment why this test is disabled if it should be.
> Does adding unused cluster_id breaks it?

Adding unused cluster id does not break it, but this test pass for the wrong
reason. I will fix the test and the bug in a follow-up CL.

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

https://codereview.webrtc.org/2005313003/diff/40001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/send_time_history_unittest.cc:39:
PacketInfo::kNotAProbe);
On 2016/05/31 13:06:49, danilchap wrote:
> better propagate and test probe_cluster_id field too.

Integrated testing of cluster id into existing tests.

https://codereview.webrtc.org/2005313003/diff/40001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/send_time_history_unittest.cc:134:
sent_packets[i].was_paced, PacketInfo::kNotAProbe);
On 2016/05/31 13:06:50, danilchap wrote:
> sent_packets[i].probe_cluster_id

Done.

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

https://codereview.webrtc.org/2005313003/diff/40001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/test/estimators/send_side.cc:97:
packet->paced(), PacketInfo::kNotAProbe);
On 2016/05/31 13:06:50, danilchap wrote:
> This look like the place to test how cluster id affects send-side BWE.
> Can you add at least add a TODO comment to extend this test suit to use
> propagated cluster_id.

Added a TODO.

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

https://codereview.webrtc.org/2005313003/diff/40001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/transport_feedback_adapter_unittest.cc:96:
}
On 2016/05/31 13:06:50, danilchap wrote:
> check probe_cluster_id too.

Done.

https://codereview.webrtc.org/2005313003/diff/40001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/transport_feedback_adapter_unittest.cc:103:
PacketInfo::kNotAProbe);
On 2016/05/31 13:06:50, danilchap wrote:
> probably info.probe_cluster_id here.

Done.

https://codereview.webrtc.org/2005313003/diff/40001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/transport_feedback_adapter_unittest.cc:118:
PacketInfo(100, 200, 0, 1500, true, PacketInfo::kNotAProbe));
On 2016/05/31 13:06:50, danilchap wrote:
> May be helpful to check probe_cluster_id here too, e.g. use '0' for 1st three
> packets, '1' for last two.

Done.

https://codereview.webrtc.org/2005313003/diff/40001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/transport_feedback_adapter_unittest.cc:154:
PacketInfo(100, 200, 0, 1500, true, PacketInfo::kNotAProbe));
On 2016/05/31 13:06:50, danilchap wrote:
> ditto

Done.

https://codereview.webrtc.org/2005313003/diff/40001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/transport_feedback_adapter_unittest.cc:204:
PacketInfo::kNotAProbe));
On 2016/05/31 13:06:50, danilchap wrote:
> ditto

Test with kNotAProbe here.

https://codereview.webrtc.org/2005313003/diff/40001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/transport_feedback_adapter_unittest.cc:242:
PacketInfo(120, 200, 0, 1500, true, PacketInfo::kNotAProbe));
On 2016/05/31 13:06:50, danilchap wrote:
> ditto

Done.

[danilchap, 2016-05-31 15:32:15]

https://codereview.webrtc.org/2005313003/diff/60001/webrtc/modules/pacing/pac...
File webrtc/modules/pacing/packet_router_unittest.cc (right):

https://codereview.webrtc.org/2005313003/diff/60001/webrtc/modules/pacing/pac...
webrtc/modules/pacing/packet_router_unittest.cc:51: retransmission,
PacketInfo::kNotAProbe))
integrate testing of cluster_id propagation through packet router here: expect
and pass some other constant.

https://codereview.webrtc.org/2005313003/diff/60001/webrtc/modules/pacing/pac...
webrtc/modules/pacing/packet_router_unittest.cc:126: EXPECT_CALL(rtp_1,
TimeToSendPadding(requested_padding_bytes, _))
Same here.

https://codereview.webrtc.org/2005313003/diff/60001/webrtc/modules/pacing/pac...
webrtc/modules/pacing/packet_router_unittest.cc:131: rtp_2,
TimeToSendPadding(requested_padding_bytes - sent_padding_bytes, _))
and here

https://codereview.webrtc.org/2005313003/diff/60001/webrtc/modules/rtp_rtcp/s...
File webrtc/modules/rtp_rtcp/source/rtp_sender_unittest.cc (right):

https://codereview.webrtc.org/2005313003/diff/60001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtp_sender_unittest.cc:1: /*
RtpSender may use a test or two too to check cluster_id propagated from
TimeToSendPacket/Padding to TransportFeedbackObserver::AddPacket.
Since that involves introduction MockTransportFeedbackObserver, it may be better
to do that in another CL.

[stefan-webrtc, 2016-05-31 15:39:53]

lgtm

https://codereview.webrtc.org/2005313003/diff/60001/webrtc/modules/pacing/pac...
File webrtc/modules/pacing/paced_sender_unittest.cc (right):

https://codereview.webrtc.org/2005313003/diff/60001/webrtc/modules/pacing/pac...
webrtc/modules/pacing/paced_sender_unittest.cc:873: TEST_F(PacedSenderTest,
DISABLED_ProbeClusterId) {
This shouldn't have to be disabled, right?

[philipel, 2016-05-31 15:48:30]

https://codereview.webrtc.org/2005313003/diff/60001/webrtc/modules/pacing/pac...
File webrtc/modules/pacing/paced_sender_unittest.cc (right):

https://codereview.webrtc.org/2005313003/diff/60001/webrtc/modules/pacing/pac...
webrtc/modules/pacing/paced_sender_unittest.cc:873: TEST_F(PacedSenderTest,
DISABLED_ProbeClusterId) {
On 2016/05/31 15:39:53, stefan-webrtc (holmer) wrote:
> This shouldn't have to be disabled, right?

I disabled it on purpose. Right now the test pass, but for the wrong reasons. I
will fix the test and the bug in a follow-up CL.

https://codereview.webrtc.org/2005313003/diff/60001/webrtc/modules/pacing/pac...
File webrtc/modules/pacing/packet_router_unittest.cc (right):

https://codereview.webrtc.org/2005313003/diff/60001/webrtc/modules/pacing/pac...
webrtc/modules/pacing/packet_router_unittest.cc:51: retransmission,
PacketInfo::kNotAProbe))
On 2016/05/31 15:32:14, danilchap wrote:
> integrate testing of cluster_id propagation through packet router here: expect
> and pass some other constant.

Done.

https://codereview.webrtc.org/2005313003/diff/60001/webrtc/modules/pacing/pac...
webrtc/modules/pacing/packet_router_unittest.cc:126: EXPECT_CALL(rtp_1,
TimeToSendPadding(requested_padding_bytes, _))
On 2016/05/31 15:32:14, danilchap wrote:
> Same here.

Done.

https://codereview.webrtc.org/2005313003/diff/60001/webrtc/modules/pacing/pac...
webrtc/modules/pacing/packet_router_unittest.cc:131: rtp_2,
TimeToSendPadding(requested_padding_bytes - sent_padding_bytes, _))
On 2016/05/31 15:32:14, danilchap wrote:
> and here

Done.

https://codereview.webrtc.org/2005313003/diff/60001/webrtc/modules/rtp_rtcp/s...
File webrtc/modules/rtp_rtcp/source/rtp_sender_unittest.cc (right):

https://codereview.webrtc.org/2005313003/diff/60001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtp_sender_unittest.cc:1: /*
On 2016/05/31 15:32:15, danilchap wrote:
> RtpSender may use a test or two too to check cluster_id propagated from
> TimeToSendPacket/Padding to TransportFeedbackObserver::AddPacket.
> Since that involves introduction MockTransportFeedbackObserver, it may be
better
> to do that in another CL.

Acknowledged.

[philipel, 2016-05-31 16:18:34]

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

[commit-bot: I haz the power, 2016-05-31 16:18:41]

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

[commit-bot: I haz the power, 2016-05-31 17:35:08]

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

[commit-bot: I haz the power, 2016-05-31 17:35:09]

Dry run: Try jobs failed on following builders:
  android_dbg on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/android_dbg/builds/13766)

[danilchap, 2016-06-01 08:41:19]

lgtm

[philipel, 2016-06-01 08:46:27]

The CQ bit was checked by philipel@webrtc.org

[philipel, 2016-06-01 08:46:27]

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/2005313003/#ps80001 (title:
"Test probe_cluster_id in PacketRouterTest.")

[commit-bot: I haz the power, 2016-06-01 08:46:36]

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

[commit-bot: I haz the power, 2016-06-01 08:49:25]

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

[commit-bot: I haz the power, 2016-06-01 08:49:27]

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

[philipel, 2016-06-01 09:11:05]

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

[philipel, 2016-06-01 09:11:06]

Magnus, need owner approval for webrtc/voice_engine/channel.cc, PTAL.

[mflodman, 2016-06-01 09:12:49]

channel.cc LGTM

[philipel, 2016-06-01 09:47:36]

The CQ bit was checked by philipel@webrtc.org

[commit-bot: I haz the power, 2016-06-01 09:47:45]

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

[commit-bot: I haz the power, 2016-06-01 09:49:29]

Description was changed from

==========
Propagate probing cluster id to SendTimeHistory, both for packets and padding.


BUG=webrtc:5859
==========

to

==========
Propagate probing cluster id to SendTimeHistory, both for packets and padding.


BUG=webrtc:5859
==========

[commit-bot: I haz the power, 2016-06-01 09:49:30]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-06-01 09:49:35]

Description was changed from

==========
Propagate probing cluster id to SendTimeHistory, both for packets and padding.


BUG=webrtc:5859
==========

to

==========
Propagate probing cluster id to SendTimeHistory, both for packets and padding.

BUG=webrtc:5859

Committed: https://crrev.com/5be28c848b91bc6e4800eac07a3f5ac09a32ad70
Cr-Commit-Position: refs/heads/master@{#12985}
==========

[commit-bot: I haz the power, 2016-06-01 09:49:36]

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

[philipel, 2016-06-01 11:04:28]

A revert of this CL (patchset #5 id:80001) has been created in
https://codereview.webrtc.org/2032463003/ by philipel@webrtc.org.

The reason for reverting is: Breaks google3 buildbot: 
http://webrtc-buildbot-master.mtv.corp.google.com:21000/builders/WebRTC%20goo....