Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2032813002:
[Chromecast] Log playback stall event. (Closed)
|
Created:
4 years, 6 months ago by ejason Modified:
4 years, 6 months ago CC:
chromium-reviews, alokp+watch_chromium.org, lcwu+watch_chromium.org, halliwell+watch_chromium.org, feature-media-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[Chromecast] Log playback stall event.
BUG=internal b/26496758
Committed: https://crrev.com/db6440ed63119748fd413c152c3f3e9868a06128
Cr-Commit-Position: refs/heads/master@{#397831}
Patch Set 1 #
Total comments: 8
Patch Set 2 : Update playback stall check to for pause/buffering conditions #
Total comments: 4
Patch Set 3 : Initialize last_media_time_ #
Total comments: 2
Patch Set 4 : Clarify units on playback stall threshold #
Messages
Total messages: 20 (8 generated)
Description was changed from ========== [Chromecast] Log playback stall event. BUG=internal b/26496758 ========== to ========== [Chromecast] Log playback stall event. BUG=internal b/26496758 ==========
jasonroberts@google.com changed reviewers: + halliwell@chromium.org, kmackay@google.com, servolk@google.com
alokp@chromium.org changed reviewers: + alokp@chromium.org
https://codereview.chromium.org/2032813002/diff/1/chromecast/media/cma/pipeli... File chromecast/media/cma/pipeline/media_pipeline_impl.cc (right): https://codereview.chromium.org/2032813002/diff/1/chromecast/media/cma/pipeli... chromecast/media/cma/pipeline/media_pipeline_impl.cc:429: base::TimeTicks current_stc) { DCHECK(media_time >= last_media_time) https://codereview.chromium.org/2032813002/diff/1/chromecast/media/cma/pipeli... chromecast/media/cma/pipeline/media_pipeline_impl.cc:436: LOG(INFO) << "Transitioning out of stalled state. Stall duration was " LOG(INFO) -> CMALOG https://codereview.chromium.org/2032813002/diff/1/chromecast/media/cma/pipeli... chromecast/media/cma/pipeline/media_pipeline_impl.cc:456: if (current_stall_duration.InMilliseconds() >= 2500) { move 2500 into a constant. https://codereview.chromium.org/2032813002/diff/1/chromecast/media/cma/pipeli... chromecast/media/cma/pipeline/media_pipeline_impl.cc:466: void MediaPipelineImpl::UpdateMediaTime() { verify that this is not scheduled when playback_rate = 0. Otherwise you will log false stall events.
PTAL https://codereview.chromium.org/2032813002/diff/1/chromecast/media/cma/pipeli... File chromecast/media/cma/pipeline/media_pipeline_impl.cc (right): https://codereview.chromium.org/2032813002/diff/1/chromecast/media/cma/pipeli... chromecast/media/cma/pipeline/media_pipeline_impl.cc:429: base::TimeTicks current_stc) { On 2016/06/02 00:09:33, alokp wrote: > DCHECK(media_time >= last_media_time) I added this DCHECK but during my local longevity tests it appears that this assumption doesn't hold (i.e., I hit the DCHECK). I didn't debug further. While I wouldn't expect this condition either, I think the initial if-condition that checks that media_time != last_media_time should handle this gracefully (it either no-ops or resets stall state and returns). Further, we only use the stc value for calculations. I did add a DCHECK for media_time != ::media::kNoTimestamp as the code as written should only be called with a valid media_time. https://codereview.chromium.org/2032813002/diff/1/chromecast/media/cma/pipeli... chromecast/media/cma/pipeline/media_pipeline_impl.cc:436: LOG(INFO) << "Transitioning out of stalled state. Stall duration was " On 2016/06/02 00:09:33, alokp wrote: > LOG(INFO) -> CMALOG Done. https://codereview.chromium.org/2032813002/diff/1/chromecast/media/cma/pipeli... chromecast/media/cma/pipeline/media_pipeline_impl.cc:456: if (current_stall_duration.InMilliseconds() >= 2500) { On 2016/06/02 00:09:33, alokp wrote: > move 2500 into a constant. Done. https://codereview.chromium.org/2032813002/diff/1/chromecast/media/cma/pipeli... chromecast/media/cma/pipeline/media_pipeline_impl.cc:466: void MediaPipelineImpl::UpdateMediaTime() { On 2016/06/02 00:09:33, alokp wrote: > verify that this is not scheduled when playback_rate = 0. Otherwise you will log > false stall events. I reviewed the code and it appears to be the case. That said, I added explicit checks to CheckForPlaybackStall() for this case and the case where we are buffering, as we have another metric for the buffering_controller case.
kmackay@chromium.org changed reviewers: + kmackay@chromium.org
https://codereview.chromium.org/2032813002/diff/20001/chromecast/media/cma/pi... File chromecast/media/cma/pipeline/media_pipeline_impl.cc (right): https://codereview.chromium.org/2032813002/diff/20001/chromecast/media/cma/pi... chromecast/media/cma/pipeline/media_pipeline_impl.cc:89: pending_time_update_task_(false), initialize last_media_time_ to kNoTimestamp https://codereview.chromium.org/2032813002/diff/20001/chromecast/media/cma/pi... chromecast/media/cma/pipeline/media_pipeline_impl.cc:432: base::TimeTicks current_stc) { what does 'stc' stand for?
https://codereview.chromium.org/2032813002/diff/20001/chromecast/media/cma/pi... File chromecast/media/cma/pipeline/media_pipeline_impl.cc (right): https://codereview.chromium.org/2032813002/diff/20001/chromecast/media/cma/pi... chromecast/media/cma/pipeline/media_pipeline_impl.cc:89: pending_time_update_task_(false), On 2016/06/03 17:37:05, kmackay wrote: > initialize last_media_time_ to kNoTimestamp Done. https://codereview.chromium.org/2032813002/diff/20001/chromecast/media/cma/pi... chromecast/media/cma/pipeline/media_pipeline_impl.cc:432: base::TimeTicks current_stc) { On 2016/06/03 17:37:05, kmackay wrote: > what does 'stc' stand for? System time clock. I simply re-used the existing nomenclature from UpdateMediaTime() (line 528). I'm open to changing this if you want, I don't feel strongly one way or the other.
lgtm % nit https://codereview.chromium.org/2032813002/diff/40001/chromecast/media/cma/pi... File chromecast/media/cma/pipeline/media_pipeline_impl.cc (right): https://codereview.chromium.org/2032813002/diff/40001/chromecast/media/cma/pi... chromecast/media/cma/pipeline/media_pipeline_impl.cc:55: constexpr int kPlaybackStallEventThreshold = 2500; Add "Ms" suffix to make the unit clear.
On 2016/06/03 19:20:15, ejason wrote: > https://codereview.chromium.org/2032813002/diff/20001/chromecast/media/cma/pi... > File chromecast/media/cma/pipeline/media_pipeline_impl.cc (right): > > https://codereview.chromium.org/2032813002/diff/20001/chromecast/media/cma/pi... > chromecast/media/cma/pipeline/media_pipeline_impl.cc:89: > pending_time_update_task_(false), > On 2016/06/03 17:37:05, kmackay wrote: > > initialize last_media_time_ to kNoTimestamp > > Done. > > https://codereview.chromium.org/2032813002/diff/20001/chromecast/media/cma/pi... > chromecast/media/cma/pipeline/media_pipeline_impl.cc:432: base::TimeTicks > current_stc) { > On 2016/06/03 17:37:05, kmackay wrote: > > what does 'stc' stand for? > > System time clock. I simply re-used the existing nomenclature from > UpdateMediaTime() (line 528). I'm open to changing this if you want, I don't > feel strongly one way or the other. lgtm
https://codereview.chromium.org/2032813002/diff/40001/chromecast/media/cma/pi... File chromecast/media/cma/pipeline/media_pipeline_impl.cc (right): https://codereview.chromium.org/2032813002/diff/40001/chromecast/media/cma/pi... chromecast/media/cma/pipeline/media_pipeline_impl.cc:55: constexpr int kPlaybackStallEventThreshold = 2500; On 2016/06/03 19:55:28, alokp wrote: > Add "Ms" suffix to make the unit clear. Done.
The CQ bit was checked by jasonroberts@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from alokp@chromium.org, kmackay@chromium.org Link to the patchset: https://codereview.chromium.org/2032813002/#ps60001 (title: "Clarify units on playback stall threshold")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2032813002/60001
Message was sent while issue was closed.
Description was changed from ========== [Chromecast] Log playback stall event. BUG=internal b/26496758 ========== to ========== [Chromecast] Log playback stall event. BUG=internal b/26496758 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== [Chromecast] Log playback stall event. BUG=internal b/26496758 ========== to ========== [Chromecast] Log playback stall event. BUG=internal b/26496758 Committed: https://crrev.com/db6440ed63119748fd413c152c3f3e9868a06128 Cr-Commit-Position: refs/heads/master@{#397831} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/db6440ed63119748fd413c152c3f3e9868a06128 Cr-Commit-Position: refs/heads/master@{#397831}
Message was sent while issue was closed.
On 2016/06/03 22:55:46, commit-bot: I haz the power wrote: > Patchset 4 (id:??) landed as > https://crrev.com/db6440ed63119748fd413c152c3f3e9868a06128 > Cr-Commit-Position: refs/heads/master@{#397831} I believe we should only log the 'stalled' event for src=/FFmpeg playback. It doesn't make sense to log this for MSE, as it doesn't provide any meaningful signals in case of MSE where the app/player controls the media buffering. See http://crbug.com/517240 for details (and https://github.com/w3c/media-source/issues/88 for proposed MSE spec change to never fire the 'stalled' events). |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
