Add histogram stats for send delay for a sent video stream.

Add histogram stats for average send delay of sent packets for a sent video stream. The delay is measured from a packet is sent to the transport until leaving the socket.

- "WebRTC.Video.SendDelayInMs"

Change so that PacketOption packet id is always set in RtpSender (if having a TransportSequenceNumberAllocator).
Add SendDelayStats class for computing delays.
Add SendPacketObserver to RtpRtcp config and register SendDelayStats as observer.
Wire up OnSentPacket to SendDelayStats.

BUG=webrtc:5215
Committed: https://crrev.com/35151f35ecb52a3a06825a946d8ee9f21bf7bc3e
Cr-Commit-Position: refs/heads/master@{#12600}

[åsapersson, 2015-12-02 11:48:10]

Patchset #9 (id:160001) has been deleted

[åsapersson, 2015-12-02 11:48:45]

Patchset #2 (id:20001) has been deleted

[åsapersson, 2015-12-02 11:49:06]

Patchset #4 (id:80001) has been deleted

[åsapersson, 2015-12-02 11:49:23]

Patchset #4 (id:100001) has been deleted

[åsapersson, 2015-12-02 11:49:40]

Patchset #5 (id:140001) has been deleted

[åsapersson, 2015-12-02 11:59:25]

Description was changed from

==========
Send side delay....

BUG=webrtc:5215
==========

to

==========
Add histogram stats for send delay for a sent video stream. Time from a packet
is sent to network until sent.

- "WebRTC.Video.SendDelayInMs" 

Change so that PacketOption packet id is always set in RtpSender (if having a
TransportSequenceNumberAllocator).
Add SendPacketObserver to RtpRtcp config and register SendStatisticsProxy as
observer.

BUG=webrtc:5215
==========

[åsapersson, 2015-12-02 12:00:53]

Description was changed from

==========
Add histogram stats for send delay for a sent video stream. Time from a packet
is sent to network until sent.

- "WebRTC.Video.SendDelayInMs" 

Change so that PacketOption packet id is always set in RtpSender (if having a
TransportSequenceNumberAllocator).
Add SendPacketObserver to RtpRtcp config and register SendStatisticsProxy as
observer.

BUG=webrtc:5215
==========

to

==========
Add histogram stats for send delay for a sent video stream. Time from a packet
is sent to network until sent.

- "WebRTC.Video.SendDelayInMs" 

Change so that PacketOption packet id is always set in RtpSender (if having a
TransportSequenceNumberAllocator).
Add SendPacketObserver to RtpRtcp config and register SendStatisticsProxy as
observer.
Wire up OnSentPacket to SendStatisticsProxy.

BUG=webrtc:5215
==========

[åsapersson, 2015-12-02 12:02:56]

Description was changed from

==========
Add histogram stats for send delay for a sent video stream. Time from a packet
is sent to network until sent.

- "WebRTC.Video.SendDelayInMs" 

Change so that PacketOption packet id is always set in RtpSender (if having a
TransportSequenceNumberAllocator).
Add SendPacketObserver to RtpRtcp config and register SendStatisticsProxy as
observer.
Wire up OnSentPacket to SendStatisticsProxy.

BUG=webrtc:5215
==========

to

==========
Add histogram stats for send delay for a sent video stream. Time from a packet
is sent to network until sent.

- "WebRTC.Video.SendDelayInMs" 

Change so that PacketOption packet id is set in RtpSender (if having a
TransportSequenceNumberAllocator).
Add SendPacketObserver to RtpRtcp config and register SendStatisticsProxy as
observer.
Wire up OnSentPacket to SendStatisticsProxy.

BUG=webrtc:5215
==========

[åsapersson, 2015-12-02 12:17:53]

asapersson@webrtc.org changed reviewers:
+ pbos@webrtc.org, stefan@webrtc.org

[åsapersson, 2015-12-02 12:18:16]

Description was changed from

==========
Add histogram stats for send delay for a sent video stream. Time from a packet
is sent to network until sent.

- "WebRTC.Video.SendDelayInMs" 

Change so that PacketOption packet id is set in RtpSender (if having a
TransportSequenceNumberAllocator).
Add SendPacketObserver to RtpRtcp config and register SendStatisticsProxy as
observer.
Wire up OnSentPacket to SendStatisticsProxy.

BUG=webrtc:5215
==========

to

==========
Add histogram stats for send delay for a sent video stream. Time from a packet
is sent to network until sent is completed.

- "WebRTC.Video.SendDelayInMs" 

Change so that PacketOption packet id is set in RtpSender (if having a
TransportSequenceNumberAllocator).
Add SendPacketObserver to RtpRtcp config and register SendStatisticsProxy as
observer.
Wire up OnSentPacket to SendStatisticsProxy.

BUG=webrtc:5215
==========

[pbos-webrtc, 2015-12-07 06:05:52]

https://codereview.webrtc.org/1478253002/diff/180001/webrtc/video/send_statis...
File webrtc/video/send_statistics_proxy.cc (right):

https://codereview.webrtc.org/1478253002/diff/180001/webrtc/video/send_statis...
webrtc/video/send_statistics_proxy.cc:390: packets_.erase(it);
This means that we can't update packets' send time from both webrtc and talk/
right?

https://codereview.webrtc.org/1478253002/diff/180001/webrtc/video/send_statis...
File webrtc/video/send_statistics_proxy_unittest.cc (right):

https://codereview.webrtc.org/1478253002/diff/180001/webrtc/video/send_statis...
webrtc/video/send_statistics_proxy_unittest.cc:326:
EXPECT_FALSE(statistics_proxy_->OnSentPacket(kPacketId));
This can be called more than once right? If this is called both when it gets out
to talk/ from webrtc/ and when it gets put on the socket, aren't we failing to
update the actual time here (calls from libjingle's OnSentPackets are ignored).
Am I reading this wrong?

https://codereview.webrtc.org/1478253002/diff/180001/webrtc/video_engine/vie_...
File webrtc/video_engine/vie_channel.h (right):

https://codereview.webrtc.org/1478253002/diff/180001/webrtc/video_engine/vie_...
webrtc/video_engine/vie_channel.h:387: class RegisterableSendPacketObserver
Can this be set on construction instead so that we don't need to hop between
this class?

[pbos-webrtc, 2015-12-07 06:06:52]

https://codereview.webrtc.org/1478253002/diff/180001/webrtc/call/call.cc
File webrtc/call/call.cc (right):

https://codereview.webrtc.org/1478253002/diff/180001/webrtc/call/call.cc#newc...
webrtc/call/call.cc:573: for (VideoSendStream* stream : video_send_streams_) {
Can you somehow know which stream to deliver to here? Otherwise should these
stats be in a CallStatisticsProxy instead of per-stream?

[pbos-webrtc, 2015-12-07 06:07:25]

On 2015/12/07 06:06:52, pbos-webrtc wrote:
> https://codereview.webrtc.org/1478253002/diff/180001/webrtc/call/call.cc
> File webrtc/call/call.cc (right):
> 
>
https://codereview.webrtc.org/1478253002/diff/180001/webrtc/call/call.cc#newc...
> webrtc/call/call.cc:573: for (VideoSendStream* stream : video_send_streams_) {
> Can you somehow know which stream to deliver to here? Otherwise should these
> stats be in a CallStatisticsProxy instead of per-stream?

This might not be a good idea (with CallStatisticsProxy) since multiple streams
will have different encoding times etc.

[åsapersson, 2015-12-08 12:37:43]

Patchset #7 (id:220001) has been deleted

[åsapersson, 2015-12-08 12:50:15]

https://codereview.webrtc.org/1478253002/diff/180001/webrtc/call/call.cc
File webrtc/call/call.cc (right):

https://codereview.webrtc.org/1478253002/diff/180001/webrtc/call/call.cc#newc...
webrtc/call/call.cc:573: for (VideoSendStream* stream : video_send_streams_) {
On 2015/12/07 06:06:52, pbos-webrtc wrote:
> Can you somehow know which stream to deliver to here? Otherwise should these
> stats be in a CallStatisticsProxy instead of per-stream?

No don't know which stream it belongs to here (but stats stored per ssrc in
SendStatisticsProxy).

https://codereview.webrtc.org/1478253002/diff/180001/webrtc/video/send_statis...
File webrtc/video/send_statistics_proxy.cc (right):

https://codereview.webrtc.org/1478253002/diff/180001/webrtc/video/send_statis...
webrtc/video/send_statistics_proxy.cc:390: packets_.erase(it);
On 2015/12/07 06:05:52, pbos-webrtc wrote:
> This means that we can't update packets' send time from both webrtc and talk/
> right?

OnSendPacket is called from webrtc
OnSentPacket is called from talk

Will think of a better name for OnSendPacket.

https://codereview.webrtc.org/1478253002/diff/180001/webrtc/video/send_statis...
File webrtc/video/send_statistics_proxy_unittest.cc (right):

https://codereview.webrtc.org/1478253002/diff/180001/webrtc/video/send_statis...
webrtc/video/send_statistics_proxy_unittest.cc:326:
EXPECT_FALSE(statistics_proxy_->OnSentPacket(kPacketId));
On 2015/12/07 06:05:52, pbos-webrtc wrote:
> This can be called more than once right? If this is called both when it gets
out
> to talk/ from webrtc/ and when it gets put on the socket, aren't we failing to
> update the actual time here (calls from libjingle's OnSentPackets are
ignored).
> Am I reading this wrong?

OnSentPacket should only be called from talk.

https://codereview.webrtc.org/1478253002/diff/180001/webrtc/video_engine/vie_...
File webrtc/video_engine/vie_channel.h (right):

https://codereview.webrtc.org/1478253002/diff/180001/webrtc/video_engine/vie_...
webrtc/video_engine/vie_channel.h:387: class RegisterableSendPacketObserver
On 2015/12/07 06:05:52, pbos-webrtc wrote:
> Can this be set on construction instead so that we don't need to hop between
> this class?

Followed the current setup. Maybe do the setup change in a separate cl?

[stefan-webrtc, 2015-12-10 08:37:50]

https://codereview.webrtc.org/1478253002/diff/240001/webrtc/call/call.cc
File webrtc/call/call.cc (right):

https://codereview.webrtc.org/1478253002/diff/240001/webrtc/call/call.cc#newc...
webrtc/call/call.cc:574: stream->OnSentPacket(sent_packet.packet_id);
Should this method return if the packet belonged to that send stream so that we
don't have to loop over all send streams? Or would it make sense to actually
move send delay tracking to Call instead? Maybe we want to track this per
stream.

https://codereview.webrtc.org/1478253002/diff/240001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/rtp_sender.cc (right):

https://codereview.webrtc.org/1478253002/diff/240001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_sender.cc:1075:
UpdateOnSendPacket(capture_time_ms, &options.packet_id);
I don't think we have to add it here since this path is deprecated and shouldn't
be used. All packets are going through the pacer.

https://codereview.webrtc.org/1478253002/diff/240001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_sender.cc:1137: ssrc = ssrc_;
The ssrc is available without locking from the rtp_header after line 912.

https://codereview.webrtc.org/1478253002/diff/240001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_sender.cc:1140: *packet_id =
transport_sequence_number_allocator_->AllocateSequenceNumber();
Can we move this out of this method and call it once before this method is
called? I think we should call it for all packets we send, but only set the
transport sequence number if the extension is used, see line 942.

https://codereview.webrtc.org/1478253002/diff/240001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_sender.cc:1142:
send_packet_observer_->OnSendPacket(*packet_id, capture_time_ms, ssrc);
Is it possible to unify this callback with the callback on line 946, so that we
only have to register a single "OnSendPacket"/"AddPacket" callback to the
module?

https://codereview.webrtc.org/1478253002/diff/240001/webrtc/video/end_to_end_...
File webrtc/video/end_to_end_tests.cc (right):

https://codereview.webrtc.org/1478253002/diff/240001/webrtc/video/end_to_end_...
webrtc/video/end_to_end_tests.cc:2059: EXPECT_EQ(1,
test::NumHistogramSamples(video_prefix + "SendDelayInMs"));
Shouldn't we just remove the SendSideDelayInMs above?

https://codereview.webrtc.org/1478253002/diff/240001/webrtc/video/send_statis...
File webrtc/video/send_statistics_proxy.cc (right):

https://codereview.webrtc.org/1478253002/diff/240001/webrtc/video/send_statis...
webrtc/video/send_statistics_proxy.cc:414: packets_[packet_id] = packet;
Would be nice to use a constructor here instead.

https://codereview.webrtc.org/1478253002/diff/240001/webrtc/video/send_statis...
webrtc/video/send_statistics_proxy.cc:426: // TODO(asapersson): Use diff to
cap_time_ms to update delay_counter_.
I think we would like to track all of capture -> send, send -> sent and capture
-> sent. That way we will be able to figure out where in the pipe delay started
to build up.

https://codereview.webrtc.org/1478253002/diff/240001/webrtc/video/send_statis...
webrtc/video/send_statistics_proxy.cc:440: }
Maybe we should add some LOG here to catch if we have any situations where the
callbacks from the socket doesn't trigger?

https://codereview.webrtc.org/1478253002/diff/240001/webrtc/video/send_statis...
File webrtc/video/send_statistics_proxy.h (right):

https://codereview.webrtc.org/1478253002/diff/240001/webrtc/video/send_statis...
webrtc/video/send_statistics_proxy.h:114: int64_t cap_time_ms;
capture_time_ms

[åsapersson, 2015-12-15 14:09:25]

Patchset #8 (id:260001) has been deleted

[åsapersson, 2015-12-15 14:28:27]

https://codereview.webrtc.org/1478253002/diff/240001/webrtc/call/call.cc
File webrtc/call/call.cc (right):

https://codereview.webrtc.org/1478253002/diff/240001/webrtc/call/call.cc#newc...
webrtc/call/call.cc:574: stream->OnSentPacket(sent_packet.packet_id);
On 2015/12/10 08:37:49, stefan-webrtc (holmer) wrote:
> Should this method return if the packet belonged to that send stream so that
we
> don't have to loop over all send streams? Or would it make sense to actually
> move send delay tracking to Call instead? Maybe we want to track this per
> stream.

Done.
See other comment about per stream.

https://codereview.webrtc.org/1478253002/diff/240001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/rtp_sender.cc (right):

https://codereview.webrtc.org/1478253002/diff/240001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_sender.cc:1075:
UpdateOnSendPacket(capture_time_ms, &options.packet_id);
On 2015/12/10 08:37:49, stefan-webrtc (holmer) wrote:
> I don't think we have to add it here since this path is deprecated and
shouldn't
> be used. All packets are going through the pacer.

OK, removed from this path.

https://codereview.webrtc.org/1478253002/diff/240001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_sender.cc:1137: ssrc = ssrc_;
On 2015/12/10 08:37:49, stefan-webrtc (holmer) wrote:
> The ssrc is available without locking from the rtp_header after line 912.

Done.

https://codereview.webrtc.org/1478253002/diff/240001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_sender.cc:1140: *packet_id =
transport_sequence_number_allocator_->AllocateSequenceNumber();
On 2015/12/10 08:37:49, stefan-webrtc (holmer) wrote:
> Can we move this out of this method and call it once before this method is
> called? I think we should call it for all packets we send, but only set the
> transport sequence number if the extension is used, see line 942.

Done.

https://codereview.webrtc.org/1478253002/diff/240001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_sender.cc:1142:
send_packet_observer_->OnSendPacket(*packet_id, capture_time_ms, ssrc);
On 2015/12/10 08:37:49, stefan-webrtc (holmer) wrote:
> Is it possible to unify this callback with the callback on line 946, so that
we
> only have to register a single "OnSendPacket"/"AddPacket" callback to the
> module?

I guess it can be done if we do not want to do it per stream. Wdyt?
Use SendTimeHistory (and add capture_time_ms info for cap -> sent)?
Would audio also be included then?

https://codereview.webrtc.org/1478253002/diff/240001/webrtc/video/end_to_end_...
File webrtc/video/end_to_end_tests.cc (right):

https://codereview.webrtc.org/1478253002/diff/240001/webrtc/video/end_to_end_...
webrtc/video/end_to_end_tests.cc:2059: EXPECT_EQ(1,
test::NumHistogramSamples(video_prefix + "SendDelayInMs"));
On 2015/12/10 08:37:50, stefan-webrtc (holmer) wrote:
> Shouldn't we just remove the SendSideDelayInMs above?

SendSideDelayInMs is currently the delay from capture -> send
I plan to update it to be capture -> sent.

https://codereview.webrtc.org/1478253002/diff/240001/webrtc/video/send_statis...
File webrtc/video/send_statistics_proxy.cc (right):

https://codereview.webrtc.org/1478253002/diff/240001/webrtc/video/send_statis...
webrtc/video/send_statistics_proxy.cc:414: packets_[packet_id] = packet;
On 2015/12/10 08:37:50, stefan-webrtc (holmer) wrote:
> Would be nice to use a constructor here instead.

Done.

https://codereview.webrtc.org/1478253002/diff/240001/webrtc/video/send_statis...
webrtc/video/send_statistics_proxy.cc:426: // TODO(asapersson): Use diff to
cap_time_ms to update delay_counter_.
On 2015/12/10 08:37:50, stefan-webrtc (holmer) wrote:
> I think we would like to track all of capture -> send, send -> sent and
capture
> -> sent. That way we will be able to figure out where in the pipe delay
started
> to build up.

Encode time stats is capture -> pacer.
I plan to add pacer delay: pacer -> send
This CL: send -> sent
Then, update delay_counter_ to be total: capture -> sent (and remove
SendSideDelayUpdated)

https://codereview.webrtc.org/1478253002/diff/240001/webrtc/video/send_statis...
File webrtc/video/send_statistics_proxy.h (right):

https://codereview.webrtc.org/1478253002/diff/240001/webrtc/video/send_statis...
webrtc/video/send_statistics_proxy.h:114: int64_t cap_time_ms;
On 2015/12/10 08:37:50, stefan-webrtc (holmer) wrote:
> capture_time_ms

Done.

[stefan-webrtc, 2016-01-18 19:48:26]

https://codereview.webrtc.org/1478253002/diff/240001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/rtp_sender.cc (right):

https://codereview.webrtc.org/1478253002/diff/240001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_sender.cc:1142:
send_packet_observer_->OnSendPacket(*packet_id, capture_time_ms, ssrc);
On 2015/12/15 14:28:26, åsapersson wrote:
> On 2015/12/10 08:37:49, stefan-webrtc (holmer) wrote:
> > Is it possible to unify this callback with the callback on line 946, so that
> we
> > only have to register a single "OnSendPacket"/"AddPacket" callback to the
> > module?
> 
> I guess it can be done if we do not want to do it per stream. Wdyt?
> Use SendTimeHistory (and add capture_time_ms info for cap -> sent)?
> Would audio also be included then?

Why doesn't it work per stream? I don't think I understand the issue. I was
mostly thinking that we could register a single "observer" object to this class
instead of two separate. That observer could still have two methods, "AddPacket"
and "OnSendPacket".

https://codereview.webrtc.org/1478253002/diff/240001/webrtc/video/send_statis...
File webrtc/video/send_statistics_proxy.cc (right):

https://codereview.webrtc.org/1478253002/diff/240001/webrtc/video/send_statis...
webrtc/video/send_statistics_proxy.cc:426: // TODO(asapersson): Use diff to
cap_time_ms to update delay_counter_.
On 2015/12/15 14:28:27, åsapersson wrote:
> On 2015/12/10 08:37:50, stefan-webrtc (holmer) wrote:
> > I think we would like to track all of capture -> send, send -> sent and
> capture
> > -> sent. That way we will be able to figure out where in the pipe delay
> started
> > to build up.
> 
> Encode time stats is capture -> pacer.
> I plan to add pacer delay: pacer -> send
> This CL: send -> sent
> Then, update delay_counter_ to be total: capture -> sent (and remove
> SendSideDelayUpdated)

Sounds great.

https://codereview.webrtc.org/1478253002/diff/240001/webrtc/video/send_statis...
webrtc/video/send_statistics_proxy.cc:440: }
On 2015/12/10 08:37:50, stefan-webrtc (holmer) wrote:
> Maybe we should add some LOG here to catch if we have any situations where the
> callbacks from the socket doesn't trigger?

Do you think this would be good to have? Especially  when we know we have had
one issue with these callbacks already

https://codereview.webrtc.org/1478253002/diff/280001/webrtc/call/call.cc
File webrtc/call/call.cc (right):

https://codereview.webrtc.org/1478253002/diff/280001/webrtc/call/call.cc#newc...
webrtc/call/call.cc:574: if (stream->OnSentPacket(sent_packet.packet_id))
Should there be a TODO here to use the sent_packet.send_time_ms?

https://codereview.webrtc.org/1478253002/diff/280001/webrtc/common_types.h
File webrtc/common_types.h (right):

https://codereview.webrtc.org/1478253002/diff/280001/webrtc/common_types.h#ne...
webrtc/common_types.h:284: // Callback, used to notify an observer whenever a
packet is sent to network.
Maybe "sent to the transport" since this is when the packet leaves the webrtc
layer and not when it leaves the socket.

https://codereview.webrtc.org/1478253002/diff/280001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/rtp_sender.cc (right):

https://codereview.webrtc.org/1478253002/diff/280001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_sender.cc:954: bool
RTPSender::UseTransportSequenceNumber(int* packet_id) const {
Maybe this should say that it actually allocates a sequence number even if the
extension isn't registered? I think it might be worth the extra code to split it
in two because of this. WDYT?

https://codereview.webrtc.org/1478253002/diff/280001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_sender.cc:1097: if
(!send_side_delay_observer_ || capture_time_ms <= 0)
Do you think it would be possible to instead use a DCHECK:
DCHECK(capture_time_ms <= 0 || audio_configured_)?

https://codereview.webrtc.org/1478253002/diff/280001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_sender.cc:1132: if (!send_packet_observer_ ||
capture_time_ms <= 0 || packet_id == -1)
Same here about DCHECK. Should be able to DCHECK both on capture_time_ms as
above and also have one on packet_id >= 0 since it always should be set, right?

https://codereview.webrtc.org/1478253002/diff/280001/webrtc/video/send_statis...
File webrtc/video/send_statistics_proxy.cc (right):

https://codereview.webrtc.org/1478253002/diff/280001/webrtc/video/send_statis...
webrtc/video/send_statistics_proxy.cc:433: if (now - it->second.capture_time_ms_
< kMaxSentPacketDelayMs) {
Feel free to remove {}

https://codereview.webrtc.org/1478253002/diff/280001/webrtc/video/send_statis...
File webrtc/video/send_statistics_proxy.h (right):

https://codereview.webrtc.org/1478253002/diff/280001/webrtc/video/send_statis...
webrtc/video/send_statistics_proxy.h:119: int64_t send_time_ms_;
I prefer to remove the trailing _ since this is a struct.

[stefan-webrtc, 2016-03-30 11:14:34]

Any progress on this?

[åsapersson, 2016-04-05 12:02:37]

Patchset #12 (id:360001) has been deleted

[åsapersson, 2016-04-05 12:13:35]

Patchset #11 (id:340001) has been deleted

[åsapersson, 2016-04-06 14:22:47]

Patchset #13 (id:420001) has been deleted

[åsapersson, 2016-04-06 14:52:37]

https://codereview.webrtc.org/1478253002/diff/240001/webrtc/video/send_statis...
File webrtc/video/send_statistics_proxy.cc (right):

https://codereview.webrtc.org/1478253002/diff/240001/webrtc/video/send_statis...
webrtc/video/send_statistics_proxy.cc:440: }
On 2016/01/18 19:48:25, stefan-webrtc (holmer) wrote:
> On 2015/12/10 08:37:50, stefan-webrtc (holmer) wrote:
> > Maybe we should add some LOG here to catch if we have any situations where
the
> > callbacks from the socket doesn't trigger?
> 
> Do you think this would be good to have? Especially  when we know we have had
> one issue with these callbacks already 

Added logs for number of old and skipped packets.

https://codereview.webrtc.org/1478253002/diff/280001/webrtc/call/call.cc
File webrtc/call/call.cc (right):

https://codereview.webrtc.org/1478253002/diff/280001/webrtc/call/call.cc#newc...
webrtc/call/call.cc:574: if (stream->OnSentPacket(sent_packet.packet_id))
On 2016/01/18 19:48:25, stefan-webrtc (holmer) wrote:
> Should there be a TODO here to use the sent_packet.send_time_ms?

Done.

https://codereview.webrtc.org/1478253002/diff/280001/webrtc/common_types.h
File webrtc/common_types.h (right):

https://codereview.webrtc.org/1478253002/diff/280001/webrtc/common_types.h#ne...
webrtc/common_types.h:284: // Callback, used to notify an observer whenever a
packet is sent to network.
On 2016/01/18 19:48:25, stefan-webrtc (holmer) wrote:
> Maybe "sent to the transport" since this is when the packet leaves the webrtc
> layer and not when it leaves the socket.

Done.

https://codereview.webrtc.org/1478253002/diff/280001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/rtp_sender.cc (right):

https://codereview.webrtc.org/1478253002/diff/280001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_sender.cc:954: bool
RTPSender::UseTransportSequenceNumber(int* packet_id) const {
On 2016/01/18 19:48:25, stefan-webrtc (holmer) wrote:
> Maybe this should say that it actually allocates a sequence number even if the
> extension isn't registered? I think it might be worth the extra code to split
it
> in two because of this. WDYT?

Changed to AllocateTransportSequenceNumber. Removed the registration check
(checked in UpdateTransportSequenceNumber).

https://codereview.webrtc.org/1478253002/diff/280001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_sender.cc:1097: if
(!send_side_delay_observer_ || capture_time_ms <= 0)
On 2016/01/18 19:48:25, stefan-webrtc (holmer) wrote:
> Do you think it would be possible to instead use a DCHECK:
> DCHECK(capture_time_ms <= 0 || audio_configured_)?

Not sure if set in all codec wrappers, would need to check separately.

https://codereview.webrtc.org/1478253002/diff/280001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_sender.cc:1132: if (!send_packet_observer_ ||
capture_time_ms <= 0 || packet_id == -1)
On 2016/01/18 19:48:25, stefan-webrtc (holmer) wrote:
> Same here about DCHECK. Should be able to DCHECK both on capture_time_ms as
> above and also have one on packet_id >= 0 since it always should be set,
right?

packet_id could be -1 as it is now.

https://codereview.webrtc.org/1478253002/diff/280001/webrtc/video/send_statis...
File webrtc/video/send_statistics_proxy.cc (right):

https://codereview.webrtc.org/1478253002/diff/280001/webrtc/video/send_statis...
webrtc/video/send_statistics_proxy.cc:433: if (now - it->second.capture_time_ms_
< kMaxSentPacketDelayMs) {
On 2016/01/18 19:48:25, stefan-webrtc (holmer) wrote:
> Feel free to remove {}

Done.

https://codereview.webrtc.org/1478253002/diff/280001/webrtc/video/send_statis...
File webrtc/video/send_statistics_proxy.h (right):

https://codereview.webrtc.org/1478253002/diff/280001/webrtc/video/send_statis...
webrtc/video/send_statistics_proxy.h:119: int64_t send_time_ms_;
On 2016/01/18 19:48:26, stefan-webrtc (holmer) wrote:
> I prefer to remove the trailing _ since this is a struct.

Done.

[åsapersson, 2016-04-07 06:58:47]

Patchset #14 (id:460001) has been deleted

[åsapersson, 2016-04-07 07:00:32]

Patchset #12 (id:400001) has been deleted

[åsapersson, 2016-04-07 07:20:20]

Patchset #13 (id:480001) has been deleted

[stefan-webrtc, 2016-04-19 11:06:15]

lg, but you probably have to rebase

https://codereview.webrtc.org/1478253002/diff/500001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/rtp_sender.cc (right):

https://codereview.webrtc.org/1478253002/diff/500001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_sender.cc:681:
AllocateTransportSequenceNumber(&options.packet_id)) {
Don't we want to set options.packet_id even if we're not using_transport_seq,
but only write it into the packet header if using_transport_seq?

https://codereview.webrtc.org/1478253002/diff/500001/webrtc/video/send_statis...
File webrtc/video/send_statistics_proxy.cc (right):

https://codereview.webrtc.org/1478253002/diff/500001/webrtc/video/send_statis...
webrtc/video/send_statistics_proxy.cc:208: if (send_delay_ms != -1)
{}

[åsapersson, 2016-04-19 14:53:32]

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

[åsapersson, 2016-04-19 14:55:33]

https://codereview.webrtc.org/1478253002/diff/500001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/rtp_sender.cc (right):

https://codereview.webrtc.org/1478253002/diff/500001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_sender.cc:681:
AllocateTransportSequenceNumber(&options.packet_id)) {
On 2016/04/19 11:06:15, stefan-webrtc (holmer) wrote:
> Don't we want to set options.packet_id even if we're not using_transport_seq,
> but only write it into the packet header if using_transport_seq?

Thought it was not necessary to allocate and set options.packet_id if not used
(by stats or transport sequence number). But I can change (remove the
using_transport_seq check), wdyt?

https://codereview.webrtc.org/1478253002/diff/500001/webrtc/video/send_statis...
File webrtc/video/send_statistics_proxy.cc (right):

https://codereview.webrtc.org/1478253002/diff/500001/webrtc/video/send_statis...
webrtc/video/send_statistics_proxy.cc:208: if (send_delay_ms != -1)
On 2016/04/19 11:06:15, stefan-webrtc (holmer) wrote:
> {}

Done.

[åsapersson, 2016-04-20 08:17:58]

Description was changed from

==========
Add histogram stats for send delay for a sent video stream. Time from a packet
is sent to network until sent is completed.

- "WebRTC.Video.SendDelayInMs" 

Change so that PacketOption packet id is set in RtpSender (if having a
TransportSequenceNumberAllocator).
Add SendPacketObserver to RtpRtcp config and register SendStatisticsProxy as
observer.
Wire up OnSentPacket to SendStatisticsProxy.

BUG=webrtc:5215
==========

to

==========
Add histogram stats for send delay for a sent video stream. The delay is
measured from a packet is sent to the transport until leaving the socket.

- "WebRTC.Video.SendDelayInMs" 

Change so that PacketOption packet id is set in RtpSender (if having a
TransportSequenceNumberAllocator).
Add SendPacketObserver to RtpRtcp config and register SendStatisticsProxy as
observer.
Wire up OnSentPacket to SendStatisticsProxy.

BUG=webrtc:5215
==========

[åsapersson, 2016-04-20 08:26:16]

Description was changed from

==========
Add histogram stats for send delay for a sent video stream. The delay is
measured from a packet is sent to the transport until leaving the socket.

- "WebRTC.Video.SendDelayInMs" 

Change so that PacketOption packet id is set in RtpSender (if having a
TransportSequenceNumberAllocator).
Add SendPacketObserver to RtpRtcp config and register SendStatisticsProxy as
observer.
Wire up OnSentPacket to SendStatisticsProxy.

BUG=webrtc:5215
==========

to

==========
Add histogram stats for average send delay for a sent video stream. The delay is
measured from a packet is sent to the transport until leaving the socket.

- "WebRTC.Video.SendDelayInMs" 

Change so that PacketOption packet id is set in RtpSender (if having a
TransportSequenceNumberAllocator).
Add SendPacketObserver to RtpRtcp config and register SendStatisticsProxy as
observer.
Wire up OnSentPacket to SendStatisticsProxy.

BUG=webrtc:5215
==========

[åsapersson, 2016-04-20 08:36:28]

Description was changed from

==========
Add histogram stats for average send delay for a sent video stream. The delay is
measured from a packet is sent to the transport until leaving the socket.

- "WebRTC.Video.SendDelayInMs" 

Change so that PacketOption packet id is set in RtpSender (if having a
TransportSequenceNumberAllocator).
Add SendPacketObserver to RtpRtcp config and register SendStatisticsProxy as
observer.
Wire up OnSentPacket to SendStatisticsProxy.

BUG=webrtc:5215
==========

to

==========
Add histogram stats for average send delay of sent packets for a sent video
stream. The delay is measured from a packet is sent to the transport until
leaving the socket.

- "WebRTC.Video.SendDelayInMs" 

Change so that PacketOption packet id is set in RtpSender (if having a
TransportSequenceNumberAllocator).
Add SendPacketObserver to RtpRtcp config and register SendStatisticsProxy as
observer.
Wire up OnSentPacket to SendStatisticsProxy.

BUG=webrtc:5215
==========

[stefan-webrtc, 2016-04-20 10:59:55]

lgtm

https://codereview.webrtc.org/1478253002/diff/500001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/rtp_sender.cc (right):

https://codereview.webrtc.org/1478253002/diff/500001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_sender.cc:681:
AllocateTransportSequenceNumber(&options.packet_id)) {
On 2016/04/19 14:55:32, åsapersson wrote:
> On 2016/04/19 11:06:15, stefan-webrtc (holmer) wrote:
> > Don't we want to set options.packet_id even if we're not
using_transport_seq,
> > but only write it into the packet header if using_transport_seq?
> 
> Thought it was not necessary to allocate and set options.packet_id if not used
> (by stats or transport sequence number). But I can change (remove the
> using_transport_seq check), wdyt? 

It's not, but I can't see that you checked if the sequence numbers are needed
for stats or not? Anyway, I think we might as well add them all the time to
options, but only write them in the bitstream when using the transport sequence
number header extension.

[åsapersson, 2016-04-20 11:16:56]

https://codereview.webrtc.org/1478253002/diff/500001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/rtp_sender.cc (right):

https://codereview.webrtc.org/1478253002/diff/500001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_sender.cc:681:
AllocateTransportSequenceNumber(&options.packet_id)) {
On 2016/04/20 10:59:54, stefan-webrtc (holmer) wrote:
> On 2016/04/19 14:55:32, åsapersson wrote:
> > On 2016/04/19 11:06:15, stefan-webrtc (holmer) wrote:
> > > Don't we want to set options.packet_id even if we're not
> using_transport_seq,
> > > but only write it into the packet header if using_transport_seq?
> > 
> > Thought it was not necessary to allocate and set options.packet_id if not
used
> > (by stats or transport sequence number). But I can change (remove the
> > using_transport_seq check), wdyt? 
> 
> It's not, but I can't see that you checked if the sequence numbers are needed
> for stats or not? Anyway, I think we might as well add them all the time to
> options, but only write them in the bitstream when using the transport
sequence
> number header extension.

Stats not updated, UpdateOnSendPacket not called.

Ok, changed so sequence numbers are always allocated either way.

[åsapersson, 2016-04-20 11:27:43]

Magnus, can you have a look at webrtc/common_types.h

[mflodman, 2016-04-26 06:49:11]

Sorry for the late review, two comments.

https://codereview.webrtc.org/1478253002/diff/580001/webrtc/video/send_statis...
File webrtc/video/send_statistics_proxy.cc (right):

https://codereview.webrtc.org/1478253002/diff/580001/webrtc/video/send_statis...
webrtc/video/send_statistics_proxy.cc:640: int diff_ms =
clock_->TimeInMilliseconds() - it->second.send_time_ms;
Is there some way we could get this time when actually sent on the socket or
doesn't that work due to clock differences?

Anything else we can do to get this time as early as possible?

https://codereview.webrtc.org/1478253002/diff/580001/webrtc/video/video_send_...
File webrtc/video/video_send_stream.h (right):

https://codereview.webrtc.org/1478253002/diff/580001/webrtc/video/video_send_...
webrtc/video/video_send_stream.h:79: bool OnSentPacket(int packet_id);
I'd prefer OnPacketSent, wdyt?

[åsapersson, 2016-04-28 10:17:01]

Patchset #18 (id:600001) has been deleted

[åsapersson, 2016-04-28 10:36:53]

Patchset #19 (id:640001) has been deleted

[åsapersson, 2016-04-28 11:16:20]

Patchset #19 (id:660001) has been deleted

[åsapersson, 2016-04-28 11:38:02]

Description was changed from

==========
Add histogram stats for average send delay of sent packets for a sent video
stream. The delay is measured from a packet is sent to the transport until
leaving the socket.

- "WebRTC.Video.SendDelayInMs" 

Change so that PacketOption packet id is set in RtpSender (if having a
TransportSequenceNumberAllocator).
Add SendPacketObserver to RtpRtcp config and register SendStatisticsProxy as
observer.
Wire up OnSentPacket to SendStatisticsProxy.

BUG=webrtc:5215
==========

to

==========
Add histogram stats for average send delay of sent packets for a sent video
stream. The delay is measured from a packet is sent to the transport until
leaving the socket.

- "WebRTC.Video.SendDelayInMs" 

Change so that PacketOption packet id is always set in RtpSender (if having a
TransportSequenceNumberAllocator).
Add SendDelayStats class in call.
Add SendPacketObserver to RtpRtcp config and register SendDelayStats as
observer.
Wire up OnSentPacket to SendDelayStats.

BUG=webrtc:5215
==========

[åsapersson, 2016-04-28 11:44:40]

Description was changed from

==========
Add histogram stats for average send delay of sent packets for a sent video
stream. The delay is measured from a packet is sent to the transport until
leaving the socket.

- "WebRTC.Video.SendDelayInMs" 

Change so that PacketOption packet id is always set in RtpSender (if having a
TransportSequenceNumberAllocator).
Add SendDelayStats class in call.
Add SendPacketObserver to RtpRtcp config and register SendDelayStats as
observer.
Wire up OnSentPacket to SendDelayStats.

BUG=webrtc:5215
==========

to

==========
Add histogram stats for average send delay of sent packets for a sent video
stream. The delay is measured from a packet is sent to the transport until
leaving the socket.

- "WebRTC.Video.SendDelayInMs" 

Change so that PacketOption packet id is always set in RtpSender (if having a
TransportSequenceNumberAllocator).
Add SendDelayStats class for computing delays.
Add SendPacketObserver to RtpRtcp config and register SendDelayStats as
observer.
Wire up OnSentPacket to SendDelayStats.

BUG=webrtc:5215
==========

[åsapersson, 2016-04-28 12:26:19]

I have moved the delay computations to a separate class to avoid taking the
video send stream lock in Call (could cause potential deadlock when sending RTCP
bye packets), PTAL.

https://codereview.webrtc.org/1478253002/diff/580001/webrtc/video/send_statis...
File webrtc/video/send_statistics_proxy.cc (right):

https://codereview.webrtc.org/1478253002/diff/580001/webrtc/video/send_statis...
webrtc/video/send_statistics_proxy.cc:640: int diff_ms =
clock_->TimeInMilliseconds() - it->second.send_time_ms;
On 2016/04/26 06:49:11, mflodman wrote:
> Is there some way we could get this time when actually sent on the socket or
> doesn't that work due to clock differences?
> 
> Anything else we can do to get this time as early as possible?

Right, it is not working due to clock differences. Changed so the time is got a
bit earlier, in Call for now.

https://codereview.webrtc.org/1478253002/diff/580001/webrtc/video/video_send_...
File webrtc/video/video_send_stream.h (right):

https://codereview.webrtc.org/1478253002/diff/580001/webrtc/video/video_send_...
webrtc/video/video_send_stream.h:79: bool OnSentPacket(int packet_id);
On 2016/04/26 06:49:11, mflodman wrote:
> I'd prefer OnPacketSent, wdyt?

Called from Call::OnSentPacket. Maybe keep the same name.

[mflodman, 2016-05-02 07:21:10]

lgtm

https://codereview.webrtc.org/1478253002/diff/580001/webrtc/video/video_send_...
File webrtc/video/video_send_stream.h (right):

https://codereview.webrtc.org/1478253002/diff/580001/webrtc/video/video_send_...
webrtc/video/video_send_stream.h:79: bool OnSentPacket(int packet_id);
On 2016/04/28 12:26:19, åsapersson wrote:
> On 2016/04/26 06:49:11, mflodman wrote:
> > I'd prefer OnPacketSent, wdyt?
> 
> Called from Call::OnSentPacket. Maybe keep the same name.

Ah, ok.

[stefan-webrtc, 2016-05-02 10:29:46]

lgtm

[pbos-webrtc, 2016-05-02 21:42:02]

Haven't had time to look through this, I think you can submit it without my
review.

[åsapersson, 2016-05-03 06:37:33]

The CQ bit was checked by asapersson@webrtc.org

[åsapersson, 2016-05-03 06:37:34]

The patchset sent to the CQ was uploaded after l-g-t-m from stefan@webrtc.org,
mflodman@webrtc.org
Link to the patchset: https://codereview.webrtc.org/1478253002/#ps700001 (title:
"rebase")

[commit-bot: I haz the power, 2016-05-03 06:37:45]

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

[commit-bot: I haz the power, 2016-05-03 06:44:05]

Description was changed from

==========
Add histogram stats for average send delay of sent packets for a sent video
stream. The delay is measured from a packet is sent to the transport until
leaving the socket.

- "WebRTC.Video.SendDelayInMs" 

Change so that PacketOption packet id is always set in RtpSender (if having a
TransportSequenceNumberAllocator).
Add SendDelayStats class for computing delays.
Add SendPacketObserver to RtpRtcp config and register SendDelayStats as
observer.
Wire up OnSentPacket to SendDelayStats.

BUG=webrtc:5215
==========

to

==========
Add histogram stats for average send delay of sent packets for a sent video
stream. The delay is measured from a packet is sent to the transport until
leaving the socket.

- "WebRTC.Video.SendDelayInMs" 

Change so that PacketOption packet id is always set in RtpSender (if having a
TransportSequenceNumberAllocator).
Add SendDelayStats class for computing delays.
Add SendPacketObserver to RtpRtcp config and register SendDelayStats as
observer.
Wire up OnSentPacket to SendDelayStats.

BUG=webrtc:5215
==========

[commit-bot: I haz the power, 2016-05-03 06:44:07]

Committed patchset #20 (id:700001)

[commit-bot: I haz the power, 2016-05-03 06:44:17]

Description was changed from

==========
Add histogram stats for average send delay of sent packets for a sent video
stream. The delay is measured from a packet is sent to the transport until
leaving the socket.

- "WebRTC.Video.SendDelayInMs" 

Change so that PacketOption packet id is always set in RtpSender (if having a
TransportSequenceNumberAllocator).
Add SendDelayStats class for computing delays.
Add SendPacketObserver to RtpRtcp config and register SendDelayStats as
observer.
Wire up OnSentPacket to SendDelayStats.

BUG=webrtc:5215
==========

to

==========
Add histogram stats for average send delay of sent packets for a sent video
stream. The delay is measured from a packet is sent to the transport until
leaving the socket.

- "WebRTC.Video.SendDelayInMs"

Change so that PacketOption packet id is always set in RtpSender (if having a
TransportSequenceNumberAllocator).
Add SendDelayStats class for computing delays.
Add SendPacketObserver to RtpRtcp config and register SendDelayStats as
observer.
Wire up OnSentPacket to SendDelayStats.

BUG=webrtc:5215

Committed: https://crrev.com/35151f35ecb52a3a06825a946d8ee9f21bf7bc3e
Cr-Commit-Position: refs/heads/master@{#12600}
==========

[commit-bot: I haz the power, 2016-05-03 06:44:19]

Patchset 20 (id:??) landed as
https://crrev.com/35151f35ecb52a3a06825a946d8ee9f21bf7bc3e
Cr-Commit-Position: refs/heads/master@{#12600}