|
|
Created:
4 years, 3 months ago by DaleCurtis Modified:
4 years, 3 months ago CC:
blink-reviews, blink-reviews-api_chromium.org, blink-reviews-html_chromium.org, chromium-reviews, dglazkov+blink, eric.carlson_apple.com, feature-media-reviews_chromium.org, fs, gasubic, mlamouri+watch-blink_chromium.org, nessy, vcarbune.chromium Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAllow suspension prior to reaching kHaveFutureData.
This change rolls in a few changes:
- Removes DelegateState::PAUSED_BUT_NOT_IDLE state.
- Removes all blocks against suspending prior to kHaveFutureData.
- If suspended, triggers resume when prior to have kHaveFutureData
if the didLoadingProgress() timer reports true.
- Always notifies the delegate of DelegateState::GONE; this ensures
the idle timer is always restarted if we didn't quite get enough
data to reach kHaveFutureData.
- Fixes pause timer to handle newly supported background video;
which doesn't set a state of DelegateState::GONE
BUG=612909, 643383
TEST=new and updated unittests.
Committed: https://crrev.com/e7120dcf27938c9604ec03b3b574e1b4b3bf6c28
Cr-Commit-Position: refs/heads/master@{#416431}
Patch Set 1 #
Total comments: 6
Patch Set 2 : Comments. #Patch Set 3 : Rename to setPaused. #
Total comments: 4
Patch Set 4 : Fix comments. #Patch Set 5 : Change approaches. #Patch Set 6 : Rollup. #Patch Set 7 : Fix pause timer. #
Messages
Total messages: 36 (9 generated)
Description was changed from ========== Add WebMediaPlayer::setPlaybackState() to improve suspend cases. Allows us to suspend in cases prior to HaveFutureData since we can be sure the resume signal is properly passed through upon user actions. BUG=none TEST=updated unittests. ========== to ========== Add WebMediaPlayer::setPlaybackState() to improve suspend cases. Allows us to suspend in cases prior to HaveFutureData since we can be sure the resume signal is properly passed through upon user actions. This is part one of letting us enable Spitzer on JellyBean; it ensures we can suspend MSE based players that are in a stalled but paused state. BUG=612909 TEST=updated unittests. ==========
dalecurtis@chromium.org changed reviewers: + foolip@chromium.org, sandersd@chromium.org
I think I need some more context on this, does this by itself fix the vimeo.com+scrolling issue described in issue 612909? How do you get into a "stalled but paused state"? Is that stalled as in the pipeline thinks it's waiting for data to feed the demuxer? If so, is it just wrong about that, or does it actually need that data to decode the current frame? HTMLMediaElement::updatePlayState() looks like it's already syncing the WebMediaPlayer instance with potentiallyPlaying(), why isn't that the right state for "trying to play"?
Prior to have future data, html media element does not forward play calls to wmpi. Note the use of playing and not paused to set the state in my change.
Vimeo gets in this state by appending an init segment but never any encoded data. So we never reach beyond have metadata. This appears to be a valid mse state that prevents us from suspending since we never know when the user presses play.
https://codereview.chromium.org/2289543005/diff/1/media/blink/webmediaplayer_... File media/blink/webmediaplayer_impl.cc (right): https://codereview.chromium.org/2289543005/diff/1/media/blink/webmediaplayer_... media/blink/webmediaplayer_impl.cc:1655: // due to ordering of calls we may temporarily be out of sync though. It's worth explicitly describing this as retaining the paused state in that interim time. I believe though that readyStateChanged() is re-entrant, so this won't happen in practice. https://codereview.chromium.org/2289543005/diff/1/media/blink/webmediaplayer_... media/blink/webmediaplayer_impl.cc:1688: (is_suspended && is_paused && !seeking_); While you're in here, can you pull this condition out into its own variable? https://codereview.chromium.org/2289543005/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/2289543005/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:3181: webMediaPlayer()->setPlaybackState(m_playing || !m_paused ? WebMediaPlayer::PlaybackState::Playing : WebMediaPlayer::PlaybackState::Paused); This way of sending the information somewhat implies that WMPI should also be reading the state at construction time, instead of assuming paused. Do we get a strong guarantee that WMPI finds out the state shortly after construction from this?
Addressed comments. I'm not really happy with setPlaybackState() as the name, but am not sure what else to call it. I think most accurately it is setPaused() with the implication that prior to have_future_data paused-state may not reflect that of the actual pause()/play() calls. @foolip: In the interested of time to be sure we're all on the same page, here's the code that prevents WMPI from being notified of play/pause changes prior to have_future_data: https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/html/HTML... https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/html/HTML... Essentially shouldBePlaying() returns false, after HTMLMediaElement::play() or HTMLMediaElement::pause() are called. Leaving m_paused reflecting the actual pause state, but without WMPI having been told the state has changed. https://codereview.chromium.org/2289543005/diff/1/media/blink/webmediaplayer_... File media/blink/webmediaplayer_impl.cc (right): https://codereview.chromium.org/2289543005/diff/1/media/blink/webmediaplayer_... media/blink/webmediaplayer_impl.cc:1655: // due to ordering of calls we may temporarily be out of sync though. On 2016/08/30 at 18:40:17, sandersd wrote: > It's worth explicitly describing this as retaining the paused state in that interim time. > > I believe though that readyStateChanged() is re-entrant, so this won't happen in practice. This does happen, HTMLMediaElement::UpdatePlayState() first calls pause() then calls WMP::UpdatePlaybackState(). https://codereview.chromium.org/2289543005/diff/1/media/blink/webmediaplayer_... media/blink/webmediaplayer_impl.cc:1688: (is_suspended && is_paused && !seeking_); On 2016/08/30 at 18:40:18, sandersd wrote: > While you're in here, can you pull this condition out into its own variable? Done. https://codereview.chromium.org/2289543005/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/2289543005/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:3181: webMediaPlayer()->setPlaybackState(m_playing || !m_paused ? WebMediaPlayer::PlaybackState::Playing : WebMediaPlayer::PlaybackState::Paused); On 2016/08/30 at 18:40:18, sandersd wrote: > This way of sending the information somewhat implies that WMPI should also be reading the state at construction time, instead of assuming paused. > > Do we get a strong guarantee that WMPI finds out the state shortly after construction from this? Added set during startPlayerLoad() -- otherwise I think you're right this could get out of sync.
Description was changed from ========== Add WebMediaPlayer::setPlaybackState() to improve suspend cases. Allows us to suspend in cases prior to HaveFutureData since we can be sure the resume signal is properly passed through upon user actions. This is part one of letting us enable Spitzer on JellyBean; it ensures we can suspend MSE based players that are in a stalled but paused state. BUG=612909 TEST=updated unittests. ========== to ========== Add WebMediaPlayer::setPaused() to improve suspend cases. Allows us to suspend in cases prior to HaveFutureData since we can be sure the resume signal is properly passed through upon user actions. This is part one of letting us enable Spitzer on JellyBean; it ensures we can suspend MSE based players that are in a stalled but paused state. BUG=612909 TEST=updated unittests. ==========
Latest patch set renames to setPaused() since it's a simpler extension of what's going on in the HTMLME. Still not happy because we have both pause() and setPaused(), but short of teaching HTMLME to send play() pause() to WMP always, I'm not sure what else do. Suggestions welcome :)
lgtm https://codereview.chromium.org/2289543005/diff/40001/media/blink/webmediapla... File media/blink/webmediaplayer_impl.cc (right): https://codereview.chromium.org/2289543005/diff/40001/media/blink/webmediapla... media/blink/webmediaplayer_impl.cc:1668: // that period, we'll retain the most accurate |paused_| state. Is it actually 'most accurate'? It happens to be correct in the path you added, but it's not necessarily so by contract. (There is a similar situation for 'ended', I opted to actually describe HTMLMediaElement's behavior inside of WMPI.) https://codereview.chromium.org/2289543005/diff/40001/media/blink/webmediapla... File media/blink/webmediaplayer_impl.h (right): https://codereview.chromium.org/2289543005/diff/40001/media/blink/webmediapla... media/blink/webmediaplayer_impl.h:550: // Reflects the playback state of the WebMediaPlayerClient. This will not Nit: will not -> may not
https://codereview.chromium.org/2289543005/diff/40001/media/blink/webmediapla... File media/blink/webmediaplayer_impl.cc (right): https://codereview.chromium.org/2289543005/diff/40001/media/blink/webmediapla... media/blink/webmediaplayer_impl.cc:1668: // that period, we'll retain the most accurate |paused_| state. On 2016/08/30 at 22:41:41, sandersd wrote: > Is it actually 'most accurate'? It happens to be correct in the path you added, but it's not necessarily so by contract. (There is a similar situation for 'ended', I opted to actually describe HTMLMediaElement's behavior inside of WMPI.) Reworded. https://codereview.chromium.org/2289543005/diff/40001/media/blink/webmediapla... File media/blink/webmediaplayer_impl.h (right): https://codereview.chromium.org/2289543005/diff/40001/media/blink/webmediapla... media/blink/webmediaplayer_impl.h:550: // Reflects the playback state of the WebMediaPlayerClient. This will not On 2016/08/30 at 22:41:41, sandersd wrote: > Nit: will not -> may not Done.
On 2016/08/30 21:56:04, DaleCurtis wrote: > Addressed comments. I'm not really happy with setPlaybackState() as the name, > but am not sure what else to call it. I think most accurately it is setPaused() > with the implication that prior to have_future_data paused-state may not reflect > that of the actual pause()/play() calls. > > @foolip: In the interested of time to be sure we're all on the same page, here's > the code that prevents WMPI from being notified of play/pause changes prior to > have_future_data: > > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/html/HTML... > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/html/HTML... > > Essentially shouldBePlaying() returns false, after HTMLMediaElement::play() or > HTMLMediaElement::pause() are called. Leaving m_paused reflecting the actual > pause state, but without WMPI having been told the state has changed. Thanks, I couldn't find it before you pointed it out :) I did notice the use of m_playing and that makes me nervous. m_playing is a weird bit of extra state that's only used to maintain the played ranges
On 2016/08/31 20:45:07, foolip wrote: > On 2016/08/30 21:56:04, DaleCurtis wrote: > > Addressed comments. I'm not really happy with setPlaybackState() as the name, > > but am not sure what else to call it. I think most accurately it is > setPaused() > > with the implication that prior to have_future_data paused-state may not > reflect > > that of the actual pause()/play() calls. > > > > @foolip: In the interested of time to be sure we're all on the same page, > here's > > the code that prevents WMPI from being notified of play/pause changes prior to > > have_future_data: > > > > > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/html/HTML... > > > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/html/HTML... > > > > Essentially shouldBePlaying() returns false, after HTMLMediaElement::play() or > > HTMLMediaElement::pause() are called. Leaving m_paused reflecting the actual > > pause state, but without WMPI having been told the state has changed. > > Thanks, I couldn't find it before you pointed it out :) > > I did notice the use of m_playing and that makes me nervous. m_playing is a > weird bit of extra state that's only used to maintain the played ranges Posted too early. m_playing is only used to maintain the played ranges, and I'm hopeful that someone will come along and get rid of it. But that's just an itch, it's not very important. Looking at webmediaplayer_impl.cc, I'm still not sure I understand. Can't one sensibly treat the eventual play/pause state as unknown? It effectively is, because the script could decide to play in response to a readyState transition, which also happens when you're using the autoplay attribute. Will comment inline as well.
Prior to this change, WMPI has assumed it's unknown, but this prevents us from suspending the pipeline in cases where unknown == paused w/ no intent to play. This seems to only occur with MSE where the time between has_metadata and has_future is entirely dependent on what the JS does. In this case we want to suspend these players to avoid them holding hardware resources indefinitely. Note: I no longer use m_playing in the CL, it's unnecessary; as such I have also renamed the method to setPaused() per previous comments.
foolip@chromium.org changed reviewers: + wolenetz@chromium.org
Adding wolenetz@ as I'm about to say some things about MSE.
On 2016/08/31 20:55:59, DaleCurtis wrote: > Prior to this change, WMPI has assumed it's unknown, but this prevents us from > suspending the pipeline in cases where unknown == paused w/ no intent to play. > This seems to only occur with MSE where the time between has_metadata and > has_future is entirely dependent on what the JS does. In this case we want to > suspend these players to avoid them holding hardware resources indefinitely. > > Note: I no longer use m_playing in the CL, it's unnecessary; as such I have also > renamed the method to setPaused() per previous comments. Commenting here instead of inline :) Some basic questions: 1. Can all players be suspended, or just for MSE? I guess all, because I see nothing MSE-specific in the changes. 2. Is the suspend mechanism supposed to be transparent to web contents and robust against making the wrong guess? Since web contents is crazy, it's seems a given that you can make the wrong guess :) 3. Issue 612909 was about a crash, does this have anything to do with the crash or is it really just about tuning a heuristic? Maybe it was an OOO crash? If this is entirely about a heuristic to guess whether the player can be suspended without immediately having to be resumed, then using state from the media element makes sense. If it's supposed to work for all kinds of media resources, then I think including the autoplay also makes sense, since otherwise you might presumably get unlucky and suspend something that's just about to autoplay? For the MSE problem specifically, how will you know with some confidence that JS isn't just waiting for more data from the network, which it will append and then call play()? At least delaying the play() call until one knows one can play makes sense, because it makes it easier to keep your controls in sync with the "is it really playing" state, not the "trying to play" state. At the end of the day, something like WebMediaPlayer::setLikelyToPlaySoon(bool) seems OK if it fixes a known problem, but it's easy to imagine more problems of this sort :)
On 2016/08/31 at 21:16:52, foolip wrote: > On 2016/08/31 20:55:59, DaleCurtis wrote: > > Prior to this change, WMPI has assumed it's unknown, but this prevents us from > > suspending the pipeline in cases where unknown == paused w/ no intent to play. > > This seems to only occur with MSE where the time between has_metadata and > > has_future is entirely dependent on what the JS does. In this case we want to > > suspend these players to avoid them holding hardware resources indefinitely. > > > > Note: I no longer use m_playing in the CL, it's unnecessary; as such I have also > > renamed the method to setPaused() per previous comments. > > Commenting here instead of inline :) > > Some basic questions: > 1. Can all players be suspended, or just for MSE? I guess all, because I see nothing MSE-specific in the changes. All players. This is already working in the majority of cases today. It's just this one case that requires a little extra glue to get suspend right in. > 2. Is the suspend mechanism supposed to be transparent to web contents and robust against making the wrong guess? Since web contents is crazy, it's seems a given that you can make the wrong guess :) Yes it's supposed to be completely transparent; though I believe sandersd@ says the ordering of events may get tweaked in some cases. > 3. Issue 612909 was about a crash, does this have anything to do with the crash or is it really just about tuning a heuristic? Maybe it was an OOO crash? It's an OO-Resources crash which suspend is intended to help with by releasing things that not really being used. > > If this is entirely about a heuristic to guess whether the player can be suspended without immediately having to be resumed, then using state from the media element makes sense. If it's supposed to work for all kinds of media resources, then I think including the autoplay also makes sense, since otherwise you might presumably get unlucky and suspend something that's just about to autoplay? The suspend timer is 15 seconds, loading will continue despite the suspension. Is HTMLME waiting for have future data before setting the !m_paused bit in the autoplaying case? Don't have access to test today. > > For the MSE problem specifically, how will you know with some confidence that JS isn't just waiting for more data from the network, which it will append and then call play()? At least delaying the play() call until one knows one can play makes sense, because it makes it easier to keep your controls in sync with the "is it really playing" state, not the "trying to play" state. I think there's a confusion that suspend is stalling loading, it's not. If loading reaches the point that playback can start we'll properly transition into the playing state. > > At the end of the day, something like WebMediaPlayer::setLikelyToPlaySoon(bool) seems OK if it fixes a known problem, but it's easy to imagine more problems of this sort :)
On 2016/08/31 21:26:03, DaleCurtis wrote: > On 2016/08/31 at 21:16:52, foolip wrote: > > On 2016/08/31 20:55:59, DaleCurtis wrote: > > > Prior to this change, WMPI has assumed it's unknown, but this prevents us > from > > > suspending the pipeline in cases where unknown == paused w/ no intent to > play. > > > This seems to only occur with MSE where the time between has_metadata and > > > has_future is entirely dependent on what the JS does. In this case we want > to > > > suspend these players to avoid them holding hardware resources indefinitely. > > > > > > Note: I no longer use m_playing in the CL, it's unnecessary; as such I have > also > > > renamed the method to setPaused() per previous comments. > > > > Commenting here instead of inline :) > > > > Some basic questions: > > 1. Can all players be suspended, or just for MSE? I guess all, because I see > nothing MSE-specific in the changes. > > All players. This is already working in the majority of cases today. It's just > this one case that requires a little extra glue to get suspend right in. > > > 2. Is the suspend mechanism supposed to be transparent to web contents and > robust against making the wrong guess? Since web contents is crazy, it's seems a > given that you can make the wrong guess :) > > Yes it's supposed to be completely transparent; though I believe sandersd@ says > the ordering of events may get tweaked in some cases. sandersd@, any details on that? It's not super relevant to this CL, but I am curious. > > 3. Issue 612909 was about a crash, does this have anything to do with the > crash or is it really just about tuning a heuristic? Maybe it was an OOO crash? > > It's an OO-Resources crash which suspend is intended to help with by releasing > things that not really being used. > > > > > If this is entirely about a heuristic to guess whether the player can be > suspended without immediately having to be resumed, then using state from the > media element makes sense. If it's supposed to work for all kinds of media > resources, then I think including the autoplay also makes sense, since otherwise > you might presumably get unlucky and suspend something that's just about to > autoplay? > > The suspend timer is 15 seconds, loading will continue despite the suspension. > Is HTMLME waiting for have future data before setting the !m_paused bit in the > autoplaying case? Don't have access to test today. Yep, see https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/html/HTML... And importantly, scripts can do exactly the same thing, so that you have no guess about whether you'll be asked to play soon or not. But if there's a 15 s timeout and an unnecessary suspend just makes things a bit slower, that sounds OK. > > For the MSE problem specifically, how will you know with some confidence that > JS isn't just waiting for more data from the network, which it will append and > then call play()? At least delaying the play() call until one knows one can play > makes sense, because it makes it easier to keep your controls in sync with the > "is it really playing" state, not the "trying to play" state. > > I think there's a confusion that suspend is stalling loading, it's not. If > loading reaches the point that playback can start we'll properly transition into > the playing state. Indeed, I guessed that suspending would also try to stop using the network. So, with the 15 s timeout, assuming that timeout is reset by appending data to MSE, would it work to just assume that if we've been in readyState < HAVE_CURRENT_DATA for that long with no activity, that we can just suspend? It seems that even if m_paused were false in this situation, the site could actually be sitting around waiting for some user-trigger event to fetch and append the data.
On 2016/08/31 at 21:53:55, foolip wrote: > On 2016/08/31 21:26:03, DaleCurtis wrote: > > On 2016/08/31 at 21:16:52, foolip wrote: > > > On 2016/08/31 20:55:59, DaleCurtis wrote: > > > > Prior to this change, WMPI has assumed it's unknown, but this prevents us > > from > > > > suspending the pipeline in cases where unknown == paused w/ no intent to > > play. > > > > This seems to only occur with MSE where the time between has_metadata and > > > > has_future is entirely dependent on what the JS does. In this case we want > > to > > > > suspend these players to avoid them holding hardware resources indefinitely. > > > > > > > > Note: I no longer use m_playing in the CL, it's unnecessary; as such I have > > also > > > > renamed the method to setPaused() per previous comments. > > > > > > Commenting here instead of inline :) > > > > > > Some basic questions: > > > 1. Can all players be suspended, or just for MSE? I guess all, because I see > > nothing MSE-specific in the changes. > > > > All players. This is already working in the majority of cases today. It's just > > this one case that requires a little extra glue to get suspend right in. > > > > > 2. Is the suspend mechanism supposed to be transparent to web contents and > > robust against making the wrong guess? Since web contents is crazy, it's seems a > > given that you can make the wrong guess :) > > > > Yes it's supposed to be completely transparent; though I believe sandersd@ says > > the ordering of events may get tweaked in some cases. > > sandersd@, any details on that? It's not super relevant to this CL, but I am curious. > > > > 3. Issue 612909 was about a crash, does this have anything to do with the > > crash or is it really just about tuning a heuristic? Maybe it was an OOO crash? > > > > It's an OO-Resources crash which suspend is intended to help with by releasing > > things that not really being used. > > > > > > > > If this is entirely about a heuristic to guess whether the player can be > > suspended without immediately having to be resumed, then using state from the > > media element makes sense. If it's supposed to work for all kinds of media > > resources, then I think including the autoplay also makes sense, since otherwise > > you might presumably get unlucky and suspend something that's just about to > > autoplay? > > > > The suspend timer is 15 seconds, loading will continue despite the suspension. > > Is HTMLME waiting for have future data before setting the !m_paused bit in the > > autoplaying case? Don't have access to test today. > > Yep, see https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/html/HTML... > > And importantly, scripts can do exactly the same thing, so that you have no guess about whether you'll be asked to play soon or not. But if there's a 15 s timeout and an unnecessary suspend just makes things a bit slower, that sounds OK. Okay, so it seems like we might want m_paused && !m_autoplaying in the setPaused() state. If a script is waiting for have_enough before hitting play(), then yes it could be a problem but manually starting playback would work normally. I think the 15s is long enough that this isn't a practical problem and the workaround of manually playing seems sufficient. I also think it's more likely that a player will use buffered ranges to make such a playback determination instead of the buffering state. > > > > For the MSE problem specifically, how will you know with some confidence that > > JS isn't just waiting for more data from the network, which it will append and > > then call play()? At least delaying the play() call until one knows one can play > > makes sense, because it makes it easier to keep your controls in sync with the > > "is it really playing" state, not the "trying to play" state. > > > > I think there's a confusion that suspend is stalling loading, it's not. If > > loading reaches the point that playback can start we'll properly transition into > > the playing state. > > Indeed, I guessed that suspending would also try to stop using the network. > > So, with the 15 s timeout, assuming that timeout is reset by appending data to MSE, would it work to just assume that if we've been in readyState < HAVE_CURRENT_DATA for that long with no activity, that we can just suspend? It seems that even if m_paused were false in this situation, the site could actually be sitting around waiting for some user-trigger event to fetch and append the data. Unfortunately not, the ready state changes will not fire when suspended since today Chrome reports have_enough based on both decoding and network data. Suspend kills the renderer so it will not reach have_enough; though buffered() will grow correctly based on what's in the demuxer / data source. Hence the reason for this API, we want to know when playback is intended so we can resume and fire the appropriate ready state changes.
> > Yes it's supposed to be completely transparent; though I believe sandersd@ > says > > the ordering of events may get tweaked in some cases. > > sandersd@, any details on that? It's not super relevant to this CL, but I am > curious. The implementation itself: - Decoders (in fact whole renderers) are thrown away. - Demuxers are kept alive. - Audio sinks are stopped. - Time freezes. - Internally, it looks like a seek, except the renderer gets swapped before prerolling. We try not to emit JS events (such a ready state changes), although it's been a while since I last audited that. There may be places where some of the internal-seek related states are leaking though. The main dangers to JS events are around startup and explicit seeking, and I have implemented some mitigations: - We don't suspend before a "stable" state, so hopefully no startup events can be missed. This CL changes our definition of "stable" to include paused-before-have-future-data. - We try to avoid completing seeks that are interrupted by a suspend (until after the resume), but sometimes Blink detects the seeks completing anyway. I'll probably remove that delay logic rather than fix it, it's not buying us much. We distinguish two different kinds of suspend, and apply different rules: - Background suspend for hidden tabs. These are allowed to be more forceful, since we expect a resume to come before the users expects them to be interactive. For example, seeks will be delayed until resume. - Idle suspend for visible, but paused video. (This mode is exited immediately by a play or seek.) Finally, there is at least one outright violation: - For streaming media, we resume at time 0, since we can't reliably seek. (This is usually what the user wants, but could be incorrect with certain server configurations. There's not much we can do about it, since we must suspend on Android when backgrounded.) The interactions are getting more complicated over time, for example we now support resuming playback in a background tab via the MediaSession UI, and this affects the definition of background suspend above.
On 2016/08/31 22:32:01, sandersd wrote: > > > Yes it's supposed to be completely transparent; though I believe sandersd@ > > says > > > the ordering of events may get tweaked in some cases. > > > > sandersd@, any details on that? It's not super relevant to this CL, but I am > > curious. > > The implementation itself: > - Decoders (in fact whole renderers) are thrown away. > - Demuxers are kept alive. > - Audio sinks are stopped. > - Time freezes. > - Internally, it looks like a seek, except the renderer gets swapped before > prerolling. > > We try not to emit JS events (such a ready state changes), although it's been a > while since I last audited that. There may be places where some of the > internal-seek related states are leaking though. > > The main dangers to JS events are around startup and explicit seeking, and I > have implemented some mitigations: > - We don't suspend before a "stable" state, so hopefully no startup events can > be missed. This CL changes our definition of "stable" to include > paused-before-have-future-data. > - We try to avoid completing seeks that are interrupted by a suspend (until > after the resume), but sometimes Blink detects the seeks completing anyway. I'll > probably remove that delay logic rather than fix it, it's not buying us much. > > We distinguish two different kinds of suspend, and apply different rules: > - Background suspend for hidden tabs. These are allowed to be more forceful, > since we expect a resume to come before the users expects them to be > interactive. For example, seeks will be delayed until resume. > - Idle suspend for visible, but paused video. (This mode is exited immediately > by a play or seek.) > > Finally, there is at least one outright violation: > - For streaming media, we resume at time 0, since we can't reliably seek. > (This is usually what the user wants, but could be incorrect with certain server > configurations. There's not much we can do about it, since we must suspend on > Android when backgrounded.) > > The interactions are getting more complicated over time, for example we now > support resuming playback in a background tab via the MediaSession UI, and this > affects the definition of background suspend above. I should add that our layout tests all pass when suspend/resume is triggered randomly (mod needing to increase some timeouts).
Latest patch set completely changes the approach. Once foolip@ mentioned autoplay I went spelunking and realized the old approach will result in the resurgence of http://crbug.com/592706. Now I'm using the didLoadingProgress() timer to determine if we should respin the renderer to figure out if we've reached have_enough data yet. This recovers quickly and keeps our suspend logic simplified as well as avoids any new state in HTMLME. I haven't fixed tests yet, but PTAL and let me know what you think. If it lg I'll fixup the tests.
On 2016/09/01 01:29:18, DaleCurtis wrote: > Latest patch set completely changes the approach. Once foolip@ mentioned > autoplay I went spelunking and realized the old approach will result in the > resurgence of http://crbug.com/592706. > > Now I'm using the didLoadingProgress() timer to determine if we should respin > the renderer to figure out if we've reached have_enough data yet. This recovers > quickly and keeps our suspend logic simplified as well as avoids any new state > in HTMLME. > > I haven't fixed tests yet, but PTAL and let me know what you think. If it lg > I'll fixup the tests. Cool, asking lots of questions sometimes reveals interesting things :) Leaving this sandersd@ now.
This makes sense to me, only concern I have is fighting between many active players. Please update comments to accurately describe the new logic.
Description was changed from ========== Add WebMediaPlayer::setPaused() to improve suspend cases. Allows us to suspend in cases prior to HaveFutureData since we can be sure the resume signal is properly passed through upon user actions. This is part one of letting us enable Spitzer on JellyBean; it ensures we can suspend MSE based players that are in a stalled but paused state. BUG=612909 TEST=updated unittests. ========== to ========== Allow suspension prior to reaching kHaveFutureData. This change rolls in a few changes: - Removes DelegateState::PAUSED_BUT_NOT_IDLE state. - Removes all blocks against suspending prior to kHaveFutureData. - If suspended, triggers resume when prior to have kHaveFutureData if the didLoadingProgress() timer reports true. - Always notifies the delegate of DelegateState::PLAYER_GONE; this ensures the idle timer is always restarted if we didn't quite get enough data to reach kHaveFutureData. BUG=612909 TEST=new and updated unittests. ==========
PTAL. I needed to roll in a few of the other changes since they became dependent.
Description was changed from ========== Allow suspension prior to reaching kHaveFutureData. This change rolls in a few changes: - Removes DelegateState::PAUSED_BUT_NOT_IDLE state. - Removes all blocks against suspending prior to kHaveFutureData. - If suspended, triggers resume when prior to have kHaveFutureData if the didLoadingProgress() timer reports true. - Always notifies the delegate of DelegateState::PLAYER_GONE; this ensures the idle timer is always restarted if we didn't quite get enough data to reach kHaveFutureData. BUG=612909 TEST=new and updated unittests. ========== to ========== Allow suspension prior to reaching kHaveFutureData. This change rolls in a few changes: - Removes DelegateState::PAUSED_BUT_NOT_IDLE state. - Removes all blocks against suspending prior to kHaveFutureData. - If suspended, triggers resume when prior to have kHaveFutureData if the didLoadingProgress() timer reports true. - Always notifies the delegate of DelegateState::GONE; this ensures the idle timer is always restarted if we didn't quite get enough data to reach kHaveFutureData. - Fixes pause timer to handle newly supported background video; which doesn't set a state of DelegateState::GONE BUG=612909, 643383 TEST=new and updated unittests. ==========
Not sure I need to be on the R for this, though in general, ++documentation around the conditions and (cough) state might help reviews :) RSLGTM % sandersd@ l*tm.
The CQ bit was checked by sandersd@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sandersd@chromium.org Link to the patchset: https://codereview.chromium.org/2289543005/#ps120001 (title: "Fix pause timer.")
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.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== Allow suspension prior to reaching kHaveFutureData. This change rolls in a few changes: - Removes DelegateState::PAUSED_BUT_NOT_IDLE state. - Removes all blocks against suspending prior to kHaveFutureData. - If suspended, triggers resume when prior to have kHaveFutureData if the didLoadingProgress() timer reports true. - Always notifies the delegate of DelegateState::GONE; this ensures the idle timer is always restarted if we didn't quite get enough data to reach kHaveFutureData. - Fixes pause timer to handle newly supported background video; which doesn't set a state of DelegateState::GONE BUG=612909, 643383 TEST=new and updated unittests. ========== to ========== Allow suspension prior to reaching kHaveFutureData. This change rolls in a few changes: - Removes DelegateState::PAUSED_BUT_NOT_IDLE state. - Removes all blocks against suspending prior to kHaveFutureData. - If suspended, triggers resume when prior to have kHaveFutureData if the didLoadingProgress() timer reports true. - Always notifies the delegate of DelegateState::GONE; this ensures the idle timer is always restarted if we didn't quite get enough data to reach kHaveFutureData. - Fixes pause timer to handle newly supported background video; which doesn't set a state of DelegateState::GONE BUG=612909, 643383 TEST=new and updated unittests. Committed: https://crrev.com/e7120dcf27938c9604ec03b3b574e1b4b3bf6c28 Cr-Commit-Position: refs/heads/master@{#416431} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/e7120dcf27938c9604ec03b3b574e1b4b3bf6c28 Cr-Commit-Position: refs/heads/master@{#416431} |