Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2115143002:
Transition media element to HAVE_CURRENT_DATA upon underflow. (Closed)
|
Created:
4 years, 5 months ago by chcunningham Modified:
4 years, 5 months ago CC:
chromium-reviews, blink-reviews, feature-media-reviews_chromium.org, posciak+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionTransition media element to HAVE_CURRENT_DATA upon underflow.
Previously underflow would cause a pause but did not change the
readyState to HAVE_CURRENT_DATA. This also meant that we did not emit
the "waiting" event upon underflow, meaning apps were not properly
informed when playback stalls. This CL fixes all of that.
This CL fixes a layout test that was meant to cover this scenario but
did not properly check the waiting event. The test has been refactored
to use the new testharness.js.
Still missing is support for transitioning to HAVE_FUTURE_DATA. Currently
we will transition immediately from HAVE_CURRENT to HAVE_ENOUGH, skipping
HAVE_FUTURE. This is less critical, but still nice to have, especially
for MediaSource.
BUG=144683, 279213
TEST=fixed layout test
Committed: https://crrev.com/eb270c9af201e549f3e2e2b1016b6b99565f1706
Cr-Commit-Position: refs/heads/master@{#405656}
Patch Set 1 #
Total comments: 23
Patch Set 2 : Feedback #Patch Set 3 : Rebase #Patch Set 4 : Fix test expectations (no pngs) #
Total comments: 6
Patch Set 5 : Rebase #Patch Set 6 : Feedback #Messages
Total messages: 31 (11 generated)
chcunningham@chromium.org changed reviewers: + wolenetz@chromium.org
Finally got that test sorted out. PTAL
Description was changed from ========== Transition media element to HAVE_CURRENT_DATA upon underflow. Previously underflow would cause a pause but did not change the readyState to HAVE_CURRENT_DATA. This also meant that we did not emit the "waiting" event upon underflow, meaning apps were not properly informed when playback stalls. This CL fixes all of that. This CL fixes a layout test that was meant to cover this scenario but did not properly check the waiting event. The test has been refactored to use the new testharness.js. Still missing is support for transitioning to HAVE_FUTURE_DATA. Currently we will transition immediately from HAVE_CURRENT to HAVE_ENOUGH, skipping HAVE_FUTURE. This is less critical, but still nice to have, especially for MediaSource. BUG=144683 TEST=fixed layout test ========== to ========== Transition media element to HAVE_CURRENT_DATA upon underflow. Previously underflow would cause a pause but did not change the readyState to HAVE_CURRENT_DATA. This also meant that we did not emit the "waiting" event upon underflow, meaning apps were not properly informed when playback stalls. This CL fixes all of that. This CL fixes a layout test that was meant to cover this scenario but did not properly check the waiting event. The test has been refactored to use the new testharness.js. Still missing is support for transitioning to HAVE_FUTURE_DATA. Currently we will transition immediately from HAVE_CURRENT to HAVE_ENOUGH, skipping HAVE_FUTURE. This is less critical, but still nice to have, especially for MediaSource. BUG=144683,279213 TEST=fixed layout test ==========
Ping.
On 2016/07/07 20:56:26, chcunningham wrote: > Ping. Yep - I'm on it today. Just finishing email catch-up from the really long weekend vacation. Matt
wolenetz@chromium.org changed reviewers: + sandersd@chromium.org
First round of comments. I didn't fully CR the new layout test, since it's failing on multiple bots. +sandersd@ for CR'ing WMPI suspend logic verification (see comment, below). https://codereview.chromium.org/2115143002/diff/1/media/blink/webmediaplayer_... File media/blink/webmediaplayer_impl.cc (right): https://codereview.chromium.org/2115143002/diff/1/media/blink/webmediaplayer_... media/blink/webmediaplayer_impl.cc:1058: client_->timeChanged(); Is timeChanged() notification also always required on underflow? https://codereview.chromium.org/2115143002/diff/1/media/blink/webmediaplayer_... media/blink/webmediaplayer_impl.cc:1064: UpdatePlayState(); Does underflow allow us to suspend if idle (stalled) long enough? From chat with sandersd@, the behavior expectation is that (barring being in the middle of a seek), an underflow should enable suspend to occur if underflowed long enough. Please add appropriate wmpi_unittest(s) to verify underflow during, and not during, seek, results on suspend behavior. +sandersd@ to CR that portion of this CL, too. https://codereview.chromium.org/2115143002/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/http/tests/media/video-play-stall.html (right): https://codereview.chromium.org/2115143002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/http/tests/media/video-play-stall.html:1: <video id="test_video"></video> {linux,win}_chromium_rel_ng bots didn't pass this test https://codereview.chromium.org/2115143002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/http/tests/media/video-play-stall.html:7: async_test(t => { nit: is the => {... consistent with other tests? Why not async_test(t) {...} ? https://codereview.chromium.org/2115143002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/http/tests/media/video-play-stall.html:7: async_test(t => { nit: left justify 0-margin the outermost portion of this <script> body (w3c wpt-style, or so I'm told). https://codereview.chromium.org/2115143002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/http/tests/media/video-play-stall.html:13: let ReadyState = { These are already exposed on the IDL for HTMLMediaElement.
On 2016/07/07 21:32:03, wolenetz wrote: > First round of comments. I didn't fully CR the new layout test, since it's > failing on multiple bots. > > +sandersd@ for CR'ing WMPI suspend logic verification (see comment, below). > > https://codereview.chromium.org/2115143002/diff/1/media/blink/webmediaplayer_... > File media/blink/webmediaplayer_impl.cc (right): > > https://codereview.chromium.org/2115143002/diff/1/media/blink/webmediaplayer_... > media/blink/webmediaplayer_impl.cc:1058: client_->timeChanged(); > Is timeChanged() notification also always required on underflow? > > https://codereview.chromium.org/2115143002/diff/1/media/blink/webmediaplayer_... > media/blink/webmediaplayer_impl.cc:1064: UpdatePlayState(); > Does underflow allow us to suspend if idle (stalled) long enough? From chat with > sandersd@, the behavior expectation is that (barring being in the middle of a > seek), an underflow should enable suspend to occur if underflowed long enough. > Please add appropriate wmpi_unittest(s) to verify underflow during, and not > during, seek, results on suspend behavior. +sandersd@ to CR that portion of this > CL, too. > > https://codereview.chromium.org/2115143002/diff/1/third_party/WebKit/LayoutTe... > File third_party/WebKit/LayoutTests/http/tests/media/video-play-stall.html > (right): > > https://codereview.chromium.org/2115143002/diff/1/third_party/WebKit/LayoutTe... > third_party/WebKit/LayoutTests/http/tests/media/video-play-stall.html:1: <video > id="test_video"></video> > {linux,win}_chromium_rel_ng bots didn't pass this test > > https://codereview.chromium.org/2115143002/diff/1/third_party/WebKit/LayoutTe... > third_party/WebKit/LayoutTests/http/tests/media/video-play-stall.html:7: > async_test(t => { > nit: is the => {... consistent with other tests? Why not async_test(t) {...} ? > > https://codereview.chromium.org/2115143002/diff/1/third_party/WebKit/LayoutTe... > third_party/WebKit/LayoutTests/http/tests/media/video-play-stall.html:7: > async_test(t => { > nit: left justify 0-margin the outermost portion of this <script> body (w3c > wpt-style, or so I'm told). > > https://codereview.chromium.org/2115143002/diff/1/third_party/WebKit/LayoutTe... > third_party/WebKit/LayoutTests/http/tests/media/video-play-stall.html:13: let > ReadyState = { > These are already exposed on the IDL for HTMLMediaElement. Now might also be a good time to fix layout tests involving https://bugs.chromium.org/p/chromium/issues/detail?id=266592 (mediasource-duration.html), now that I've updated the duration change behavior to be compliant with current MSE spec, and since you're actually allowing 'waiting' to occur :)
https://codereview.chromium.org/2115143002/diff/1/media/blink/webmediaplayer_... File media/blink/webmediaplayer_impl.cc (right): https://codereview.chromium.org/2115143002/diff/1/media/blink/webmediaplayer_... media/blink/webmediaplayer_impl.cc:1058: client_->timeChanged(); On 2016/07/07 21:32:03, wolenetz wrote: > Is timeChanged() notification also always required on underflow? Changing the ready state does not directly change anything that HTMLMediaElement cares about, so I don't think so. (There is also a separate readyStateChanged() call that happens inside SetReadyState().) https://codereview.chromium.org/2115143002/diff/1/media/blink/webmediaplayer_... media/blink/webmediaplayer_impl.cc:1064: UpdatePlayState(); On 2016/07/07 21:32:03, wolenetz wrote: > Does underflow allow us to suspend if idle (stalled) long enough? From chat with > sandersd@, the behavior expectation is that (barring being in the middle of a > seek), an underflow should enable suspend to occur if underflowed long enough. > Please add appropriate wmpi_unittest(s) to verify underflow during, and not > during, seek, results on suspend behavior. +sandersd@ to CR that portion of this > CL, too. In general these conditions should be unchanged; the state that matters is whether the current state is >= HAVE_METADATA, whether we have ever been at HAVE_FUTURE_DATA. Se we expect the usual behavior: suspend while paused or when backgrounded, don't suspend during seek (except for MSE), etc.
On 2016/07/07 21:35:25, wolenetz wrote: > Now might also be a good time to fix layout tests involving > https://bugs.chromium.org/p/chromium/issues/detail?id=266592 > (mediasource-duration.html), now that I've updated the duration change behavior > to be compliant with current MSE spec, and since you're actually allowing > 'waiting' to occur :) I don't think this CL fixes Issue 266592. I'll fix an update the layout tests for that separately.
https://codereview.chromium.org/2115143002/diff/1/media/blink/webmediaplayer_... File media/blink/webmediaplayer_impl.cc (right): https://codereview.chromium.org/2115143002/diff/1/media/blink/webmediaplayer_... media/blink/webmediaplayer_impl.cc:1058: client_->timeChanged(); On 2016/07/07 21:49:19, sandersd wrote: > On 2016/07/07 21:32:03, wolenetz wrote: > > Is timeChanged() notification also always required on underflow? > > Changing the ready state does not directly change anything that HTMLMediaElement > cares about, so I don't think so. sandersd@, do we need to bother calling timeChanged in the HAVE_ENOUGH case then? Can we just remove the call altogether? I initially thought this was needed to fire the timeupdate event which the spec asks for at the transition from HAVE_CURRENT to less than HAVE_CURRENT. But I see that this is already taken care of in HTMLMediaElement's SetReadyState https://codereview.chromium.org/2115143002/diff/1/media/blink/webmediaplayer_... media/blink/webmediaplayer_impl.cc:1064: UpdatePlayState(); On 2016/07/07 21:49:19, sandersd wrote: > On 2016/07/07 21:32:03, wolenetz wrote: > > Does underflow allow us to suspend if idle (stalled) long enough? From chat > with > > sandersd@, the behavior expectation is that (barring being in the middle of a > > seek), an underflow should enable suspend to occur if underflowed long enough. > > Please add appropriate wmpi_unittest(s) to verify underflow during, and not > > during, seek, results on suspend behavior. +sandersd@ to CR that portion of > this > > CL, too. > > In general these conditions should be unchanged; the state that matters is > whether the current state is >= HAVE_METADATA, whether we have ever been at > HAVE_FUTURE_DATA. > > Se we expect the usual behavior: suspend while paused or when backgrounded, > don't suspend during seek (except for MSE), etc. sandersd@ do we already have a test for this usual behavior? Is a new test needed? https://codereview.chromium.org/2115143002/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/http/tests/media/video-play-stall.html (right): https://codereview.chromium.org/2115143002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/http/tests/media/video-play-stall.html:1: <video id="test_video"></video> On 2016/07/07 21:32:03, wolenetz wrote: > {linux,win}_chromium_rel_ng bots didn't pass this test The failure is: 17:12:24.365 252 worker/0 http/tests/media/video-play-stall.html failed: 17:12:24.365 252 worker/0 -expected.txt was missing What do I need to change so that it will stop looking for the expected test (instead: just expect that the test will pass)?
On 2016/07/07 22:29:41, chcunningham wrote: > https://codereview.chromium.org/2115143002/diff/1/media/blink/webmediaplayer_... > File media/blink/webmediaplayer_impl.cc (right): > > https://codereview.chromium.org/2115143002/diff/1/media/blink/webmediaplayer_... > media/blink/webmediaplayer_impl.cc:1058: client_->timeChanged(); > On 2016/07/07 21:49:19, sandersd wrote: > > On 2016/07/07 21:32:03, wolenetz wrote: > > > Is timeChanged() notification also always required on underflow? > > > > Changing the ready state does not directly change anything that > HTMLMediaElement > > cares about, so I don't think so. > > sandersd@, do we need to bother calling timeChanged in the HAVE_ENOUGH case > then? Can we just remove the call altogether? > > I initially thought this was needed to fire the timeupdate event which the spec > asks for at the transition from HAVE_CURRENT to less than HAVE_CURRENT. But I > see that this is already taken care of in HTMLMediaElement's SetReadyState > > https://codereview.chromium.org/2115143002/diff/1/media/blink/webmediaplayer_... > media/blink/webmediaplayer_impl.cc:1064: UpdatePlayState(); > On 2016/07/07 21:49:19, sandersd wrote: > > On 2016/07/07 21:32:03, wolenetz wrote: > > > Does underflow allow us to suspend if idle (stalled) long enough? From chat > > with > > > sandersd@, the behavior expectation is that (barring being in the middle of > a > > > seek), an underflow should enable suspend to occur if underflowed long > enough. > > > Please add appropriate wmpi_unittest(s) to verify underflow during, and not > > > during, seek, results on suspend behavior. +sandersd@ to CR that portion of > > this > > > CL, too. > > > > In general these conditions should be unchanged; the state that matters is > > whether the current state is >= HAVE_METADATA, whether we have ever been at > > HAVE_FUTURE_DATA. > > > > Se we expect the usual behavior: suspend while paused or when backgrounded, > > don't suspend during seek (except for MSE), etc. > > sandersd@ do we already have a test for this usual behavior? Is a new test > needed? > > https://codereview.chromium.org/2115143002/diff/1/third_party/WebKit/LayoutTe... > File third_party/WebKit/LayoutTests/http/tests/media/video-play-stall.html > (right): > > https://codereview.chromium.org/2115143002/diff/1/third_party/WebKit/LayoutTe... > third_party/WebKit/LayoutTests/http/tests/media/video-play-stall.html:1: <video > id="test_video"></video> > On 2016/07/07 21:32:03, wolenetz wrote: > > {linux,win}_chromium_rel_ng bots didn't pass this test > > The failure is: > > 17:12:24.365 252 worker/0 http/tests/media/video-play-stall.html failed: > 17:12:24.365 252 worker/0 -expected.txt was missing > > What do I need to change so that it will stop looking for the expected test > (instead: just expect that the test will pass)? If there is any stdout (e.g. console logging) done within the test that doesn't match precisely what a pass report from the testharness would look like, then there must be an -expected.txt with the precise output that's expected. Since your test is doing debug logging, that's probably producing output that needs to be either put into an -expected.txt, or you'll need to disable the logging.
https://codereview.chromium.org/2115143002/diff/1/media/blink/webmediaplayer_... File media/blink/webmediaplayer_impl.cc (right): https://codereview.chromium.org/2115143002/diff/1/media/blink/webmediaplayer_... media/blink/webmediaplayer_impl.cc:1058: client_->timeChanged(); On 2016/07/07 22:29:41, chcunningham wrote: > On 2016/07/07 21:49:19, sandersd wrote: > > On 2016/07/07 21:32:03, wolenetz wrote: > > > Is timeChanged() notification also always required on underflow? > > > > Changing the ready state does not directly change anything that > HTMLMediaElement > > cares about, so I don't think so. > > sandersd@, do we need to bother calling timeChanged in the HAVE_ENOUGH case > then? Can we just remove the call altogether? > > I initially thought this was needed to fire the timeupdate event which the spec > asks for at the transition from HAVE_CURRENT to less than HAVE_CURRENT. But I > see that this is already taken care of in HTMLMediaElement's SetReadyState HTMLMediaElement actually hooks this to fire the 'seeked' event, we do need it. https://codereview.chromium.org/2115143002/diff/1/media/blink/webmediaplayer_... media/blink/webmediaplayer_impl.cc:1064: UpdatePlayState(); On 2016/07/07 22:29:41, chcunningham wrote: > On 2016/07/07 21:49:19, sandersd wrote: > > On 2016/07/07 21:32:03, wolenetz wrote: > > > Does underflow allow us to suspend if idle (stalled) long enough? From chat > > with > > > sandersd@, the behavior expectation is that (barring being in the middle of > a > > > seek), an underflow should enable suspend to occur if underflowed long > enough. > > > Please add appropriate wmpi_unittest(s) to verify underflow during, and not > > > during, seek, results on suspend behavior. +sandersd@ to CR that portion of > > this > > > CL, too. > > > > In general these conditions should be unchanged; the state that matters is > > whether the current state is >= HAVE_METADATA, whether we have ever been at > > HAVE_FUTURE_DATA. > > > > Se we expect the usual behavior: suspend while paused or when backgrounded, > > don't suspend during seek (except for MSE), etc. > > sandersd@ do we already have a test for this usual behavior? Is a new test > needed? There are already tests for the main cases of note, you'll just want to add some to make sure we don't regress handling of this new ready state.
On 2016/07/07 22:29:14, chcunningham wrote: > On 2016/07/07 21:35:25, wolenetz wrote: > > Now might also be a good time to fix layout tests involving > > https://bugs.chromium.org/p/chromium/issues/detail?id=266592 > > (mediasource-duration.html), now that I've updated the duration change > behavior > > to be compliant with current MSE spec, and since you're actually allowing > > 'waiting' to occur :) > > I don't think this CL fixes Issue 266592. I'll fix an update the layout tests > for that separately. Acknowledged.
https://codereview.chromium.org/2115143002/diff/1/media/blink/webmediaplayer_... File media/blink/webmediaplayer_impl.cc (right): https://codereview.chromium.org/2115143002/diff/1/media/blink/webmediaplayer_... media/blink/webmediaplayer_impl.cc:1058: client_->timeChanged(); On 2016/07/07 22:33:12, sandersd wrote: > On 2016/07/07 22:29:41, chcunningham wrote: > > On 2016/07/07 21:49:19, sandersd wrote: > > > On 2016/07/07 21:32:03, wolenetz wrote: > > > > Is timeChanged() notification also always required on underflow? > > > > > > Changing the ready state does not directly change anything that > > HTMLMediaElement > > > cares about, so I don't think so. > > > > sandersd@, do we need to bother calling timeChanged in the HAVE_ENOUGH case > > then? Can we just remove the call altogether? > > > > I initially thought this was needed to fire the timeupdate event which the > spec > > asks for at the transition from HAVE_CURRENT to less than HAVE_CURRENT. But I > > see that this is already taken care of in HTMLMediaElement's SetReadyState > > HTMLMediaElement actually hooks this to fire the 'seeked' event, we do need it. Ack. Moved this up. FYI I also moved ReportMemoryUsage into the HAVE_ENOUGH case because its comment mentions "have enough" ... double check me here. https://codereview.chromium.org/2115143002/diff/1/media/blink/webmediaplayer_... media/blink/webmediaplayer_impl.cc:1064: UpdatePlayState(); Underflow is a very different user experience then intentional pause. After chatting with sanders, we agree that underflow should never be capable of triggering idle suspend. We agreed on 2 main scenarios: 1. simulating underflow does not causes idle suspend 2. simulating underflow does not interfere with legit background susp I've added a test to cover both these cases. https://codereview.chromium.org/2115143002/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/http/tests/media/video-play-stall.html (right): https://codereview.chromium.org/2115143002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/http/tests/media/video-play-stall.html:1: <video id="test_video"></video> I've changed the console.log calls to instead log messages to the document. Bots should be happy now. https://codereview.chromium.org/2115143002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/http/tests/media/video-play-stall.html:7: async_test(t => { On 2016/07/07 21:32:03, wolenetz wrote: > nit: is the => {... consistent with other tests? Why not async_test(t) {...} ? I think you mean async_test(function(t) { ... }). I find cases of both used. https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/http/test... https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/broadcast... I prefer the => syntax. https://codereview.chromium.org/2115143002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/http/tests/media/video-play-stall.html:7: async_test(t => { On 2016/07/07 21:32:03, wolenetz wrote: > nit: is the => {... consistent with other tests? Why not async_test(t) {...} ? Done. https://codereview.chromium.org/2115143002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/http/tests/media/video-play-stall.html:13: let ReadyState = { On 2016/07/07 21:32:03, wolenetz wrote: > These are already exposed on the IDL for HTMLMediaElement. Duh! Done.
Tests lgtm.
The CQ bit was checked by chcunningham@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM % nits: https://codereview.chromium.org/2115143002/diff/1/media/blink/webmediaplayer_... File media/blink/webmediaplayer_impl.cc (right): https://codereview.chromium.org/2115143002/diff/1/media/blink/webmediaplayer_... media/blink/webmediaplayer_impl.cc:1058: client_->timeChanged(); On 2016/07/12 00:47:17, chcunningham wrote: > On 2016/07/07 22:33:12, sandersd wrote: > > On 2016/07/07 22:29:41, chcunningham wrote: > > > On 2016/07/07 21:49:19, sandersd wrote: > > > > On 2016/07/07 21:32:03, wolenetz wrote: > > > > > Is timeChanged() notification also always required on underflow? > > > > > > > > Changing the ready state does not directly change anything that > > > HTMLMediaElement > > > > cares about, so I don't think so. > > > > > > sandersd@, do we need to bother calling timeChanged in the HAVE_ENOUGH case > > > then? Can we just remove the call altogether? > > > > > > I initially thought this was needed to fire the timeupdate event which the > > spec > > > asks for at the transition from HAVE_CURRENT to less than HAVE_CURRENT. But > I > > > see that this is already taken care of in HTMLMediaElement's SetReadyState > > > > HTMLMediaElement actually hooks this to fire the 'seeked' event, we do need > it. > > Ack. Moved this up. FYI I also moved ReportMemoryUsage into the HAVE_ENOUGH case > because its comment mentions "have enough" ... double check me here. I think this is correct. https://codereview.chromium.org/2115143002/diff/1/media/blink/webmediaplayer_... media/blink/webmediaplayer_impl.cc:1064: UpdatePlayState(); On 2016/07/12 00:47:17, chcunningham wrote: > Underflow is a very different user experience then intentional pause. After > chatting with sanders, we agree that underflow should never be capable of > triggering idle suspend. We agreed on 2 main scenarios: > > 1. simulating underflow does not causes idle suspend > 2. simulating underflow does not interfere with legit background susp > > I've added a test to cover both these cases. Acknowledged. https://codereview.chromium.org/2115143002/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/http/tests/media/video-play-stall.html (right): https://codereview.chromium.org/2115143002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/http/tests/media/video-play-stall.html:1: <video id="test_video"></video> On 2016/07/12 00:47:17, chcunningham wrote: > I've changed the console.log calls to instead log messages to the document. Bots > should be happy now. Acknowledged. https://codereview.chromium.org/2115143002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/http/tests/media/video-play-stall.html:7: async_test(t => { On 2016/07/12 00:47:18, chcunningham wrote: > On 2016/07/07 21:32:03, wolenetz wrote: > > nit: is the => {... consistent with other tests? Why not async_test(t) {...} ? > > I think you mean async_test(function(t) { ... }). I find cases of both used. > > https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/http/test... > https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/broadcast... > > I prefer the => syntax. Acknowledged. https://codereview.chromium.org/2115143002/diff/60001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/http/tests/media/video-play-stall.html (right): https://codereview.chromium.org/2115143002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/media/video-play-stall.html:58: .then(() => playback_ew.wait_for('loadedmetadata')).then(logEvent) nit: hard to read. please indent. https://codereview.chromium.org/2115143002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/media/video-play-stall.html:109: // The delay of 6 seconds is chosen to reduce flakiness in waiting for the This lengthy test therefore will need to be added to https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/SlowTests... Please do that in this CL to further ensure no flakiness. https://codereview.chromium.org/2115143002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/media/video-play-stall.html:111: video.src = "http://127.0.0.1:8000/resources/load-and-stall.cgi?name=../../../media/" + mediaFile + "&mimeType=" + mimeType + "&stallAt=100000&stallFor=8"; Which is desired? 6 or 8 (fix the comments, above, or this line...)
https://codereview.chromium.org/2115143002/diff/60001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/http/tests/media/video-play-stall.html (right): https://codereview.chromium.org/2115143002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/media/video-play-stall.html:109: // The delay of 6 seconds is chosen to reduce flakiness in waiting for the On 2016/07/13 22:27:54, wolenetz wrote: > This lengthy test therefore will need to be added to > https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/SlowTests... > > Please do that in this CL to further ensure no flakiness. Oops. This test is already listed in SlowTests (https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/SlowTests...). Ignore my original comment here.
Thanks for review! https://codereview.chromium.org/2115143002/diff/60001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/http/tests/media/video-play-stall.html (right): https://codereview.chromium.org/2115143002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/media/video-play-stall.html:58: .then(() => playback_ew.wait_for('loadedmetadata')).then(logEvent) On 2016/07/13 22:27:54, wolenetz wrote: > nit: hard to read. please indent. Done. https://codereview.chromium.org/2115143002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/media/video-play-stall.html:111: video.src = "http://127.0.0.1:8000/resources/load-and-stall.cgi?name=../../../media/" + mediaFile + "&mimeType=" + mimeType + "&stallAt=100000&stallFor=8"; On 2016/07/13 22:27:54, wolenetz wrote: > Which is desired? 6 or 8 (fix the comments, above, or this line...) Fixed the comment. I cranked it to 8 because the stalled event was coming a little too close to the download's resuming for my comfort.
The CQ bit was checked by chcunningham@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sandersd@chromium.org, wolenetz@chromium.org Link to the patchset: https://codereview.chromium.org/2115143002/#ps100001 (title: "Feedback")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Transition media element to HAVE_CURRENT_DATA upon underflow. Previously underflow would cause a pause but did not change the readyState to HAVE_CURRENT_DATA. This also meant that we did not emit the "waiting" event upon underflow, meaning apps were not properly informed when playback stalls. This CL fixes all of that. This CL fixes a layout test that was meant to cover this scenario but did not properly check the waiting event. The test has been refactored to use the new testharness.js. Still missing is support for transitioning to HAVE_FUTURE_DATA. Currently we will transition immediately from HAVE_CURRENT to HAVE_ENOUGH, skipping HAVE_FUTURE. This is less critical, but still nice to have, especially for MediaSource. BUG=144683,279213 TEST=fixed layout test ========== to ========== Transition media element to HAVE_CURRENT_DATA upon underflow. Previously underflow would cause a pause but did not change the readyState to HAVE_CURRENT_DATA. This also meant that we did not emit the "waiting" event upon underflow, meaning apps were not properly informed when playback stalls. This CL fixes all of that. This CL fixes a layout test that was meant to cover this scenario but did not properly check the waiting event. The test has been refactored to use the new testharness.js. Still missing is support for transitioning to HAVE_FUTURE_DATA. Currently we will transition immediately from HAVE_CURRENT to HAVE_ENOUGH, skipping HAVE_FUTURE. This is less critical, but still nice to have, especially for MediaSource. BUG=144683,279213 TEST=fixed layout test ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
CQ bit was unchecked.
Message was sent while issue was closed.
Description was changed from ========== Transition media element to HAVE_CURRENT_DATA upon underflow. Previously underflow would cause a pause but did not change the readyState to HAVE_CURRENT_DATA. This also meant that we did not emit the "waiting" event upon underflow, meaning apps were not properly informed when playback stalls. This CL fixes all of that. This CL fixes a layout test that was meant to cover this scenario but did not properly check the waiting event. The test has been refactored to use the new testharness.js. Still missing is support for transitioning to HAVE_FUTURE_DATA. Currently we will transition immediately from HAVE_CURRENT to HAVE_ENOUGH, skipping HAVE_FUTURE. This is less critical, but still nice to have, especially for MediaSource. BUG=144683,279213 TEST=fixed layout test ========== to ========== Transition media element to HAVE_CURRENT_DATA upon underflow. Previously underflow would cause a pause but did not change the readyState to HAVE_CURRENT_DATA. This also meant that we did not emit the "waiting" event upon underflow, meaning apps were not properly informed when playback stalls. This CL fixes all of that. This CL fixes a layout test that was meant to cover this scenario but did not properly check the waiting event. The test has been refactored to use the new testharness.js. Still missing is support for transitioning to HAVE_FUTURE_DATA. Currently we will transition immediately from HAVE_CURRENT to HAVE_ENOUGH, skipping HAVE_FUTURE. This is less critical, but still nice to have, especially for MediaSource. BUG=144683,279213 TEST=fixed layout test Committed: https://crrev.com/eb270c9af201e549f3e2e2b1016b6b99565f1706 Cr-Commit-Position: refs/heads/master@{#405656} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/eb270c9af201e549f3e2e2b1016b6b99565f1706 Cr-Commit-Position: refs/heads/master@{#405656} |
