Report timing frames info in GetStats.

Report timing frames info in GetStats.

Some frames are already marked as 'timing frames' via video-timing RTP header extension. Timestamps along full WebRTC pipeline are gathered for these frames. This CL implements reporting of these timestamps for a single 
timing frame since the last GetStats(). The frame with the longest end-to-end delay between two consecutive GetStats calls is reported.

The purpose of this timing information is not to provide a realtime statistics but to provide debugging information as it will help identify problematic places in video pipeline for outliers (frames which took longest to process). 

BUG=webrtc:7594
Review-Url: https://codereview.webrtc.org/2946413002
Cr-Commit-Position: refs/heads/master@{#18909}
Committed: https://chromium.googlesource.com/external/webrtc/+/2edc6845ac12a7216aeb84d9ad9da8260e2bb434

[ilnik, 2017-06-22 09:48:55]

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

[commit-bot: I haz the power, 2017-06-22 09:48:58]

Dry run: CQ is trying da patch.

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

[ilnik, 2017-06-22 10:09:29]

ilnik@webrtc.org changed reviewers:
+ asapersson@webrtc.org, deadbeef@webrtc.org, sprang@webrtc.org

[commit-bot: I haz the power, 2017-06-22 10:52:02]

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

[commit-bot: I haz the power, 2017-06-22 10:52:03]

Dry run: This issue passed the CQ dry run.

[Taylor Brandstetter, 2017-06-25 20:48:02]

deadbeef@webrtc.org changed reviewers:
+ hbos@webrtc.org

[Taylor Brandstetter, 2017-06-25 20:48:04]

+hbos

Added some minor comments, but my general concern is that this goes against the
general stats guidelines
(https://w3c.github.io/webrtc-stats/#guidelines-for-implementing-stats-objects),
and only adds the stats to the deprecated implementation of getStats.

The working group has generally decided against stats that are "max/average
value in between two getStats calls." Instead, we have increasing counters (like
totalEncodeTime:
https://w3c.github.io/webrtc-stats/#dom-rtcoutboundrtpstreamstats-totalencode...),
such that the application consuming the stats can take the difference between
any two points and compute the average per-frame.

So, is there a reason why we can't follow the pattern of totalEncodeTime, and
add totalCaptureTime, totalPacketizationTime, etc.? Or is the plan to add the
TimingFrameInfo stuff to the legacy stats implementation first, gather more
data, then work on standardizing? Sorry if I'm just out of the loop.

Also: it would be nice if the CL description provided enough detail that someone
without access to the design doc could understand what's going on.

https://codereview.webrtc.org/2946413002/diff/1/webrtc/api/statstypes.h
File webrtc/api/statstypes.h (right):

https://codereview.webrtc.org/2946413002/diff/1/webrtc/api/statstypes.h#newco...
webrtc/api/statstypes.h:213: kStatsValueNameTimingFrameInfo,
Can you add a comment saying that this value is the result of
TimingFrameInfo::ToString?

https://codereview.webrtc.org/2946413002/diff/1/webrtc/api/video/video_timing.cc
File webrtc/api/video/video_timing.cc (right):

https://codereview.webrtc.org/2946413002/diff/1/webrtc/api/video/video_timing...
webrtc/api/video/video_timing.cc:17: 
nit: Extra newline

https://codereview.webrtc.org/2946413002/diff/1/webrtc/api/video/video_timing...
webrtc/api/video/video_timing.cc:34: render_time_ms(-1) {}
Can we use rtc::Optional instead of using -1 as a special value? Also, these
default values can be put in the header

https://codereview.webrtc.org/2946413002/diff/1/webrtc/api/video/video_timing.h
File webrtc/api/video/video_timing.h (right):

https://codereview.webrtc.org/2946413002/diff/1/webrtc/api/video/video_timing...
webrtc/api/video/video_timing.h:50: struct TimingFrameInfo {
Can you add a comment (here, or elsewhere in the api/ directory) explaining how
this information is derived? And also point out that it's reset every time
GetStats is called, which is important.

https://codereview.webrtc.org/2946413002/diff/1/webrtc/api/video/video_timing...
webrtc/api/video/video_timing.h:57: // Returns if current frame took longer to
process  than |other| frame.
nit: "Returns true if...", extra space between "process  than".

https://codereview.webrtc.org/2946413002/diff/1/webrtc/api/video/video_timing...
webrtc/api/video/video_timing.h:73: int64_t receive_finish_ms;
Some of these (mostly the 4 preceding this comment) don't have very obvious
meanings. Can you add comments?

https://codereview.webrtc.org/2946413002/diff/1/webrtc/media/base/mediachannel.h
File webrtc/media/base/mediachannel.h (right):

https://codereview.webrtc.org/2946413002/diff/1/webrtc/media/base/mediachanne...
webrtc/media/base/mediachannel.h:798: // Timing frame info.
This comment doesn't add anything.

https://codereview.webrtc.org/2946413002/diff/1/webrtc/video/receive_statisti...
File webrtc/video/receive_statistics_proxy.cc (right):

https://codereview.webrtc.org/2946413002/diff/1/webrtc/video/receive_statisti...
webrtc/video/receive_statistics_proxy.cc:376: // Reset reported value to be
overwritten by new values.
nit: This comment says what the code does, but not why; I'd copy (or point to)
the comment below instead.

https://codereview.webrtc.org/2946413002/diff/1/webrtc/video/receive_statisti...
webrtc/video/receive_statistics_proxy.cc:442: // Only the frame which was
processed the longest is reported since last
nit: "since the last..."

https://codereview.webrtc.org/2946413002/diff/1/webrtc/video/receive_statisti...
webrtc/video/receive_statistics_proxy.cc:443: // GetStats() call. Therefore,
only it's stored here.
I don't understand the "Therefore, only it's stored here" part.

[ilnik, 2017-06-26 08:24:14]

On 2017/06/25 20:48:04, Taylor Brandstetter wrote:
> +hbos
> 
> Added some minor comments, but my general concern is that this goes against
the
> general stats guidelines
>
(https://w3c.github.io/webrtc-stats/#guidelines-for-implementing-stats-objects),
> and only adds the stats to the deprecated implementation of getStats.
> 
> The working group has generally decided against stats that are "max/average
> value in between two getStats calls." Instead, we have increasing counters
(like
> totalEncodeTime:
>
https://w3c.github.io/webrtc-stats/#dom-rtcoutboundrtpstreamstats-totalencode...),
> such that the application consuming the stats can take the difference between
> any two points and compute the average per-frame.
> 
> So, is there a reason why we can't follow the pattern of totalEncodeTime, and
> add totalCaptureTime, totalPacketizationTime, etc.? Or is the plan to add the
> TimingFrameInfo stuff to the legacy stats implementation first, gather more
> data, then work on standardizing? Sorry if I'm just out of the loop.
> 
> Also: it would be nice if the CL description provided enough detail that
someone
> without access to the design doc could understand what's going on.

I understand, that it's against guidelines. I thought it was advised not to even
touch old GetStats. But, some downstream projects push very hard for having
these metrics. Thus, we decided to add this info to old get stats only. Finally,
in a distant and bright future, some separate API will be implemented for that
metric (per frame callback API). I don't think that reporting timestamps in a
cumulative way is a good idea. I will update the description of this CL to be
more detailed. The reason for having a timestamps instead of totalXXXTime, is
that we have 3 separate clocks here (sender, network, receiver). Eventually they
will be synchronized, but before that we can't provide some differences. Also,
idea is to provide info about a one specific worse frame, so calculating
averages here is not applicable.

[ilnik, 2017-06-26 08:46:38]

https://codereview.webrtc.org/2946413002/diff/1/webrtc/api/statstypes.h
File webrtc/api/statstypes.h (right):

https://codereview.webrtc.org/2946413002/diff/1/webrtc/api/statstypes.h#newco...
webrtc/api/statstypes.h:213: kStatsValueNameTimingFrameInfo,
On 2017/06/25 20:48:03, Taylor Brandstetter wrote:
> Can you add a comment saying that this value is the result of
> TimingFrameInfo::ToString?

Done.

https://codereview.webrtc.org/2946413002/diff/1/webrtc/api/video/video_timing.cc
File webrtc/api/video/video_timing.cc (right):

https://codereview.webrtc.org/2946413002/diff/1/webrtc/api/video/video_timing...
webrtc/api/video/video_timing.cc:17: 
On 2017/06/25 20:48:03, Taylor Brandstetter wrote:
> nit: Extra newline

Done.

https://codereview.webrtc.org/2946413002/diff/1/webrtc/api/video/video_timing...
webrtc/api/video/video_timing.cc:34: render_time_ms(-1) {}
On 2017/06/25 20:48:03, Taylor Brandstetter wrote:
> Can we use rtc::Optional instead of using -1 as a special value? Also, these
> default values can be put in the header

Since some default values will have to be reported, we would have to convert
these empty optionals to -1 anyway in the end. Thus I think it's better not to
do so.

https://codereview.webrtc.org/2946413002/diff/1/webrtc/api/video/video_timing.h
File webrtc/api/video/video_timing.h (right):

https://codereview.webrtc.org/2946413002/diff/1/webrtc/api/video/video_timing...
webrtc/api/video/video_timing.h:50: struct TimingFrameInfo {
On 2017/06/25 20:48:03, Taylor Brandstetter wrote:
> Can you add a comment (here, or elsewhere in the api/ directory) explaining
how
> this information is derived? And also point out that it's reset every time
> GetStats is called, which is important.

Done.

https://codereview.webrtc.org/2946413002/diff/1/webrtc/api/video/video_timing...
webrtc/api/video/video_timing.h:57: // Returns if current frame took longer to
process  than |other| frame.
On 2017/06/25 20:48:03, Taylor Brandstetter wrote:
> nit: "Returns true if...", extra space between "process  than".

Done.

https://codereview.webrtc.org/2946413002/diff/1/webrtc/api/video/video_timing...
webrtc/api/video/video_timing.h:73: int64_t receive_finish_ms;
On 2017/06/25 20:48:03, Taylor Brandstetter wrote:
> Some of these (mostly the 4 preceding this comment) don't have very obvious
> meanings. Can you add comments?

Done.

https://codereview.webrtc.org/2946413002/diff/1/webrtc/media/base/mediachannel.h
File webrtc/media/base/mediachannel.h (right):

https://codereview.webrtc.org/2946413002/diff/1/webrtc/media/base/mediachanne...
webrtc/media/base/mediachannel.h:798: // Timing frame info.
On 2017/06/25 20:48:03, Taylor Brandstetter wrote:
> This comment doesn't add anything.

Done.

https://codereview.webrtc.org/2946413002/diff/1/webrtc/video/receive_statisti...
File webrtc/video/receive_statistics_proxy.cc (right):

https://codereview.webrtc.org/2946413002/diff/1/webrtc/video/receive_statisti...
webrtc/video/receive_statistics_proxy.cc:376: // Reset reported value to be
overwritten by new values.
On 2017/06/25 20:48:04, Taylor Brandstetter wrote:
> nit: This comment says what the code does, but not why; I'd copy (or point to)
> the comment below instead.

revised.

https://codereview.webrtc.org/2946413002/diff/1/webrtc/video/receive_statisti...
webrtc/video/receive_statistics_proxy.cc:442: // Only the frame which was
processed the longest is reported since last
On 2017/06/25 20:48:03, Taylor Brandstetter wrote:
> nit: "since the last..."

Done.

https://codereview.webrtc.org/2946413002/diff/1/webrtc/video/receive_statisti...
webrtc/video/receive_statistics_proxy.cc:443: // GetStats() call. Therefore,
only it's stored here.
On 2017/06/25 20:48:04, Taylor Brandstetter wrote:
> I don't understand the "Therefore, only it's stored here" part.

Revised.

[ilnik, 2017-06-26 08:49:32]

Description was changed from

==========
Report timing frames info in GetStats.

BUG=webrtc:7594
==========

to

==========
Report timing frames info in GetStats.

Some frames are already marked as 'timing frames' via video-timing RTP header
extension. Timestamps along full WebRTC pipeline are gathered for these frames.
This CL implements reporting of these timestamps for a single 
timing frame since the last GetStats(). The frame with the longest end-to-end
delay between two consecutive GetStats calls is reported 

BUG=webrtc:7594
==========

[Taylor Brandstetter, 2017-06-27 07:01:38]

Thanks for explaining; lgtm for the code I'm an owner of (api, pc, media)

https://codereview.webrtc.org/2946413002/diff/1/webrtc/api/video/video_timing.cc
File webrtc/api/video/video_timing.cc (right):

https://codereview.webrtc.org/2946413002/diff/1/webrtc/api/video/video_timing...
webrtc/api/video/video_timing.cc:34: render_time_ms(-1) {}
On 2017/06/26 08:46:38, ilnik wrote:
> On 2017/06/25 20:48:03, Taylor Brandstetter wrote:
> > Can we use rtc::Optional instead of using -1 as a special value? Also, these
> > default values can be put in the header
> 
> Since some default values will have to be reported, we would have to convert
> these empty optionals to -1 anyway in the end. Thus I think it's better not to
> do so.

That seems like an implementation detail of the ToString method, so I'd still
prefer if the rest of the code was unaware of it. But it's not worth blocking
the CL over.

[ilnik, 2017-06-27 07:41:34]

ilnik@webrtc.org changed reviewers:
+ stefan@webrtc.org
- hbos@webrtc.org

[ilnik, 2017-06-27 07:47:23]

+stefan for webrtc/common_types.h, webrtc/test/fuzzerws/rtp_packet_fuzzer.cc and
webrtc/video_receive_stream.h

Asa, please take a look at modules/ it's mostly a simple refactoring there.

Erik, please review video/*.

[stefan-webrtc, 2017-06-27 13:11:03]

lgtm for my parts.

[hbos, 2017-06-27 14:08:29]

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

[hbos, 2017-06-27 14:08:31]

https://codereview.webrtc.org/2946413002/diff/20001/webrtc/api/video/video_ti...
File webrtc/api/video/video_timing.cc (right):

https://codereview.webrtc.org/2946413002/diff/20001/webrtc/api/video/video_ti...
webrtc/api/video/video_timing.cc:12: #define WEBRTC_API_VIDEO_VIDEO_TIMING_CC_
No need for #ifndef in a .cc file.

https://codereview.webrtc.org/2946413002/diff/20001/webrtc/api/video/video_ti...
webrtc/api/video/video_timing.cc:33: render_time_ms(-1) {}
If a value is optional please prefer rtc::Optional to special values.

If all or no values are set, perhaps the Optional should be where this class is
used (rtc::Optional<TimingFrameInfo>) as opposed to on a per-timestamp basis.

https://codereview.webrtc.org/2946413002/diff/20001/webrtc/api/video/video_ti...
webrtc/api/video/video_timing.cc:52: return out.str();
Not sure if to DCHECK that all values are set or use a string like "N/A" or -1
for missing values. Is this expected to be used if missing?

https://codereview.webrtc.org/2946413002/diff/20001/webrtc/api/video/video_ti...
File webrtc/api/video/video_timing.h (right):

https://codereview.webrtc.org/2946413002/diff/20001/webrtc/api/video/video_ti...
webrtc/api/video/video_timing.h:60: int64_t EndToEndDelay() const;
Generally speaking, is there any way to know if the timestamps are synchronized?
Is this only useful an application-specific contexts where you know that peers
all synchronize to the same NTP servers?

https://codereview.webrtc.org/2946413002/diff/20001/webrtc/api/video/video_ti...
webrtc/api/video/video_timing.h:70: int64_t capture_time_ms;
Are these relative to session or relative to unix epoch, etc? RTP or NTP? Can
you add a comment and/or clarify the names.

https://codereview.webrtc.org/2946413002/diff/20001/webrtc/api/video/video_ti...
webrtc/api/video/video_timing.h:76: int64_t network2_timestamp_ms;  // Meaning
is application specific.
nit: If comment spans more than one line add it to before the variable instead
of on the right side of it.

https://codereview.webrtc.org/2946413002/diff/20001/webrtc/api/video/video_ti...
webrtc/api/video/video_timing.h:81: int64_t render_time_ms;  // Inferred smooth
render time.
What does "Inferred smooth render time" mean?

https://codereview.webrtc.org/2946413002/diff/20001/webrtc/media/base/mediach...
File webrtc/media/base/mediachannel.h (right):

https://codereview.webrtc.org/2946413002/diff/20001/webrtc/media/base/mediach...
webrtc/media/base/mediachannel.h:800: webrtc::TimingFrameInfo timing_frame_info;
Should this be rtc::Optional<webrtc::TimingFrameInfo>?

What is required in order to obtain this metric? That an extension header is
present? No need to have both sending and receiving streams?

https://codereview.webrtc.org/2946413002/diff/20001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/generic_decoder.cc (right):

https://codereview.webrtc.org/2946413002/diff/20001/webrtc/modules/video_codi...
webrtc/modules/video_coding/generic_decoder.cc:95: int64_t capture_time_ms =
decodedImage.ntp_time_ms() - ntp_offset_;
What does |capture_time_ms| imply if |decodedImage.ntp_time_ms()| is negative /
clock not estimated yet?

https://codereview.webrtc.org/2946413002/diff/20001/webrtc/modules/video_codi...
webrtc/modules/video_coding/generic_decoder.cc:107: // negative to indicate
that. Yet they still should be relatively correct.
Can you document the meaning of negative values in VideoSendTiming or use a bool
to indicate is_clock_estimated.

https://codereview.webrtc.org/2946413002/diff/20001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/timing.h (right):

https://codereview.webrtc.org/2946413002/diff/20001/webrtc/modules/video_codi...
webrtc/modules/video_coding/timing.h:137: TimingFrameInfo timing_frame_info_
GUARDED_BY(crit_sect_);
rtc::Optional<>?

https://codereview.webrtc.org/2946413002/diff/20001/webrtc/pc/statscollector.cc
File webrtc/pc/statscollector.cc (right):

https://codereview.webrtc.org/2946413002/diff/20001/webrtc/pc/statscollector....
webrtc/pc/statscollector.cc:253: info.timing_frame_info.ToString());
Do not add this stat if timing_frame_info is undefined.

Does this have to rely on "since last GetStats()"? Applications may call
GetStats() in multiple places (have multiple "stats consumers"). This will make
GetStats() break for >1 stats consumer unless each consumer is aware of when the
last call was and what the result of that call was. Can this be avoided?

https://codereview.webrtc.org/2946413002/diff/20001/webrtc/video/receive_stat...
File webrtc/video/receive_statistics_proxy.cc (right):

https://codereview.webrtc.org/2946413002/diff/20001/webrtc/video/receive_stat...
webrtc/video/receive_statistics_proxy.cc:446: // stored.
Could we get the latest instead of the longest? Or something which does not rely
on "previous call to GetStats"?

[ilnik, 2017-06-27 15:23:57]

https://codereview.webrtc.org/2946413002/diff/1/webrtc/api/video/video_timing.cc
File webrtc/api/video/video_timing.cc (right):

https://codereview.webrtc.org/2946413002/diff/1/webrtc/api/video/video_timing...
webrtc/api/video/video_timing.cc:34: render_time_ms(-1) {}
On 2017/06/27 07:01:38, Taylor Brandstetter wrote:
> On 2017/06/26 08:46:38, ilnik wrote:
> > On 2017/06/25 20:48:03, Taylor Brandstetter wrote:
> > > Can we use rtc::Optional instead of using -1 as a special value? Also,
these
> > > default values can be put in the header
> > 
> > Since some default values will have to be reported, we would have to convert
> > these empty optionals to -1 anyway in the end. Thus I think it's better not
to
> > do so.
> 
> That seems like an implementation detail of the ToString method, so I'd still
> prefer if the rest of the code was unaware of it. But it's not worth blocking
> the CL over.

Acknowledged.

https://codereview.webrtc.org/2946413002/diff/20001/webrtc/api/video/video_ti...
File webrtc/api/video/video_timing.cc (right):

https://codereview.webrtc.org/2946413002/diff/20001/webrtc/api/video/video_ti...
webrtc/api/video/video_timing.cc:12: #define WEBRTC_API_VIDEO_VIDEO_TIMING_CC_
On 2017/06/27 14:08:30, hbos wrote:
> No need for #ifndef in a .cc file.

Ooops. Copy-paste failed me.

https://codereview.webrtc.org/2946413002/diff/20001/webrtc/api/video/video_ti...
webrtc/api/video/video_timing.cc:33: render_time_ms(-1) {}
On 2017/06/27 14:08:30, hbos wrote:
> If a value is optional please prefer rtc::Optional to special values.
> 
> If all or no values are set, perhaps the Optional should be where this class
is
> used (rtc::Optional<TimingFrameInfo>) as opposed to on a per-timestamp basis.
Done.

https://codereview.webrtc.org/2946413002/diff/20001/webrtc/api/video/video_ti...
webrtc/api/video/video_timing.cc:52: return out.str();
On 2017/06/27 14:08:30, hbos wrote:
> Not sure if to DCHECK that all values are set or use a string like "N/A" or -1
> for missing values. Is this expected to be used if missing?

Now, as TimingFrameInfo is wrapped in rtc::Optional where needed, here no unset
values will ever be used.

https://codereview.webrtc.org/2946413002/diff/20001/webrtc/api/video/video_ti...
File webrtc/api/video/video_timing.h (right):

https://codereview.webrtc.org/2946413002/diff/20001/webrtc/api/video/video_ti...
webrtc/api/video/video_timing.h:60: int64_t EndToEndDelay() const;
On 2017/06/27 14:08:30, hbos wrote:
> Generally speaking, is there any way to know if the timestamps are
synchronized?
> Is this only useful an application-specific contexts where you know that peers
> all synchronize to the same NTP servers?

We know when timestamps are synchronised, because they are derived from ntp
capture time of a frame, which is estimated from rtp timestamp using continious
clock synchronization via rtcp messages. We don't in any way rely on how system
synchronizes its ntp time.

https://codereview.webrtc.org/2946413002/diff/20001/webrtc/api/video/video_ti...
webrtc/api/video/video_timing.h:70: int64_t capture_time_ms;
On 2017/06/27 14:08:30, hbos wrote:
> Are these relative to session or relative to unix epoch, etc? RTP or NTP? Can
> you add a comment and/or clarify the names.

These are in some local monotonous clock. Added clarifications.

https://codereview.webrtc.org/2946413002/diff/20001/webrtc/api/video/video_ti...
webrtc/api/video/video_timing.h:76: int64_t network2_timestamp_ms;  // Meaning
is application specific.
On 2017/06/27 14:08:30, hbos wrote:
> nit: If comment spans more than one line add it to before the variable instead
> of on the right side of it.

Done.

https://codereview.webrtc.org/2946413002/diff/20001/webrtc/api/video/video_ti...
webrtc/api/video/video_timing.h:81: int64_t render_time_ms;  // Inferred smooth
render time.
On 2017/06/27 14:08:31, hbos wrote:
> What does "Inferred smooth render time" mean?

WebRTC provides intended render time for each frame, so there will be some
playout delay to provide smooth playback.

https://codereview.webrtc.org/2946413002/diff/20001/webrtc/media/base/mediach...
File webrtc/media/base/mediachannel.h (right):

https://codereview.webrtc.org/2946413002/diff/20001/webrtc/media/base/mediach...
webrtc/media/base/mediachannel.h:800: webrtc::TimingFrameInfo timing_frame_info;
On 2017/06/27 14:08:31, hbos wrote:
> Should this be rtc::Optional<webrtc::TimingFrameInfo>?
> 
> What is required in order to obtain this metric? That an extension header is
> present? No need to have both sending and receiving streams?

Now it's Optional<>. This info will be reported by design only if extension
header is present only for frames for which we know full information.

https://codereview.webrtc.org/2946413002/diff/20001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/generic_decoder.cc (right):

https://codereview.webrtc.org/2946413002/diff/20001/webrtc/modules/video_codi...
webrtc/modules/video_coding/generic_decoder.cc:95: int64_t capture_time_ms =
decodedImage.ntp_time_ms() - ntp_offset_;
On 2017/06/27 14:08:31, hbos wrote:
> What does |capture_time_ms| imply if |decodedImage.ntp_time_ms()| is negative
/
> clock not estimated yet?

ntp_time_ms() is -1 if sender clock is not estimated. Otherwise it's capture
time in ntp format in receiver clock.

https://codereview.webrtc.org/2946413002/diff/20001/webrtc/modules/video_codi...
webrtc/modules/video_coding/generic_decoder.cc:107: // negative to indicate
that. Yet they still should be relatively correct.
On 2017/06/27 14:08:31, hbos wrote:
> Can you document the meaning of negative values in VideoSendTiming or use a
bool
> to indicate is_clock_estimated.

Done.

https://codereview.webrtc.org/2946413002/diff/20001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/timing.h (right):

https://codereview.webrtc.org/2946413002/diff/20001/webrtc/modules/video_codi...
webrtc/modules/video_coding/timing.h:137: TimingFrameInfo timing_frame_info_
GUARDED_BY(crit_sect_);
On 2017/06/27 14:08:31, hbos wrote:
> rtc::Optional<>?

Done.

https://codereview.webrtc.org/2946413002/diff/20001/webrtc/pc/statscollector.cc
File webrtc/pc/statscollector.cc (right):

https://codereview.webrtc.org/2946413002/diff/20001/webrtc/pc/statscollector....
webrtc/pc/statscollector.cc:253: info.timing_frame_info.ToString());
On 2017/06/27 14:08:31, hbos wrote:
> Do not add this stat if timing_frame_info is undefined.
> 
> Does this have to rely on "since last GetStats()"? Applications may call
> GetStats() in multiple places (have multiple "stats consumers"). This will
make
> GetStats() break for >1 stats consumer unless each consumer is aware of when
the
> last call was and what the result of that call was. Can this be avoided?

Multiple stats consumers do not break anything. The purpose of this stat is to
return some single timing frame info. "Since last GetStats " restriction just
makes reports as fine grained as possible. We could return latest available
info, but it is not so interesting as most delayed frame.

https://codereview.webrtc.org/2946413002/diff/20001/webrtc/video/receive_stat...
File webrtc/video/receive_statistics_proxy.cc (right):

https://codereview.webrtc.org/2946413002/diff/20001/webrtc/video/receive_stat...
webrtc/video/receive_statistics_proxy.cc:446: // stored.
On 2017/06/27 14:08:31, hbos wrote:
> Could we get the latest instead of the longest? Or something which does not
rely
> on "previous call to GetStats"?

Can be simply latest, but this way we may loose important bit of information.
Outliers are more interesting for analysis. For each call we can report only a
single frame information. So we choose most interesting among all frames since
last call instead of latest.

[ilnik, 2017-06-27 15:24:26]

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

[commit-bot: I haz the power, 2017-06-27 15:24:29]

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-27 15:32:00]

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

[commit-bot: I haz the power, 2017-06-27 15:32:01]

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

[Taylor Brandstetter, 2017-06-28 07:27:46]

https://codereview.webrtc.org/2946413002/diff/20001/webrtc/video/receive_stat...
File webrtc/video/receive_statistics_proxy.cc (right):

https://codereview.webrtc.org/2946413002/diff/20001/webrtc/video/receive_stat...
webrtc/video/receive_statistics_proxy.cc:446: // stored.
On 2017/06/27 15:23:56, ilnik wrote:
> On 2017/06/27 14:08:31, hbos wrote:
> > Could we get the latest instead of the longest? Or something which does not
> rely
> > on "previous call to GetStats"?
> 
> Can be simply latest, but this way we may loose important bit of information.
> Outliers are more interesting for analysis. For each call we can report only a
> single frame information. So we choose most interesting among all frames since
> last call instead of latest.

If I understand correctly, the whole motivation here is to find the outliers,
since they're the frames that end up increasing latency or causing frame drops.
And for those outliers, find out what parts of the pipeline are the bottlenecks
so they can be improved. With that in mind, this makes sense. It's more of a
debugging tool than a typical "stat", which justifies it not adhering to normal
GetStats conventions. If this interpretation is correct, it would be helpful if
the CL description and/or comments made this more clear, in case someone
encounters this code later without context.

[ilnik, 2017-06-28 09:45:52]

On 2017/06/28 07:27:46, Taylor Brandstetter wrote:
>
https://codereview.webrtc.org/2946413002/diff/20001/webrtc/video/receive_stat...
> File webrtc/video/receive_statistics_proxy.cc (right):
> 
>
https://codereview.webrtc.org/2946413002/diff/20001/webrtc/video/receive_stat...
> webrtc/video/receive_statistics_proxy.cc:446: // stored.
> On 2017/06/27 15:23:56, ilnik wrote:
> > On 2017/06/27 14:08:31, hbos wrote:
> > > Could we get the latest instead of the longest? Or something which does
not
> > rely
> > > on "previous call to GetStats"?
> > 
> > Can be simply latest, but this way we may loose important bit of
information.
> > Outliers are more interesting for analysis. For each call we can report only
a
> > single frame information. So we choose most interesting among all frames
since
> > last call instead of latest.
> 
> If I understand correctly, the whole motivation here is to find the outliers,
> since they're the frames that end up increasing latency or causing frame
drops.
> And for those outliers, find out what parts of the pipeline are the
bottlenecks
> so they can be improved. With that in mind, this makes sense. It's more of a
> debugging tool than a typical "stat", which justifies it not adhering to
normal
> GetStats conventions. If this interpretation is correct, it would be helpful
if
> the CL description and/or comments made this more clear, in case someone
> encounters this code later without context.

Ok. Adding description.

[ilnik, 2017-06-28 09:47:47]

Description was changed from

==========
Report timing frames info in GetStats.

Some frames are already marked as 'timing frames' via video-timing RTP header
extension. Timestamps along full WebRTC pipeline are gathered for these frames.
This CL implements reporting of these timestamps for a single 
timing frame since the last GetStats(). The frame with the longest end-to-end
delay between two consecutive GetStats calls is reported 

BUG=webrtc:7594
==========

to

==========
Report timing frames info in GetStats.

Some frames are already marked as 'timing frames' via video-timing RTP header
extension. Timestamps along full WebRTC pipeline are gathered for these frames.
This CL implements reporting of these timestamps for a single 
timing frame since the last GetStats(). The frame with the longest end-to-end
delay between two consecutive GetStats calls is reported.

The purpose of this timing information is not to provide a realtime statistics
but to provide debugging information as it will help identify problematic places
in video pipeline for outliers (frames which took longest to process). 

BUG=webrtc:7594
==========

[sprang_webrtc, 2017-06-28 15:31:41]

Overall looks good, but I think you need to improve test coverage. Especially an
EndToEndTest, but maybe also a few simple unit tests and a media-level
integration test.

https://codereview.webrtc.org/2946413002/diff/20001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/frame_buffer2.cc (right):

https://codereview.webrtc.org/2946413002/diff/20001/webrtc/modules/video_codi...
webrtc/modules/video_coding/frame_buffer2.cc:539: TRACE_EVENT0("webrtc",
"FrameBuffer::UpdateTimingFrameInfo");
Do you a specific use case in mind for tracing this?
Feels like doing trace recording of tracing functionality as a bit overkill?

https://codereview.webrtc.org/2946413002/diff/40001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/timing.h (right):

https://codereview.webrtc.org/2946413002/diff/40001/webrtc/modules/video_codi...
webrtc/modules/video_coding/timing.h:106: void
GetTimingFrameInfo(rtc::Optional<TimingFrameInfo>* info);
I'd rather this return and optional than use an output argument

https://codereview.webrtc.org/2946413002/diff/40001/webrtc/pc/statscollector.cc
File webrtc/pc/statscollector.cc (right):

https://codereview.webrtc.org/2946413002/diff/40001/webrtc/pc/statscollector....
webrtc/pc/statscollector.cc:254: info.timing_frame_info->ToString());
nit: curly brackets for multi-line if, plus indentation?

https://codereview.webrtc.org/2946413002/diff/40001/webrtc/video/receive_stat...
File webrtc/video/receive_statistics_proxy.cc (right):

https://codereview.webrtc.org/2946413002/diff/40001/webrtc/video/receive_stat...
webrtc/video/receive_statistics_proxy.cc:378: timing_frame_info_.reset();
I think this is problematic, since the GetStats calls can come from several
places, not all of them are from the "desired" call site.
Could we break out the timing frame polling to a separate method for appending
that data? Then we have a way to explicitly reset it and don't need a mutable
member. We'd need on in VideoReceiveStream as well then

[ilnik, 2017-06-29 08:39:56]

https://codereview.webrtc.org/2946413002/diff/20001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/frame_buffer2.cc (right):

https://codereview.webrtc.org/2946413002/diff/20001/webrtc/modules/video_codi...
webrtc/modules/video_coding/frame_buffer2.cc:539: TRACE_EVENT0("webrtc",
"FrameBuffer::UpdateTimingFrameInfo");
On 2017/06/28 15:31:40, sprang_webrtc wrote:
> Do you a specific use case in mind for tracing this?
> Feels like doing trace recording of tracing functionality as a bit overkill?

Well, we have tracing for |FrameBuffer::UpdateJitterDelay| in which image i made
this method. I don't have any preferences on tracing this, so I have removed
tracing here.

https://codereview.webrtc.org/2946413002/diff/20001/webrtc/video/receive_stat...
File webrtc/video/receive_statistics_proxy.cc (right):

https://codereview.webrtc.org/2946413002/diff/20001/webrtc/video/receive_stat...
webrtc/video/receive_statistics_proxy.cc:446: // stored.
On 2017/06/28 07:27:46, Taylor Brandstetter wrote:
> On 2017/06/27 15:23:56, ilnik wrote:
> > On 2017/06/27 14:08:31, hbos wrote:
> > > Could we get the latest instead of the longest? Or something which does
not
> > rely
> > > on "previous call to GetStats"?
> > 
> > Can be simply latest, but this way we may loose important bit of
information.
> > Outliers are more interesting for analysis. For each call we can report only
a
> > single frame information. So we choose most interesting among all frames
since
> > last call instead of latest.
> 
> If I understand correctly, the whole motivation here is to find the outliers,
> since they're the frames that end up increasing latency or causing frame
drops.
> And for those outliers, find out what parts of the pipeline are the
bottlenecks
> so they can be improved. With that in mind, this makes sense. It's more of a
> debugging tool than a typical "stat", which justifies it not adhering to
normal
> GetStats conventions. If this interpretation is correct, it would be helpful
if
> the CL description and/or comments made this more clear, in case someone
> encounters this code later without context.

Done.

https://codereview.webrtc.org/2946413002/diff/40001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/timing.h (right):

https://codereview.webrtc.org/2946413002/diff/40001/webrtc/modules/video_codi...
webrtc/modules/video_coding/timing.h:106: void
GetTimingFrameInfo(rtc::Optional<TimingFrameInfo>* info);
On 2017/06/28 15:31:41, sprang_webrtc wrote:
> I'd rather this return and optional than use an output argument

Done.

https://codereview.webrtc.org/2946413002/diff/40001/webrtc/pc/statscollector.cc
File webrtc/pc/statscollector.cc (right):

https://codereview.webrtc.org/2946413002/diff/40001/webrtc/pc/statscollector....
webrtc/pc/statscollector.cc:254: info.timing_frame_info->ToString());
On 2017/06/28 15:31:41, sprang_webrtc wrote:
> nit: curly brackets for multi-line if, plus indentation?

Done.

https://codereview.webrtc.org/2946413002/diff/40001/webrtc/video/receive_stat...
File webrtc/video/receive_statistics_proxy.cc (right):

https://codereview.webrtc.org/2946413002/diff/40001/webrtc/video/receive_stat...
webrtc/video/receive_statistics_proxy.cc:378: timing_frame_info_.reset();
On 2017/06/28 15:31:41, sprang_webrtc wrote:
> I think this is problematic, since the GetStats calls can come from several
> places, not all of them are from the "desired" call site.
> Could we break out the timing frame polling to a separate method for appending
> that data? Then we have a way to explicitly reset it and don't need a mutable
> member. We'd need on in VideoReceiveStream as well then

I don't understand what do you mean. In the end, it still will be returned via
GetStats, won't it? And there are no parameters to GetStats to understand if
it's called from a 'desired' state.

One solution would be to not reset observed timing frame info at all and return
the 'longest' timing frame for whole call so far. Or we could just return the
latest one available. Some outliers may be missed this way.

[ilnik, 2017-06-29 08:41:05]

On 2017/06/28 15:31:41, sprang_webrtc wrote:
> Overall looks good, but I think you need to improve test coverage. Especially
an
> EndToEndTest, but maybe also a few simple unit tests and a media-level
> integration test.
> 

Working on adding more tests.

[ilnik, 2017-06-29 12:47:28]

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

[commit-bot: I haz the power, 2017-06-29 12:47:35]

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-29 13:40:45]

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

[commit-bot: I haz the power, 2017-06-29 13:40:46]

Dry run: This issue passed the CQ dry run.

[ilnik, 2017-06-30 08:26:29]

On 2017/06/29 08:41:05, ilnik wrote:
> On 2017/06/28 15:31:41, sprang_webrtc wrote:
> > Overall looks good, but I think you need to improve test coverage.
Especially
> an
> > EndToEndTest, but maybe also a few simple unit tests and a media-level
> > integration test.
> > 
> 
> Working on adding more tests.

Tests added.

[sprang_webrtc, 2017-06-30 21:38:21]

https://codereview.webrtc.org/2946413002/diff/40001/webrtc/video/receive_stat...
File webrtc/video/receive_statistics_proxy.cc (right):

https://codereview.webrtc.org/2946413002/diff/40001/webrtc/video/receive_stat...
webrtc/video/receive_statistics_proxy.cc:378: timing_frame_info_.reset();
On 2017/06/29 08:39:56, ilnik wrote:
> On 2017/06/28 15:31:41, sprang_webrtc wrote:
> > I think this is problematic, since the GetStats calls can come from several
> > places, not all of them are from the "desired" call site.
> > Could we break out the timing frame polling to a separate method for
appending
> > that data? Then we have a way to explicitly reset it and don't need a
mutable
> > member. We'd need on in VideoReceiveStream as well then
> 
> I don't understand what do you mean. In the end, it still will be returned via
> GetStats, won't it? And there are no parameters to GetStats to understand if
> it's called from a 'desired' state.
> 
> One solution would be to not reset observed timing frame info at all and
return
> the 'longest' timing frame for whole call so far. Or we could just return the
> latest one available. Some outliers may be missed this way.

I was thinking of direct calls to ReceiveStatisticsProxy from within the native
stack. I've seen usages like that where rather than implement a new rate tracker
or add another callback for bitrate/framrate etc we just call the stats proxies.


It'd be silly if we lost data because we had calls from vie_encode or jitter
estimator or whatnot.

https://codereview.webrtc.org/2946413002/diff/100001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/frame_buffer2.cc (right):

https://codereview.webrtc.org/2946413002/diff/100001/webrtc/modules/video_cod...
webrtc/modules/video_coding/frame_buffer2.cc:541: info =
timing_->GetTimingFrameInfo();
nit: make this a one-liner

[ilnik, 2017-07-03 08:13:59]

On 2017/06/30 21:38:21, sprang_webrtc wrote:
> I was thinking of direct calls to ReceiveStatisticsProxy from within the
native
> stack. I've seen usages like that where rather than implement a new rate
tracker
> or add another callback for bitrate/framrate etc we just call the stats
proxies.
> 
> 
> It'd be silly if we lost data because we had calls from vie_encode or jitter
> estimator or whatnot.

Well, the correct way would be to report it via a per-frame API. Since we don't
have it, we have to improvise.
Could you please give me pointers to that calls to stats proxies from within a
native stack.

[ilnik, 2017-07-03 13:43:55]

https://codereview.webrtc.org/2946413002/diff/100001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/frame_buffer2.cc (right):

https://codereview.webrtc.org/2946413002/diff/100001/webrtc/modules/video_cod...
webrtc/modules/video_coding/frame_buffer2.cc:541: info =
timing_->GetTimingFrameInfo();
On 2017/06/30 21:38:21, sprang_webrtc wrote:
> nit: make this a one-liner

Done.

[ilnik, 2017-07-03 13:46:56]

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

[commit-bot: I haz the power, 2017-07-03 13:47:03]

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-07-03 14:24:52]

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

[commit-bot: I haz the power, 2017-07-03 14:24:53]

Dry run: This issue passed the CQ dry run.

[sprang_webrtc, 2017-07-06 09:13:09]

lgtm with nit

https://codereview.webrtc.org/2946413002/diff/140001/webrtc/video/receive_sta...
File webrtc/video/receive_statistics_proxy.cc (right):

https://codereview.webrtc.org/2946413002/diff/140001/webrtc/video/receive_sta...
webrtc/video/receive_statistics_proxy.cc:409: rtc::Optional<TimingFrameInfo>
ReceiveStatisticsProxy::GetTimingFrameInfo() {
nit: Rename this method so it's clear we're mutating the state, for instance
GetAndResetTimingFrameInfo().

https://codereview.webrtc.org/2946413002/diff/140001/webrtc/video/video_recei...
File webrtc/video/video_receive_stream.cc (right):

https://codereview.webrtc.org/2946413002/diff/140001/webrtc/video/video_recei...
webrtc/video/video_receive_stream.cc:347: rtc::Optional<TimingFrameInfo>
VideoReceiveStream::GetTimingFrameInfo() {
same here

[ilnik, 2017-07-06 09:23:34]

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

[commit-bot: I haz the power, 2017-07-06 09:23: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-07-06 09:25:24]

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

[commit-bot: I haz the power, 2017-07-06 09:25:26]

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...)
  ios_api_framework on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_api_framework/build...)
  ios_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_dbg/builds/27068)
  mac_asan on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/mac_asan/builds/26058)
  mac_compile_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/mac_compile_dbg/builds/...)

[ilnik, 2017-07-06 09:28:37]

https://codereview.webrtc.org/2946413002/diff/140001/webrtc/video/receive_sta...
File webrtc/video/receive_statistics_proxy.cc (right):

https://codereview.webrtc.org/2946413002/diff/140001/webrtc/video/receive_sta...
webrtc/video/receive_statistics_proxy.cc:409: rtc::Optional<TimingFrameInfo>
ReceiveStatisticsProxy::GetTimingFrameInfo() {
On 2017/07/06 09:13:08, sprang_webrtc wrote:
> nit: Rename this method so it's clear we're mutating the state, for instance
> GetAndResetTimingFrameInfo().

Done.

https://codereview.webrtc.org/2946413002/diff/140001/webrtc/video/video_recei...
File webrtc/video/video_receive_stream.cc (right):

https://codereview.webrtc.org/2946413002/diff/140001/webrtc/video/video_recei...
webrtc/video/video_receive_stream.cc:347: rtc::Optional<TimingFrameInfo>
VideoReceiveStream::GetTimingFrameInfo() {
On 2017/07/06 09:13:08, sprang_webrtc wrote:
> same here

Done.

[ilnik, 2017-07-06 09:31:29]

The CQ bit was checked by ilnik@webrtc.org

[ilnik, 2017-07-06 09:31:29]

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

[commit-bot: I haz the power, 2017-07-06 09:31:34]

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-07-06 10:06:51]

CQ is committing da patch.

Bot data: {"patchset_id": 180001, "attempt_start_ts": 1499333489414220,
"parent_rev": "5b7fc8ce424490ccf3229c855facdef78e6cb615", "commit_rev":
"2edc6845ac12a7216aeb84d9ad9da8260e2bb434"}

[commit-bot: I haz the power, 2017-07-06 10:06:58]

Description was changed from

==========
Report timing frames info in GetStats.

Some frames are already marked as 'timing frames' via video-timing RTP header
extension. Timestamps along full WebRTC pipeline are gathered for these frames.
This CL implements reporting of these timestamps for a single 
timing frame since the last GetStats(). The frame with the longest end-to-end
delay between two consecutive GetStats calls is reported.

The purpose of this timing information is not to provide a realtime statistics
but to provide debugging information as it will help identify problematic places
in video pipeline for outliers (frames which took longest to process). 

BUG=webrtc:7594
==========

to

==========
Report timing frames info in GetStats.

Some frames are already marked as 'timing frames' via video-timing RTP header
extension. Timestamps along full WebRTC pipeline are gathered for these frames.
This CL implements reporting of these timestamps for a single 
timing frame since the last GetStats(). The frame with the longest end-to-end
delay between two consecutive GetStats calls is reported.

The purpose of this timing information is not to provide a realtime statistics
but to provide debugging information as it will help identify problematic places
in video pipeline for outliers (frames which took longest to process). 

BUG=webrtc:7594

Review-Url: https://codereview.webrtc.org/2946413002
Cr-Commit-Position: refs/heads/master@{#18909}
Committed:
https://chromium.googlesource.com/external/webrtc/+/2edc6845ac12a7216aeb84d9a...
==========

[commit-bot: I haz the power, 2017-07-06 10:07:00]

Committed patchset #10 (id:180001) as
https://chromium.googlesource.com/external/webrtc/+/2edc6845ac12a7216aeb84d9a...

[hbos, 2017-07-06 13:27:30]

https://codereview.webrtc.org/2946413002/diff/20001/webrtc/api/video/video_ti...
File webrtc/api/video/video_timing.h (right):

https://codereview.webrtc.org/2946413002/diff/20001/webrtc/api/video/video_ti...
webrtc/api/video/video_timing.h:60: int64_t EndToEndDelay() const;
On 2017/06/27 15:23:56, ilnik wrote:
> On 2017/06/27 14:08:30, hbos wrote:
> > Generally speaking, is there any way to know if the timestamps are
> synchronized?
> > Is this only useful an application-specific contexts where you know that
peers
> > all synchronize to the same NTP servers?
> 
> We know when timestamps are synchronised, because they are derived from ntp
> capture time of a frame, which is estimated from rtp timestamp using
continious
> clock synchronization via rtcp messages. We don't in any way rely on how
system
> synchronizes its ntp time.

Is this synchronization method and the video-timing RTP header extension
standards? Is end-to-end delay possible to calculate through standard-supported
means?
A stat for end-to-end delay was blocked on not being able to calculate this with
satisfactory estimates, https://github.com/w3c/webrtc-stats/issues/222.

[ilnik, 2017-07-06 14:30:56]

https://codereview.webrtc.org/2946413002/diff/20001/webrtc/api/video/video_ti...
File webrtc/api/video/video_timing.h (right):

https://codereview.webrtc.org/2946413002/diff/20001/webrtc/api/video/video_ti...
webrtc/api/video/video_timing.h:60: int64_t EndToEndDelay() const;
On 2017/07/06 13:27:29, hbos wrote:
> On 2017/06/27 15:23:56, ilnik wrote:
> > On 2017/06/27 14:08:30, hbos wrote:
> > > Generally speaking, is there any way to know if the timestamps are
> > synchronized?
> > > Is this only useful an application-specific contexts where you know that
> peers
> > > all synchronize to the same NTP servers?
> > 
> > We know when timestamps are synchronised, because they are derived from ntp
> > capture time of a frame, which is estimated from rtp timestamp using
> continious
> > clock synchronization via rtcp messages. We don't in any way rely on how
> system
> > synchronizes its ntp time.
> 
> Is this synchronization method and the video-timing RTP header extension
> standards? Is end-to-end delay possible to calculate through
standard-supported
> means?
> A stat for end-to-end delay was blocked on not being able to calculate this
with
> satisfactory estimates, https://github.com/w3c/webrtc-stats/issues/222.

I don't know about synchronization method standard. We utilize Receiver Reports
to make it. I guess, nothing non-standardized is used. And, yes, it's not very
precise. It just estimates RTT and assumes that packets take RTT/2 to go from
sender to receiver.  The timing frames extension is not standardized at all.
End-to-end delay is already calculated without blatant standards violation, but
reporting timings for all parts of a video pipeline, like i am trying to do, is
impossible with such an extension.