Cast: Logging estimates clock difference based on packets

Cast: Logging estimates clock difference based on packets

Makes the clock offset estimator more accurate and reacts faster.
This comes from issue 519163002 (but without the tracing.)

Committed: https://crrev.com/d589cbb03475c509e0c6445534b6358e982ad014
Cr-Commit-Position: refs/heads/master@{#294325}

[hubbe, 2014-09-09 00:11:55]

hubbe@chromium.org changed reviewers:
+ imcheng@chromium.org

[imcheng, 2014-09-09 01:04:23]

Thanks for the patch.

https://codereview.chromium.org/556693002/diff/1/media/cast/common/mod_util.h
File media/cast/common/mod_util.h (right):

https://codereview.chromium.org/556693002/diff/1/media/cast/common/mod_util.h...
media/cast/common/mod_util.h:5: #ifndef MEDIA_CAST_MOD_UTIL_H_
MEDIA_CAST_COMMON_MOD_UTIL_H_

https://codereview.chromium.org/556693002/diff/1/media/cast/common/mod_util.h...
media/cast/common/mod_util.h:18: typename MAP::iterator ModMapOldest(MAP* map) {
Can map be a const ref?

https://codereview.chromium.org/556693002/diff/1/media/cast/common/mod_util.h...
media/cast/common/mod_util.h:37: typename MAP::iterator ModMapPrevious(MAP* map,
typename MAP::iterator i) {
Can map be a const ref?

https://codereview.chromium.org/556693002/diff/1/media/cast/logging/receiver_...
File media/cast/logging/receiver_time_offset_estimator_impl.cc (right):

https://codereview.chromium.org/556693002/diff/1/media/cast/logging/receiver_...
media/cast/logging/receiver_time_offset_estimator_impl.cc:46: bound_ += (delta -
bound_) / kClockDriftSpeed;
What does this do? Does it handle drifting in either direction?

https://codereview.chromium.org/556693002/diff/1/media/cast/logging/receiver_...
media/cast/logging/receiver_time_offset_estimator_impl.cc:56: TimeTickPair
&ticks = events_[key];
const?

https://codereview.chromium.org/556693002/diff/1/media/cast/logging/receiver_...
File media/cast/logging/receiver_time_offset_estimator_impl.h (right):

https://codereview.chromium.org/556693002/diff/1/media/cast/logging/receiver_...
media/cast/logging/receiver_time_offset_estimator_impl.h:52: typedef
std::pair<base::TimeTicks, base::TimeTicks> TimeTickPair;
would it be possible to define such that the first element is always the sender
event timestamp, and second is receiver event timestamp? It seems strange that
the lower_bound_.bounds() has to be inverted before it is returned.

[hubbe, 2014-09-09 19:06:15]

https://codereview.chromium.org/556693002/diff/1/media/cast/common/mod_util.h
File media/cast/common/mod_util.h (right):

https://codereview.chromium.org/556693002/diff/1/media/cast/common/mod_util.h...
media/cast/common/mod_util.h:5: #ifndef MEDIA_CAST_MOD_UTIL_H_
On 2014/09/09 01:04:23, imcheng1 wrote:
> MEDIA_CAST_COMMON_MOD_UTIL_H_

Done.

https://codereview.chromium.org/556693002/diff/1/media/cast/common/mod_util.h...
media/cast/common/mod_util.h:18: typename MAP::iterator ModMapOldest(MAP* map) {
On 2014/09/09 01:04:23, imcheng1 wrote:
> Can map be a const ref?

It can, but then I must return a MAP::const_iterator, which is less useful.

https://codereview.chromium.org/556693002/diff/1/media/cast/common/mod_util.h...
media/cast/common/mod_util.h:37: typename MAP::iterator ModMapPrevious(MAP* map,
typename MAP::iterator i) {
On 2014/09/09 01:04:23, imcheng1 wrote:
> Can map be a const ref?

Same argument as above.

https://codereview.chromium.org/556693002/diff/1/media/cast/logging/receiver_...
File media/cast/logging/receiver_time_offset_estimator_impl.cc (right):

https://codereview.chromium.org/556693002/diff/1/media/cast/logging/receiver_...
media/cast/logging/receiver_time_offset_estimator_impl.cc:46: bound_ += (delta -
bound_) / kClockDriftSpeed;
On 2014/09/09 01:04:23, imcheng1 wrote:
> What does this do? Does it handle drifting in either direction?

Added some (hopefully helpful) comments to the class.

https://codereview.chromium.org/556693002/diff/1/media/cast/logging/receiver_...
media/cast/logging/receiver_time_offset_estimator_impl.cc:56: TimeTickPair
&ticks = events_[key];
On 2014/09/09 01:04:23, imcheng1 wrote:
> const?

Done.

https://codereview.chromium.org/556693002/diff/1/media/cast/logging/receiver_...
File media/cast/logging/receiver_time_offset_estimator_impl.h (right):

https://codereview.chromium.org/556693002/diff/1/media/cast/logging/receiver_...
media/cast/logging/receiver_time_offset_estimator_impl.h:52: typedef
std::pair<base::TimeTicks, base::TimeTicks> TimeTickPair;
On 2014/09/09 01:04:23, imcheng1 wrote:
> would it be possible to define such that the first element is always the
sender
> event timestamp, and second is receiver event timestamp? It seems strange that
> the lower_bound_.bounds() has to be inverted before it is returned.

I find it far less confusing to have the events ordered in time.
(a always occurs before b)
Changing it means making BoundCalculator into a template which takes a
comparison function as argument, which also confuses the code I think.

[imcheng, 2014-09-09 19:08:47]

lgtm

[hubbe, 2014-09-09 19:11:31]

The CQ bit was checked by hubbe@chromium.org

[commit-bot: I haz the power, 2014-09-09 19:14:01]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/hubbe@chromium.org/556693002/20001

[commit-bot: I haz the power, 2014-09-09 19:14:02]

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

[commit-bot: I haz the power, 2014-09-09 19:14:03]

No LGTM from a valid reviewer yet. Only full committers are accepted.
Even if an LGTM may have been provided, it was from a non-committer or
a provisional committer, _not_ a full super star committer.
See http://www.chromium.org/getting-involved/become-a-committer
Note that this has nothing to do with OWNERS files.

[hubbe, 2014-09-09 20:46:55]

hubbe@chromium.org changed reviewers:
+ miu@chromium.org

[hubbe, 2014-09-09 20:46:56]

+miu for committer review

[miu, 2014-09-10 18:55:44]

lgtm

https://codereview.chromium.org/556693002/diff/20001/media/cast/logging/recei...
File media/cast/logging/receiver_time_offset_estimator_impl.cc (left):

https://codereview.chromium.org/556693002/diff/20001/media/cast/logging/recei...
media/cast/logging/receiver_time_offset_estimator_impl.cc:50: case
FRAME_ENCODED:
Did you mean to handle this case in the new code?  Otherwise, it seems like this
behavior change should be explained in the change description.  Unless this was
a dead code path?

https://codereview.chromium.org/556693002/diff/20001/media/cast/logging/recei...
File media/cast/logging/receiver_time_offset_estimator_impl.h (right):

https://codereview.chromium.org/556693002/diff/20001/media/cast/logging/recei...
media/cast/logging/receiver_time_offset_estimator_impl.h:24: // The lower, this
is, the faster we adjusts to clock drift.
nit: s/adjusts/adjust/

[hubbe, 2014-09-10 20:43:31]

https://codereview.chromium.org/556693002/diff/20001/media/cast/logging/recei...
File media/cast/logging/receiver_time_offset_estimator_impl.cc (left):

https://codereview.chromium.org/556693002/diff/20001/media/cast/logging/recei...
media/cast/logging/receiver_time_offset_estimator_impl.cc:50: case
FRAME_ENCODED:
On 2014/09/10 18:55:43, miu wrote:
> Did you mean to handle this case in the new code?  Otherwise, it seems like
this
> behavior change should be explained in the change description.  Unless this
was
> a dead code path?

That's what "based on packets" refers to.
The new code doesn't uses "packet_sent_to_network -> packet_received" instead of
"frame_encoded -> frame_ack_sent" since that give a much tighter bound.

https://codereview.chromium.org/556693002/diff/20001/media/cast/logging/recei...
File media/cast/logging/receiver_time_offset_estimator_impl.h (right):

https://codereview.chromium.org/556693002/diff/20001/media/cast/logging/recei...
media/cast/logging/receiver_time_offset_estimator_impl.h:24: // The lower, this
is, the faster we adjusts to clock drift.
On 2014/09/10 18:55:43, miu wrote:
> nit: s/adjusts/adjust/

Done.

[hubbe, 2014-09-10 23:21:21]

The CQ bit was checked by hubbe@chromium.org

[commit-bot: I haz the power, 2014-09-10 23:49:46]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/556693002/80001

[commit-bot: I haz the power, 2014-09-11 05:36:56]

Committed patchset #5 (id:80001) as 22200387a40f1fd87d9480905286bc8c3a4582d7

[commit-bot: I haz the power, 2014-09-11 05:40:15]

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