Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1961273002:
Disable idle suspend for Chromecast (Closed)
|
Created:
4 years, 7 months ago by halliwell Modified:
4 years, 6 months ago CC:
chromium-reviews, feature-media-reviews_chromium.org, halliwell+watch_chromium.org, lcwu+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionDisable idle suspend for Chromecast
Idle suspend is causing the screen to go black when users leave
media paused.
BUG=internal b/28635426
Committed: https://crrev.com/6451e24920bf43abefb3a21655094b2bb3d601f3
Cr-Commit-Position: refs/heads/master@{#397142}
Patch Set 1 #Patch Set 2 : Allow flag to work on platforms besides OSX/Windows #Patch Set 3 : Don't disable suspend on Android #
Total comments: 1
Patch Set 4 : Extend ifdef to include CHROMEOS #Patch Set 5 : rebase #Messages
Total messages: 31 (8 generated)
Description was changed from ========== Disable idle suspend for Chromecast Idle suspend is causing the screen to go black when users leave media paused. BUG=internal b/28635426 ========== to ========== Disable idle suspend for Chromecast Idle suspend is causing the screen to go black when users leave media paused. BUG=internal b/28635426 ==========
halliwell@chromium.org changed reviewers: + alokp@chromium.org, dalecurtis@chromium.org, sandersd@chromium.org
Dale, Dan: I notice you have a TODO to eliminate this flag. If there is a better way, let me know. I think it is unlikely that we will have a short-term path to allowing video content to persist on screen after suspend. However, we are attempting to move to mojo pipeline, so the mechanism for disabling suspend could be different there?
I don't have a better way for you immediately, but it does seem that this should be opt-in instead of opt-out; more decoders are having problems with it than are working. (Mojo doesn't completely fix this, but it will allow VideoFrames to outlive an out-of-process decoder, which is most of the problem.) lgtm
You'll need to update the #if in WMPI to get this to work. This should probably instead be exposed as a setting to the content embedder that WMPI can accept on construction.
On 2016/05/10 19:39:38, DaleCurtis_OOO_Until_May_16 wrote: > You'll need to update the #if in WMPI to get this to work. This should probably > instead be exposed as a setting to the content embedder that WMPI can accept on > construction. Good point. It's actually impossible to change that #if to support Chromecast without changing the existing logic slightly. Which might be ok, but I don't have enough context ... I put together this change instead with a content embedder API, PTAL: https://codereview.chromium.org/1972783003/
On 2016/05/12 03:51:39, halliwell wrote: > On 2016/05/10 19:39:38, DaleCurtis_OOO_Until_May_16 wrote: > > You'll need to update the #if in WMPI to get this to work. This should > probably > > instead be exposed as a setting to the content embedder that WMPI can accept > on > > construction. > > Good point. It's actually impossible to change that #if to support Chromecast > without changing the existing logic slightly. Which might be ok, but I don't > have enough context ... > > I put together this change instead with a content embedder API, PTAL: > https://codereview.chromium.org/1972783003/ The other review has led me rather out of my depth, at least for the time I have available to fix this. So I've come back to this as a simpler way forward for the short term at least. Here, I've removed the Windows/OSX ifdef from the video frame metadata flag check. Since the flag is only set on those platforms + Chromecast, it shouldn't affect any other platform behaviour. PTAL.
This is not correct anymore since Android does not want this behavior.
On 2016/05/27 19:17:18, DaleCurtis wrote: > This is not correct anymore since Android does not want this behavior. I didn't think Android set the DECODER_OWNS_FRAME flag to true?
Android uses a VDA now, so it sets this flag.
On 2016/05/27 19:30:30, DaleCurtis wrote: > Android uses a VDA now, so it sets this flag. Updated to exclude Android. I still don't entirely understand though: * I only see the flag being set here, which is restricted to OSX and Windows: https://code.google.com/p/chromium/codesearch#chromium/src/media/filters/gpu_... * I only see the flag being read here in WMPI, so if Android doesn't want this behaviour, it could just not set the flag? What am I missing?
On 2016/05/27 at 19:50:53, halliwell wrote: > On 2016/05/27 19:30:30, DaleCurtis wrote: > > Android uses a VDA now, so it sets this flag. > > Updated to exclude Android. > > I still don't entirely understand though: > * I only see the flag being set here, which is restricted to OSX and Windows: https://code.google.com/p/chromium/codesearch#chromium/src/media/filters/gpu_... > * I only see the flag being read here in WMPI, so if Android doesn't want this behaviour, it could just not set the flag? > > What am I missing? That flag is set for all platforms which use VDAs. We only want to disable suspend for Windows, OSX (probably not much longer), and now Chromecast. So this is still incorrect. This functionality works fine on ChromeOS (and is in fact highly desired there due to limited resources). I don't think there's a way to do this correctly at this location w/o a Chromecast define.
On 2016/05/27 20:21:10, DaleCurtis wrote: > On 2016/05/27 at 19:50:53, halliwell wrote: > > On 2016/05/27 19:30:30, DaleCurtis wrote: > > > Android uses a VDA now, so it sets this flag. > > > > Updated to exclude Android. > > > > I still don't entirely understand though: > > * I only see the flag being set here, which is restricted to OSX and Windows: > https://code.google.com/p/chromium/codesearch#chromium/src/media/filters/gpu_... > > * I only see the flag being read here in WMPI, so if Android doesn't want this > behaviour, it could just not set the flag? > > > > What am I missing? > > That flag is set for all platforms which use VDAs. We only want to disable > suspend for Windows, OSX (probably not much longer), and now Chromecast. So this > is still incorrect. This functionality works fine on ChromeOS (and is in fact > highly desired there due to limited resources). I don't think there's a way to > do this correctly at this location w/o a Chromecast define. Where is the flag set? And what is it used for besides disabling idle suspend? There is something I'm still missing here.
On 2016/05/27 at 23:45:04, halliwell wrote: > On 2016/05/27 20:21:10, DaleCurtis wrote: > > On 2016/05/27 at 19:50:53, halliwell wrote: > > > On 2016/05/27 19:30:30, DaleCurtis wrote: > > > > Android uses a VDA now, so it sets this flag. > > > > > > Updated to exclude Android. > > > > > > I still don't entirely understand though: > > > * I only see the flag being set here, which is restricted to OSX and Windows: > > https://code.google.com/p/chromium/codesearch#chromium/src/media/filters/gpu_... > > > * I only see the flag being read here in WMPI, so if Android doesn't want this > > behaviour, it could just not set the flag? > > > > > > What am I missing? > > > > That flag is set for all platforms which use VDAs. We only want to disable > > suspend for Windows, OSX (probably not much longer), and now Chromecast. So this > > is still incorrect. This functionality works fine on ChromeOS (and is in fact > > highly desired there due to limited resources). I don't think there's a way to > > do this correctly at this location w/o a Chromecast define. > > Where is the flag set? And what is it used for besides disabling idle suspend? > There is something I'm still missing here. It's set in GpuVideoDecoder which is used by all of those platforms. I guess I don't understand your confusion :)
On 2016/05/28 00:07:53, DaleCurtis wrote: > On 2016/05/27 at 23:45:04, halliwell wrote: > > On 2016/05/27 20:21:10, DaleCurtis wrote: > > > On 2016/05/27 at 19:50:53, halliwell wrote: > > > > On 2016/05/27 19:30:30, DaleCurtis wrote: > > > > > Android uses a VDA now, so it sets this flag. > > > > > > > > Updated to exclude Android. > > > > > > > > I still don't entirely understand though: > > > > * I only see the flag being set here, which is restricted to OSX and > Windows: > > > > https://code.google.com/p/chromium/codesearch#chromium/src/media/filters/gpu_... > > > > * I only see the flag being read here in WMPI, so if Android doesn't want > this > > > behaviour, it could just not set the flag? > > > > > > > > What am I missing? > > > > > > That flag is set for all platforms which use VDAs. We only want to disable > > > suspend for Windows, OSX (probably not much longer), and now Chromecast. So > this > > > is still incorrect. This functionality works fine on ChromeOS (and is in > fact > > > highly desired there due to limited resources). I don't think there's a way > to > > > do this correctly at this location w/o a Chromecast define. > > > > Where is the flag set? And what is it used for besides disabling idle > suspend? > > There is something I'm still missing here. > > It's set in GpuVideoDecoder which is used by all of those platforms. I guess I > don't understand your confusion :) But it's only set for Windows and Mac: #if defined(OS_MACOSX) || defined(OS_WIN) frame->metadata()->SetBoolean(VideoFrameMetadata::DECODER_OWNS_FRAME, true); #endif
Oh, sorry I'm the one confused. I forgot we did that. In which case this code should be fine. lgtm
You can remove the !ifdef completely from wmpi now though.
The reason I did that originally is because GetCurrentFrameFromCompositor is a sync post across two threads :/
As a consequence of that last bit, lets leave and expand the #if on performance sensitive platforms. Long term as we fix Windows and Mac though we'll need a different solution for Chromecast. https://codereview.chromium.org/1961273002/diff/40001/media/blink/webmediapla... File media/blink/webmediaplayer_impl.cc (right): https://codereview.chromium.org/1961273002/diff/40001/media/blink/webmediapla... media/blink/webmediaplayer_impl.cc:1455: #if !defined(OS_ANDROID) OS_ANDROID, OS_CHROMEOS are worth keeping I think.
On 2016/05/28 00:23:32, DaleCurtis wrote: > As a consequence of that last bit, lets leave and expand the #if on performance > sensitive platforms. Long term as we fix Windows and Mac though we'll need a > different solution for Chromecast. > > https://codereview.chromium.org/1961273002/diff/40001/media/blink/webmediapla... > File media/blink/webmediaplayer_impl.cc (right): > > https://codereview.chromium.org/1961273002/diff/40001/media/blink/webmediapla... > media/blink/webmediaplayer_impl.cc:1455: #if !defined(OS_ANDROID) > OS_ANDROID, OS_CHROMEOS are worth keeping I think. Aha, gotcha ... thanks for the explanation. Will make it Android+ChromeOS as you suggest, and yes, we can look for a longer-term solution for Chromecast beyond this.
The CQ bit was checked by halliwell@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sandersd@chromium.org, dalecurtis@chromium.org Link to the patchset: https://codereview.chromium.org/1961273002/#ps80001 (title: "rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1961273002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1961273002/80001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by halliwell@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1961273002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1961273002/80001
Message was sent while issue was closed.
Description was changed from ========== Disable idle suspend for Chromecast Idle suspend is causing the screen to go black when users leave media paused. BUG=internal b/28635426 ========== to ========== Disable idle suspend for Chromecast Idle suspend is causing the screen to go black when users leave media paused. BUG=internal b/28635426 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Disable idle suspend for Chromecast Idle suspend is causing the screen to go black when users leave media paused. BUG=internal b/28635426 ========== to ========== Disable idle suspend for Chromecast Idle suspend is causing the screen to go black when users leave media paused. BUG=internal b/28635426 Committed: https://crrev.com/6451e24920bf43abefb3a21655094b2bb3d601f3 Cr-Commit-Position: refs/heads/master@{#397142} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/6451e24920bf43abefb3a21655094b2bb3d601f3 Cr-Commit-Position: refs/heads/master@{#397142} |
