Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1416533003:
Handle corrupted data in Telemetry WebRtcRendering (Closed)
|
Created:
5 years, 2 months ago by cpaulin (no longer in chrome) Modified:
5 years, 2 months ago Reviewers:
nednguyen CC:
chromium-reviews, telemetry-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionHandle corrupted data in Telemetry WebRTcRendering
BUG=chromium:545165
Committed: https://crrev.com/91962356aaecdc436a2aed5d68133041731da374
Cr-Commit-Position: refs/heads/master@{#355120}
Patch Set 1 #Patch Set 2 : Added Unit test to make sure fix is solid #
Total comments: 7
Patch Set 3 : Minor fixes to unittest #
Total comments: 2
Patch Set 4 : Added more checks to testCorruptData test #
Messages
Total messages: 26 (8 generated)
Description was changed from ========== Handle corrupted data in Telemetry WebRTcRendering BUG=chromium:545165 ========== to ========== Handle corrupted data in Telemetry WebRTcRendering BUG=chromium:545165 ==========
cpaulin@chromium.org changed reviewers: + nednguyen@chromium.org
cpaulin@chromium.org changed reviewers: + nednguyen@google.com - nednguyen@chromium.org
Hi Ned, Here is the zero division fix that deals with corrupted data Thanks Christian
The CQ bit was checked by cpaulin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1416533003/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1416533003/1
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: No L-G-T-M from a valid reviewer yet. Only full committers are accepted. Even if an L-G-T-M may have been provided, it was from a non-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.
https://codereview.chromium.org/1416533003/diff/20001/tools/telemetry/telemet... File tools/telemetry/telemetry/web_perf/metrics/webrtc_rendering_stats_unittest.py (right): https://codereview.chromium.org/1416533003/diff/20001/tools/telemetry/telemet... tools/telemetry/telemetry/web_perf/metrics/webrtc_rendering_stats_unittest.py:117: def _GetCorruptEvents(self): Can you add comment explain why these events are corrupted? https://codereview.chromium.org/1416533003/diff/20001/tools/telemetry/telemet... tools/telemetry/telemetry/web_perf/metrics/webrtc_rendering_stats_unittest.py:260: self.assertEqual(expected_invalid_data, stats.invalid_data) nits: you can just do "self.assertTrue(stats.invalid_data)" instead https://codereview.chromium.org/1416533003/diff/20001/tools/telemetry/telemet... File tools/telemetry/telemetry/web_perf/metrics/webrtc_rendering_timeline.py (right): https://codereview.chromium.org/1416533003/diff/20001/tools/telemetry/telemet... tools/telemetry/telemetry/web_perf/metrics/webrtc_rendering_timeline.py:53: none_reason = 'WebMediaPlayerMS data is corrupted.' Note that using none_value_reason require that the value of Value(..) object must also be none. Since you use the same |none_reason| variable for all the values below, does that mean all members of |rendering_stats| are None when |rendering_stats.invalid_data| is True?
On 2015/10/19 22:28:31, nednguyen wrote: > https://codereview.chromium.org/1416533003/diff/20001/tools/telemetry/telemet... > File > tools/telemetry/telemetry/web_perf/metrics/webrtc_rendering_stats_unittest.py > (right): > > https://codereview.chromium.org/1416533003/diff/20001/tools/telemetry/telemet... > tools/telemetry/telemetry/web_perf/metrics/webrtc_rendering_stats_unittest.py:117: > def _GetCorruptEvents(self): > Can you add comment explain why these events are corrupted? > > https://codereview.chromium.org/1416533003/diff/20001/tools/telemetry/telemet... > tools/telemetry/telemetry/web_perf/metrics/webrtc_rendering_stats_unittest.py:260: > self.assertEqual(expected_invalid_data, stats.invalid_data) > nits: you can just do "self.assertTrue(stats.invalid_data)" instead > > https://codereview.chromium.org/1416533003/diff/20001/tools/telemetry/telemet... > File tools/telemetry/telemetry/web_perf/metrics/webrtc_rendering_timeline.py > (right): > > https://codereview.chromium.org/1416533003/diff/20001/tools/telemetry/telemet... > tools/telemetry/telemetry/web_perf/metrics/webrtc_rendering_timeline.py:53: > none_reason = 'WebMediaPlayerMS data is corrupted.' > Note that using none_value_reason require that the value of Value(..) object > must also be none. Since you use the same |none_reason| variable for all the > values below, does that mean all members of |rendering_stats| are None when > |rendering_stats.invalid_data| is True? Yes indeed, the reason is that because 'Ideal Render Instant' is zero, there is no way to calculate any metric (essentially cadence = []), so stats = TimeStats() means all stats are set to None, except the stats.invalid_data flag, and that flag is not used in the results.
Ned, I addressed your comments and published patch set 3 https://codereview.chromium.org/1416533003/diff/20001/tools/telemetry/telemet... File tools/telemetry/telemetry/web_perf/metrics/webrtc_rendering_stats_unittest.py (right): https://codereview.chromium.org/1416533003/diff/20001/tools/telemetry/telemet... tools/telemetry/telemetry/web_perf/metrics/webrtc_rendering_stats_unittest.py:117: def _GetCorruptEvents(self): On 2015/10/19 22:28:31, nednguyen wrote: > Can you add comment explain why these events are corrupted? Sounds good. https://codereview.chromium.org/1416533003/diff/20001/tools/telemetry/telemet... tools/telemetry/telemetry/web_perf/metrics/webrtc_rendering_stats_unittest.py:260: self.assertEqual(expected_invalid_data, stats.invalid_data) On 2015/10/19 22:28:31, nednguyen wrote: > nits: you can just do "self.assertTrue(stats.invalid_data)" instead Acknowledged.
https://codereview.chromium.org/1416533003/diff/20001/tools/telemetry/telemet... File tools/telemetry/telemetry/web_perf/metrics/webrtc_rendering_timeline.py (right): https://codereview.chromium.org/1416533003/diff/20001/tools/telemetry/telemet... tools/telemetry/telemetry/web_perf/metrics/webrtc_rendering_timeline.py:53: none_reason = 'WebMediaPlayerMS data is corrupted.' On 2015/10/19 22:28:31, nednguyen wrote: > Note that using none_value_reason require that the value of Value(..) object > must also be none. Since you use the same |none_reason| variable for all the > values below, does that mean all members of |rendering_stats| are None when > |rendering_stats.invalid_data| is True? Ping
On 2015/10/19 23:04:13, nednguyen wrote: > https://codereview.chromium.org/1416533003/diff/20001/tools/telemetry/telemet... > File tools/telemetry/telemetry/web_perf/metrics/webrtc_rendering_timeline.py > (right): > > https://codereview.chromium.org/1416533003/diff/20001/tools/telemetry/telemet... > tools/telemetry/telemetry/web_perf/metrics/webrtc_rendering_timeline.py:53: > none_reason = 'WebMediaPlayerMS data is corrupted.' > On 2015/10/19 22:28:31, nednguyen wrote: > > Note that using none_value_reason require that the value of Value(..) object > > must also be none. Since you use the same |none_reason| variable for all the > > values below, does that mean all members of |rendering_stats| are None when > > |rendering_stats.invalid_data| is True? > > Ping Ned, when I call TimeStats() all the values are set to none, except the invalid_data flag which is not saved in the results and therefore does not count.
Pong https://codereview.chromium.org/1416533003/diff/20001/tools/telemetry/telemet... File tools/telemetry/telemetry/web_perf/metrics/webrtc_rendering_timeline.py (right): https://codereview.chromium.org/1416533003/diff/20001/tools/telemetry/telemet... tools/telemetry/telemetry/web_perf/metrics/webrtc_rendering_timeline.py:53: none_reason = 'WebMediaPlayerMS data is corrupted.' On 2015/10/19 23:04:13, nednguyen wrote: > On 2015/10/19 22:28:31, nednguyen wrote: > > Note that using none_value_reason require that the value of Value(..) object > > must also be none. Since you use the same |none_reason| variable for all the > > values below, does that mean all members of |rendering_stats| are None when > > |rendering_stats.invalid_data| is True? > > Ping Looks like my comment was not saved: new TimeStats() gives a TimeStats object populated with None values, except the invalid_data member which is not a metrics value and not outputted to results file, so it is safe to say that the rule is not broken.
https://codereview.chromium.org/1416533003/diff/40001/tools/telemetry/telemet... File tools/telemetry/telemetry/web_perf/metrics/webrtc_rendering_stats_unittest.py (right): https://codereview.chromium.org/1416533003/diff/40001/tools/telemetry/telemet... tools/telemetry/telemetry/web_perf/metrics/webrtc_rendering_stats_unittest.py:263: self.assertTrue(stats.invalid_data) According to your comment, we should also add self.assertIsNone(stats.drift_time), self.assertIsNone(stats.rendering_length_error),...
On 2015/10/19 23:31:28, nednguyen wrote: > https://codereview.chromium.org/1416533003/diff/40001/tools/telemetry/telemet... > File > tools/telemetry/telemetry/web_perf/metrics/webrtc_rendering_stats_unittest.py > (right): > > https://codereview.chromium.org/1416533003/diff/40001/tools/telemetry/telemet... > tools/telemetry/telemetry/web_perf/metrics/webrtc_rendering_stats_unittest.py:263: > self.assertTrue(stats.invalid_data) > According to your comment, we should also add > self.assertIsNone(stats.drift_time), > self.assertIsNone(stats.rendering_length_error),... Oh yeah, you are right thanks.
Here is patch set 4. https://codereview.chromium.org/1416533003/diff/40001/tools/telemetry/telemet... File tools/telemetry/telemetry/web_perf/metrics/webrtc_rendering_stats_unittest.py (right): https://codereview.chromium.org/1416533003/diff/40001/tools/telemetry/telemet... tools/telemetry/telemetry/web_perf/metrics/webrtc_rendering_stats_unittest.py:263: self.assertTrue(stats.invalid_data) On 2015/10/19 23:31:28, nednguyen wrote: > According to your comment, we should also add > self.assertIsNone(stats.drift_time), > self.assertIsNone(stats.rendering_length_error),... You are right, new patch set 4 adresses this. Thanks
lgtm
The CQ bit was checked by cpaulin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1416533003/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1416533003/60001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by cpaulin@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1416533003/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1416533003/60001
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/91962356aaecdc436a2aed5d68133041731da374 Cr-Commit-Position: refs/heads/master@{#355120} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
