Fix Media.Video.KeyFrameDistance metric calculation

Fix Media.Video.KeyFrameDistance metric calculation

BUG=678108
TEST=manual

Committed: https://crrev.com/1b539730c8c3ce51ec2f58a3aae2a43af4f32eaa
Cr-Commit-Position: refs/heads/master@{#441381}

[whywhat, 2017-01-03 23:38:19]

avayvod@chromium.org changed reviewers:
+ sandersd@chromium.org

[whywhat, 2017-01-03 23:38:19]

Dan, PTaL

[whywhat, 2017-01-03 23:38:24]

The CQ bit was checked by avayvod@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-03 23:38:46]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[whywhat, 2017-01-03 23:40:53]

avayvod@chromium.org changed reviewers:
+ isherman@chromium.org

[whywhat, 2017-01-03 23:40:53]

+Ilya for how to handle this bug histogram wise: create a new histogram, add a
comment that data is only valid after a certain date?

[sandersd (OOO until July 31), 2017-01-03 23:41:41]

lgtm % suggestion.

https://codereview.chromium.org/2614583002/diff/1/media/filters/decoder_strea...
File media/filters/decoder_stream_traits.cc (right):

https://codereview.chromium.org/2614583002/diff/1/media/filters/decoder_strea...
media/filters/decoder_stream_traits.cc:140: last_keyframe_timestamp_ =
current_frame_timestamp;
Looks like this should just not be conditional here, rather than duplicating the
code.

[whywhat, 2017-01-04 01:00:14]

https://codereview.chromium.org/2614583002/diff/1/media/filters/decoder_strea...
File media/filters/decoder_stream_traits.cc (right):

https://codereview.chromium.org/2614583002/diff/1/media/filters/decoder_strea...
media/filters/decoder_stream_traits.cc:140: last_keyframe_timestamp_ =
current_frame_timestamp;
On 2017/01/03 at 23:41:41, sandersd wrote:
> Looks like this should just not be conditional here, rather than duplicating
the code.

You mean something like:

if (!last_kf_ts_.is_zero()) {
  UMA_HISTOGRAM_MEDIUM_TIMES(...);
}

last_kf_ts_ = cur_f_ts;

[commit-bot: I haz the power, 2017-01-04 01:19:43]

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

[commit-bot: I haz the power, 2017-01-04 01:19:43]

Dry run: This issue passed the CQ dry run.

[whywhat, 2017-01-04 15:44:26]

avayvod@chromium.org changed reviewers:
- isherman@chromium.org

[whywhat, 2017-01-04 15:44:27]

Seems like we're fine just ignoring the data from before the updated release is
rolled out and we did it before for other metrics without creating a new name,
etc. Submitting then.

[whywhat, 2017-01-04 15:44:31]

The CQ bit was checked by avayvod@chromium.org

[commit-bot: I haz the power, 2017-01-04 15:44:52]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-04 15:50:22]

CQ is committing da patch.

Bot data: {"patchset_id": 1, "attempt_start_ts": 1483544671134560, "parent_rev":
"3be5f4bdbda5f3e001754077ee916d88c620326f", "commit_rev":
"d1235b67eb4784eb90c96fda07c55f58709355be"}

[commit-bot: I haz the power, 2017-01-04 15:50:57]

Description was changed from

==========
Fix Media.Video.KeyFrameDistance metric calculation

BUG=678108
TEST=manual
==========

to

==========
Fix Media.Video.KeyFrameDistance metric calculation

BUG=678108
TEST=manual

Review-Url: https://codereview.chromium.org/2614583002
==========

[commit-bot: I haz the power, 2017-01-04 15:50:58]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2017-01-04 15:54:05]

Description was changed from

==========
Fix Media.Video.KeyFrameDistance metric calculation

BUG=678108
TEST=manual

Review-Url: https://codereview.chromium.org/2614583002
==========

to

==========
Fix Media.Video.KeyFrameDistance metric calculation

BUG=678108
TEST=manual

Committed: https://crrev.com/1b539730c8c3ce51ec2f58a3aae2a43af4f32eaa
Cr-Commit-Position: refs/heads/master@{#441381}
==========

[commit-bot: I haz the power, 2017-01-04 15:54:06]

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

[Ilya Sherman, 2017-01-07 00:53:54]

On 2017/01/04 15:44:27, whywhat wrote:
> Seems like we're fine just ignoring the data from before the updated release
is
> rolled out and we did it before for other metrics without creating a new name,
> etc. Submitting then.

[Ilya Sherman, 2017-01-07 00:55:21]

On 2017/01/04 15:44:27, whywhat wrote:
> Seems like we're fine just ignoring the data from before the updated release
is
> rolled out and we did it before for other metrics without creating a new name,
> etc. Submitting then.

FWIW, the metrics team does recommend renaming histograms for each change in
semantics, or at least adding documentation about what Chrome versions are
uploading trustworthy or untrustworthy data.  But, we won't mandate this if your
team has a different preference.  Just keep in mind that somebody outside of
your team, or perhaps a new team member, might encounter this histogram and not
know about the bug, and be confused.