Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1830913005:
Convert WMPI state management to level-triggered. (Closed)
|
Created:
4 years, 9 months ago by sandersd (OOO until July 31) Modified:
4 years, 8 months ago CC:
chromium-reviews, 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. |
DescriptionConvert WMPI state management to level-triggered.
Calling NotifyPlaybackStarted()/NotifyPlaybackPaused() at the correct
times has been a recurring source of bugs. This change moves all of the
logic to a single method, which will hopefully be easier to understand
and debug.
This new method handles delegate state, suspend/resume, and the memory
usage reporting timer together in one place. It's not simple, but it is
commented liberally.
The new suspend/resume logic does not rely on the suspend/resume
callbacks (instead it's only based on the goal state, never the current
state). As a result I was also able to remove the resume callback from
PipelineController in this CL.
BUG=595900, 597692
Committed: https://crrev.com/50a635ebbf250f8f35ea060d564b102b804dcea7
Cr-Commit-Position: refs/heads/master@{#385038}
Patch Set 1 #Patch Set 2 : Remove resumed callback. #Patch Set 3 : Fix grammar. #
Total comments: 2
Patch Set 4 : Do not throw away the media session immediately on ended. #Patch Set 5 : Further reduce reliance on |ended_|. #
Total comments: 18
Patch Set 6 : #
Total comments: 2
Patch Set 7 : Contain dependencies better for testing. #Patch Set 8 : Tests #
Total comments: 12
Patch Set 9 : #
Total comments: 2
Patch Set 10 : Rebase. #Patch Set 11 : Use EXPECT_TRUE/EXPECT_FALSE. #
Messages
Total messages: 42 (19 generated)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:40001) has been deleted
Description was changed from ========== Convert WMPI state management to level-triggered. Calling NotifyPlaybackStarted()/NotifyPlaybackPaused() at the correct times has been a recurring source of bugs. This change moves all of the logit to a single method, which will hopefully be easier to understand and debug. BUG=595900 ========== to ========== Convert WMPI state management to level-triggered. Calling NotifyPlaybackStarted()/NotifyPlaybackPaused() at the correct times has been a recurring source of bugs. This change moves all of the logig to a single method, which will hopefully be easier to understand and debug. BUG=595900 ==========
Description was changed from ========== Convert WMPI state management to level-triggered. Calling NotifyPlaybackStarted()/NotifyPlaybackPaused() at the correct times has been a recurring source of bugs. This change moves all of the logig to a single method, which will hopefully be easier to understand and debug. BUG=595900 ========== to ========== Convert WMPI state management to level-triggered. Calling NotifyPlaybackStarted()/NotifyPlaybackPaused() at the correct times has been a recurring source of bugs. This change moves all of the logic to a single method, which will hopefully be easier to understand and debug. This new method handles delegate state, suspend/resume, and the memory usage reporting timer together. BUG=595900 ==========
Description was changed from ========== Convert WMPI state management to level-triggered. Calling NotifyPlaybackStarted()/NotifyPlaybackPaused() at the correct times has been a recurring source of bugs. This change moves all of the logic to a single method, which will hopefully be easier to understand and debug. This new method handles delegate state, suspend/resume, and the memory usage reporting timer together. BUG=595900 ========== to ========== Convert WMPI state management to level-triggered. Calling NotifyPlaybackStarted()/NotifyPlaybackPaused() at the correct times has been a recurring source of bugs. This change moves all of the logic to a single method, which will hopefully be easier to understand and debug. This new method handles delegate state, suspend/resume, and the memory usage reporting timer together in one place. BUG=595900 ==========
Description was changed from ========== Convert WMPI state management to level-triggered. Calling NotifyPlaybackStarted()/NotifyPlaybackPaused() at the correct times has been a recurring source of bugs. This change moves all of the logic to a single method, which will hopefully be easier to understand and debug. This new method handles delegate state, suspend/resume, and the memory usage reporting timer together in one place. BUG=595900 ========== to ========== Convert WMPI state management to level-triggered. Calling NotifyPlaybackStarted()/NotifyPlaybackPaused() at the correct times has been a recurring source of bugs. This change moves all of the logic to a single method, which will hopefully be easier to understand and debug. This new method handles delegate state, suspend/resume, and the memory usage reporting timer together in one place. It's not simple, but it is commented liberally. BUG=595900 ==========
Description was changed from ========== Convert WMPI state management to level-triggered. Calling NotifyPlaybackStarted()/NotifyPlaybackPaused() at the correct times has been a recurring source of bugs. This change moves all of the logic to a single method, which will hopefully be easier to understand and debug. This new method handles delegate state, suspend/resume, and the memory usage reporting timer together in one place. It's not simple, but it is commented liberally. BUG=595900 ========== to ========== Convert WMPI state management to level-triggered. Calling NotifyPlaybackStarted()/NotifyPlaybackPaused() at the correct times has been a recurring source of bugs. This change moves all of the logic to a single method, which will hopefully be easier to understand and debug. This new method handles delegate state, suspend/resume, and the memory usage reporting timer together in one place. It's not simple, but it is commented liberally. The new suspend/resume logic does not rely on the suspend/resume callbacks (instead it's only based on the goal state, never the current state). As a result I was also able to remove the resume callback from PipelineController in this CL. BUG=595900 ==========
sandersd@chromium.org changed reviewers: + dalecurtis@chromium.org, watk@chromium.org
The CQ bit was checked by sandersd@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/1830913005/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1830913005/100001
Nice work! Can you add a test for this? The logic is fairly complicated. It also seems primarily to revolve around state changes, so it seems this could be faked pretty easily. Also if you're planning to put it in the WebMediaPlayerDelegate why not do it in this patch set? Certainly it'd be a lot easier to test if you put it there. https://codereview.chromium.org/1830913005/diff/100001/media/blink/webmediapl... File media/blink/webmediaplayer_impl.h (right): https://codereview.chromium.org/1830913005/diff/100001/media/blink/webmediapl... media/blink/webmediaplayer_impl.h:200: // TODO(sandersd): This should move into WebMediaPlayerDelegate. Why not do it in this patch set then?
On 2016/03/25 23:21:23, DaleCurtis wrote: > Nice work! Can you add a test for this? The logic is fairly complicated. It also > seems primarily to revolve around state changes, so it seems this could be faked > pretty easily. > > Also if you're planning to put it in the WebMediaPlayerDelegate why not do it in > this patch set? Certainly it'd be a lot easier to test if you put it there. > > https://codereview.chromium.org/1830913005/diff/100001/media/blink/webmediapl... > File media/blink/webmediaplayer_impl.h (right): > > https://codereview.chromium.org/1830913005/diff/100001/media/blink/webmediapl... > media/blink/webmediaplayer_impl.h:200: // TODO(sandersd): This should move into > WebMediaPlayerDelegate. > Why not do it in this patch set then? Testing via wmpi_unittest is in-progress (it won't get finished today though).
https://codereview.chromium.org/1830913005/diff/100001/media/blink/webmediapl... File media/blink/webmediaplayer_impl.h (right): https://codereview.chromium.org/1830913005/diff/100001/media/blink/webmediapl... media/blink/webmediaplayer_impl.h:200: // TODO(sandersd): This should move into WebMediaPlayerDelegate. On 2016/03/25 23:21:23, DaleCurtis wrote: > Why not do it in this patch set then? The easy answer is that the Play state has additional metadata to be passed, and so the correct API is not clear. (I suspect that the TODO for duration change will inform that choice though.) There is also a related refactoring for the other direction: OnHidden()/OnShown()/OnSuspendRequested() should be merged into a similar set of 'suspend reasons'. There is a different problem there: idle and background can be true at the same time, so the best API isn't obvious either.
Description was changed from ========== Convert WMPI state management to level-triggered. Calling NotifyPlaybackStarted()/NotifyPlaybackPaused() at the correct times has been a recurring source of bugs. This change moves all of the logic to a single method, which will hopefully be easier to understand and debug. This new method handles delegate state, suspend/resume, and the memory usage reporting timer together in one place. It's not simple, but it is commented liberally. The new suspend/resume logic does not rely on the suspend/resume callbacks (instead it's only based on the goal state, never the current state). As a result I was also able to remove the resume callback from PipelineController in this CL. BUG=595900 ========== to ========== Convert WMPI state management to level-triggered. Calling NotifyPlaybackStarted()/NotifyPlaybackPaused() at the correct times has been a recurring source of bugs. This change moves all of the logic to a single method, which will hopefully be easier to understand and debug. This new method handles delegate state, suspend/resume, and the memory usage reporting timer together in one place. It's not simple, but it is commented liberally. The new suspend/resume logic does not rely on the suspend/resume callbacks (instead it's only based on the goal state, never the current state). As a result I was also able to remove the resume callback from PipelineController in this CL. BUG=595900,597692 ==========
Awesome cleanup! Did you give any though to putting this into a standalone helper class? The interfaces seem pretty well defined. You would need something like OnReadyStateChange() though. https://codereview.chromium.org/1830913005/diff/140001/media/blink/webmediapl... File media/blink/webmediaplayer_impl.cc (left): https://codereview.chromium.org/1830913005/diff/140001/media/blink/webmediapl... media/blink/webmediaplayer_impl.cc:1118: pending_suspend_resume_cycle_ = false; You dropped this bit. I think it's still necessary. https://codereview.chromium.org/1830913005/diff/140001/media/blink/webmediapl... File media/blink/webmediaplayer_impl.cc (right): https://codereview.chromium.org/1830913005/diff/140001/media/blink/webmediapl... media/blink/webmediaplayer_impl.cc:161: highest_ready_state_(WebMediaPlayer::ReadyStateHaveNothing), Is this sufficient? What happens when a seek takes longer than idle timeout while in the paused state? https://codereview.chromium.org/1830913005/diff/140001/media/blink/webmediapl... media/blink/webmediaplayer_impl.cc:1328: // TODO(sandersd): Do we need to re-init the media session? Probably a good idea. It will really only matter if we transition from < 5 seconds to > 5 seconds. https://codereview.chromium.org/1830913005/diff/140001/media/blink/webmediapl... media/blink/webmediaplayer_impl.cc:1396: bool casting = isRemote(); Maybe just is_remote ? https://codereview.chromium.org/1830913005/diff/140001/media/blink/webmediapl... media/blink/webmediaplayer_impl.cc:1402: bool have_metadata = (ready_state_ >= WebMediaPlayer::ReadyStateHaveMetadata); nit: no unnecessary parens. ditto for below. https://codereview.chromium.org/1830913005/diff/140001/media/blink/webmediapl... File media/blink/webmediaplayer_impl.h (right): https://codereview.chromium.org/1830913005/diff/140001/media/blink/webmediapl... media/blink/webmediaplayer_impl.h:285: // Inspectes the current playback state and: Inspects
https://codereview.chromium.org/1830913005/diff/140001/media/blink/webmediapl... File media/blink/webmediaplayer_impl.cc (left): https://codereview.chromium.org/1830913005/diff/140001/media/blink/webmediapl... media/blink/webmediaplayer_impl.cc:1118: pending_suspend_resume_cycle_ = false; On 2016/03/26 01:12:55, DaleCurtis wrote: > You dropped this bit. I think it's still necessary. We need to eventually suspend, and as soon as we do this bit is cleared. |pending_suspend_resume_cycle_| no longer controls resuming; resuming happens automatically when nothing is causing a suspend (and backgrounding continues to cause a suspension in this case). https://codereview.chromium.org/1830913005/diff/140001/media/blink/webmediapl... File media/blink/webmediaplayer_impl.cc (right): https://codereview.chromium.org/1830913005/diff/140001/media/blink/webmediapl... media/blink/webmediaplayer_impl.cc:161: highest_ready_state_(WebMediaPlayer::ReadyStateHaveNothing), On 2016/03/26 01:12:55, DaleCurtis wrote: > Is this sufficient? What happens when a seek takes longer than idle timeout > while in the paused state? Currently the logic is that a suspend while seeking does not complete until after Resume(), so if the page is waiting for the seeked event to play() there will be a problem. We could change it to complete before suspending, but that prevents us from implementing seek cancellation for suspend. As a matter of compatibility, we may want to change this so that idle suspend always waits for a stable state before suspending. It would add some complexity to PipelineController but is probably manageable. For maximum compatibility, we could avoid idle suspending for the same reasons that removed video elements stay alive, but not doing that was a key goal for idle suspend. https://codereview.chromium.org/1830913005/diff/140001/media/blink/webmediapl... media/blink/webmediaplayer_impl.cc:1328: // TODO(sandersd): Do we need to re-init the media session? On 2016/03/26 01:12:55, DaleCurtis wrote: > Probably a good idea. It will really only matter if we transition from < 5 > seconds to > 5 seconds. Acknowledged. https://codereview.chromium.org/1830913005/diff/140001/media/blink/webmediapl... media/blink/webmediaplayer_impl.cc:1396: bool casting = isRemote(); On 2016/03/26 01:12:55, DaleCurtis wrote: > Maybe just is_remote ? Done. https://codereview.chromium.org/1830913005/diff/140001/media/blink/webmediapl... media/blink/webmediaplayer_impl.cc:1402: bool have_metadata = (ready_state_ >= WebMediaPlayer::ReadyStateHaveMetadata); On 2016/03/26 01:12:55, DaleCurtis wrote: > nit: no unnecessary parens. ditto for below. Done. https://codereview.chromium.org/1830913005/diff/140001/media/blink/webmediapl... File media/blink/webmediaplayer_impl.h (right): https://codereview.chromium.org/1830913005/diff/140001/media/blink/webmediapl... media/blink/webmediaplayer_impl.h:285: // Inspectes the current playback state and: On 2016/03/26 01:12:55, DaleCurtis wrote: > Inspects Done.
On 2016/03/26 01:12:55, DaleCurtis wrote: > Awesome cleanup! Did you give any though to putting this into a standalone > helper class? The interfaces seem pretty well defined. You would need something > like OnReadyStateChange() though. I took a look at this, but I didn't like how it required proxying all of the delegate_ operations. Two possible fixes for that: (1) Allow multiple registrations with the same delegate ID (2) Separate the delegate interface so that the suspend-related ones are not tied to OnPlay()/OnPause(). Along with some of the other API questions here it seems like the delagate API needs a complete re-think, but what we have now is working for this CL. I'll stick with a friend test class for now.
This is really nice. https://codereview.chromium.org/1830913005/diff/140001/media/blink/webmediapl... File media/blink/webmediaplayer_impl.cc (right): https://codereview.chromium.org/1830913005/diff/140001/media/blink/webmediapl... media/blink/webmediaplayer_impl.cc:1439: // similer, but excludes idle suspension, because in the audio-only case we similar https://codereview.chromium.org/1830913005/diff/140001/media/blink/webmediapl... media/blink/webmediaplayer_impl.cc:1440: // expect that the notification controls to remain. We also require: s/that the/the https://codereview.chromium.org/1830913005/diff/140001/media/blink/webmediapl... media/blink/webmediaplayer_impl.cc:1486: } else if (memory_usage_reporting_timer_.IsRunning()) { Should this be: else if (!is_playing && memory_usage_reporting_timer.IsRunning())? https://codereview.chromium.org/1830913005/diff/160001/media/blink/webmediapl... File media/blink/webmediaplayer_impl.cc (right): https://codereview.chromium.org/1830913005/diff/160001/media/blink/webmediapl... media/blink/webmediaplayer_impl.cc:1440: // later case. |ended_| does result in a separate paused state though, to did you mean s/later/latter/?
https://codereview.chromium.org/1830913005/diff/140001/media/blink/webmediapl... File media/blink/webmediaplayer_impl.cc (right): https://codereview.chromium.org/1830913005/diff/140001/media/blink/webmediapl... media/blink/webmediaplayer_impl.cc:1439: // similer, but excludes idle suspension, because in the audio-only case we On 2016/03/29 00:47:53, watk wrote: > similar Acknowledged. https://codereview.chromium.org/1830913005/diff/140001/media/blink/webmediapl... media/blink/webmediaplayer_impl.cc:1440: // expect that the notification controls to remain. We also require: On 2016/03/29 00:47:53, watk wrote: > s/that the/the Done. https://codereview.chromium.org/1830913005/diff/140001/media/blink/webmediapl... media/blink/webmediaplayer_impl.cc:1486: } else if (memory_usage_reporting_timer_.IsRunning()) { On 2016/03/29 00:47:53, watk wrote: > Should this be: else if (!is_playing && > memory_usage_reporting_timer.IsRunning())? Acknowledged. https://codereview.chromium.org/1830913005/diff/160001/media/blink/webmediapl... File media/blink/webmediaplayer_impl.cc (right): https://codereview.chromium.org/1830913005/diff/160001/media/blink/webmediapl... media/blink/webmediaplayer_impl.cc:1440: // later case. |ended_| does result in a separate paused state though, to On 2016/03/29 00:47:53, watk wrote: > did you mean s/later/latter/? Done.
And now with tests. PTAL.
https://codereview.chromium.org/1830913005/diff/200001/media/blink/webmediapl... File media/blink/webmediaplayer_impl.cc (left): https://codereview.chromium.org/1830913005/diff/200001/media/blink/webmediapl... media/blink/webmediaplayer_impl.cc:1154: #if defined(OS_MACOSX) This was dropped. https://codereview.chromium.org/1830913005/diff/200001/media/blink/webmediapl... File media/blink/webmediaplayer_impl.cc (right): https://codereview.chromium.org/1830913005/diff/200001/media/blink/webmediapl... media/blink/webmediaplayer_impl.cc:122: bool result = (state == blink::WebMediaPlayer::NetworkStateFormatError || Remove unnecessary parens. https://codereview.chromium.org/1830913005/diff/200001/media/blink/webmediapl... media/blink/webmediaplayer_impl.cc:1402: bool is_backgrounded = Naming is a bit confusing when you mix in IsSuspendUponHiddenEnabled() https://codereview.chromium.org/1830913005/diff/200001/media/blink/webmediapl... media/blink/webmediaplayer_impl.cc:1524: // TODO(sandersd): There should be a paused+seeking state, during which Should OnSuspend() just ignore the request if it's in the seeking state? Calling DidPause() a second time will readd the delegate to the idle list. https://codereview.chromium.org/1830913005/diff/200001/media/blink/webmediapl... File media/blink/webmediaplayer_impl.h (right): https://codereview.chromium.org/1830913005/diff/200001/media/blink/webmediapl... media/blink/webmediaplayer_impl.h:301: // This method should be called any time its dependend values change. These Dependent. Instead of trying to enforce this via a comment, should we stick those values in a private inner class w/ setters so we can guarantee this occurs? Update: Discussed offline, we need to change multiple values at once and don't want to call UpdatePlayState for each one. We should still come up with a stronger protection here, but can be done in a followup. https://codereview.chromium.org/1830913005/diff/200001/media/blink/webmediapl... media/blink/webmediaplayer_impl.h:338: // TODO(hclam): get rid of these members and read from the pipeline directly. Delete? :)
https://codereview.chromium.org/1830913005/diff/200001/media/blink/webmediapl... File media/blink/webmediaplayer_impl.cc (left): https://codereview.chromium.org/1830913005/diff/200001/media/blink/webmediapl... media/blink/webmediaplayer_impl.cc:1154: #if defined(OS_MACOSX) On 2016/04/01 18:25:48, DaleCurtis wrote: > This was dropped. Done. https://codereview.chromium.org/1830913005/diff/200001/media/blink/webmediapl... File media/blink/webmediaplayer_impl.cc (right): https://codereview.chromium.org/1830913005/diff/200001/media/blink/webmediapl... media/blink/webmediaplayer_impl.cc:122: bool result = (state == blink::WebMediaPlayer::NetworkStateFormatError || On 2016/04/01 18:25:48, DaleCurtis wrote: > Remove unnecessary parens. Done. https://codereview.chromium.org/1830913005/diff/200001/media/blink/webmediapl... media/blink/webmediaplayer_impl.cc:1402: bool is_backgrounded = On 2016/04/01 18:25:48, DaleCurtis wrote: > Naming is a bit confusing when you mix in IsSuspendUponHiddenEnabled() Done. https://codereview.chromium.org/1830913005/diff/200001/media/blink/webmediapl... media/blink/webmediaplayer_impl.cc:1524: // TODO(sandersd): There should be a paused+seeking state, during which On 2016/04/01 18:25:48, DaleCurtis wrote: > Should OnSuspend() just ignore the request if it's in the seeking state? Calling > DidPause() a second time will readd the delegate to the idle list. This is a plausible option, I'll implement it (or the full solution) in a followup. https://codereview.chromium.org/1830913005/diff/200001/media/blink/webmediapl... File media/blink/webmediaplayer_impl.h (right): https://codereview.chromium.org/1830913005/diff/200001/media/blink/webmediapl... media/blink/webmediaplayer_impl.h:301: // This method should be called any time its dependend values change. These On 2016/04/01 18:25:48, DaleCurtis wrote: > Dependent. Instead of trying to enforce this via a comment, should we stick > those values in a private inner class w/ setters so we can guarantee this > occurs? Update: Discussed offline, we need to change multiple values at once and > don't want to call UpdatePlayState for each one. > > We should still come up with a stronger protection here, but can be done in a > followup. Acknowledged. https://codereview.chromium.org/1830913005/diff/200001/media/blink/webmediapl... media/blink/webmediaplayer_impl.h:338: // TODO(hclam): get rid of these members and read from the pipeline directly. On 2016/04/01 18:25:48, DaleCurtis wrote: > Delete? :) Done.
lgtm
lgtm https://codereview.chromium.org/1830913005/diff/220001/media/blink/webmediapl... File media/blink/webmediaplayer_impl_unittest.cc (right): https://codereview.chromium.org/1830913005/diff/220001/media/blink/webmediapl... media/blink/webmediaplayer_impl_unittest.cc:202: EXPECT_EQ(WebMediaPlayerImpl::DelegateState::GONE, state.delegate_state); All these WebMediaPlayerImpls only add noise. I wonder if you could do using DelegateState = WebMediaPlayerImpl::DelegateState or something?
https://codereview.chromium.org/1830913005/diff/220001/media/blink/webmediapl... File media/blink/webmediaplayer_impl_unittest.cc (right): https://codereview.chromium.org/1830913005/diff/220001/media/blink/webmediapl... media/blink/webmediaplayer_impl_unittest.cc:202: EXPECT_EQ(WebMediaPlayerImpl::DelegateState::GONE, state.delegate_state); On 2016/04/02 00:12:53, watk wrote: > All these WebMediaPlayerImpls only add noise. I wonder if you could do using > DelegateState = WebMediaPlayerImpl::DelegateState or something? Ack. I think I'll hold off for now, hopefully there is an overall more clear structure waiting to be found.
The CQ bit was checked by sandersd@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/1830913005/220001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1830913005/220001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_chromium_gn_compile_dbg on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_chro...)
The CQ bit was checked by sandersd@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/1830913005/260001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1830913005/260001
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 sandersd@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from watk@chromium.org, dalecurtis@chromium.org Link to the patchset: https://codereview.chromium.org/1830913005/#ps260001 (title: "Use EXPECT_TRUE/EXPECT_FALSE.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1830913005/260001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1830913005/260001
Message was sent while issue was closed.
Description was changed from ========== Convert WMPI state management to level-triggered. Calling NotifyPlaybackStarted()/NotifyPlaybackPaused() at the correct times has been a recurring source of bugs. This change moves all of the logic to a single method, which will hopefully be easier to understand and debug. This new method handles delegate state, suspend/resume, and the memory usage reporting timer together in one place. It's not simple, but it is commented liberally. The new suspend/resume logic does not rely on the suspend/resume callbacks (instead it's only based on the goal state, never the current state). As a result I was also able to remove the resume callback from PipelineController in this CL. BUG=595900,597692 ========== to ========== Convert WMPI state management to level-triggered. Calling NotifyPlaybackStarted()/NotifyPlaybackPaused() at the correct times has been a recurring source of bugs. This change moves all of the logic to a single method, which will hopefully be easier to understand and debug. This new method handles delegate state, suspend/resume, and the memory usage reporting timer together in one place. It's not simple, but it is commented liberally. The new suspend/resume logic does not rely on the suspend/resume callbacks (instead it's only based on the goal state, never the current state). As a result I was also able to remove the resume callback from PipelineController in this CL. BUG=595900,597692 ==========
Message was sent while issue was closed.
Committed patchset #11 (id:260001)
Message was sent while issue was closed.
Description was changed from ========== Convert WMPI state management to level-triggered. Calling NotifyPlaybackStarted()/NotifyPlaybackPaused() at the correct times has been a recurring source of bugs. This change moves all of the logic to a single method, which will hopefully be easier to understand and debug. This new method handles delegate state, suspend/resume, and the memory usage reporting timer together in one place. It's not simple, but it is commented liberally. The new suspend/resume logic does not rely on the suspend/resume callbacks (instead it's only based on the goal state, never the current state). As a result I was also able to remove the resume callback from PipelineController in this CL. BUG=595900,597692 ========== to ========== Convert WMPI state management to level-triggered. Calling NotifyPlaybackStarted()/NotifyPlaybackPaused() at the correct times has been a recurring source of bugs. This change moves all of the logic to a single method, which will hopefully be easier to understand and debug. This new method handles delegate state, suspend/resume, and the memory usage reporting timer together in one place. It's not simple, but it is commented liberally. The new suspend/resume logic does not rely on the suspend/resume callbacks (instead it's only based on the goal state, never the current state). As a result I was also able to remove the resume callback from PipelineController in this CL. BUG=595900,597692 Committed: https://crrev.com/50a635ebbf250f8f35ea060d564b102b804dcea7 Cr-Commit-Position: refs/heads/master@{#385038} ==========
Message was sent while issue was closed.
Patchset 11 (id:??) landed as https://crrev.com/50a635ebbf250f8f35ea060d564b102b804dcea7 Cr-Commit-Position: refs/heads/master@{#385038} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
