Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1766783003:
Expand suspension of idle media players to all platforms. (Closed)
|
Created:
4 years, 9 months ago by DaleCurtis Modified:
4 years, 9 months ago CC:
avayvod+watch_chromium.org, chromium-reviews, darin-cc_chromium.org, feature-media-reviews_chromium.org, jam, mcasas+watch_chromium.org, mkwst+moarreviews-renderer_chromium.org, mlamouri+watch-content_chromium.org, mlamouri+watch-media_chromium.org, posciak+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@notify_pause Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionExpand suspension of idle media players to all platforms.
This CL enables suspension of idle players on the desktop with the same
timers as used on Android: 15 seconds idle (paused, ended, never started),
polled every 5 seconds.
It also replaces the overloaded OnHidden() method with a more appropriate
OnSuspendRequested() method for handling both forced and idle suspensions;
OnHidden() continues to handle suspensions upon backgrounding on Android.
By separating suspends into a new method we can always suspend idle players
without worrying about if they have a media session or not. If they do and
the user interacts with the session, we'll resume the player dynamically.
BUG=590099
TEST=updated unittest, manual testing.
Committed: https://crrev.com/0431cbf89bc6bc50c9ad2ce0cc4ac5117e38b1d9
Cr-Commit-Position: refs/heads/master@{#380819}
Patch Set 1 #Patch Set 2 : Cleanup. #
Total comments: 21
Patch Set 3 : Simplify. #
Total comments: 12
Patch Set 4 : Fixup API comments. #Messages
Total messages: 28 (9 generated)
Description was changed from ========== Add OnSuspend() instead of overloading OnHidden() for WMP delegates. The overload of OnHidden() for forced suspend operations prevented us from suspending players which may have a media session attached; it was also a bit ugly. By separating forced suspends into a new method we can always suspend idle (paused, ended, never started) players without worrying about if they have a media session or not. If they do and the user interacts with the session, we'll resume the player dynamically. This CL also enables suspension of idle players on the desktop. BUG=590099 TEST=updated unittest manual testing. ========== to ========== Add OnSuspend() instead of overloading OnHidden() for WMP delegates. The overload of OnHidden() for forced suspend operations prevented us from suspending players which may have a media session attached; it was also a bit ugly. By separating forced suspends into a new method we can always suspend idle (paused, ended, never started) players without worrying about if they have a media session or not. If they do and the user interacts with the session, we'll resume the player dynamically. This CL also enables suspension of idle players on the desktop with the same timers as used on Android: 15 seconds idle (paused, ended, or never started) polled every 5 seconds. BUG=590099 TEST=updated unittest manual testing. ==========
dalecurtis@chromium.org changed reviewers: + sandersd@chromium.org, xhwang@chromium.org
This brings allows us to always suspend idle players on Android (without impacting the media session) and brings suspend / resume to the desktop for idle players. Needs more local testing, but the ideas are ready for review.
mlamouri@chromium.org changed reviewers: + mlamouri@chromium.org
https://codereview.chromium.org/1766783003/diff/20001/content/renderer/media/... File content/renderer/media/renderer_webmediaplayer_delegate.cc (right): https://codereview.chromium.org/1766783003/diff/20001/content/renderer/media/... content/renderer/media/renderer_webmediaplayer_delegate.cc:158: id_map_.Lookup(kv.first)->OnSuspend(false); Why did this changed from OnHidden(true) to OnSuspend(false)? https://codereview.chromium.org/1766783003/diff/20001/media/blink/webmediapla... File media/blink/webmediaplayer_impl.cc (right): https://codereview.chromium.org/1766783003/diff/20001/media/blink/webmediapla... media/blink/webmediaplayer_impl.cc:370: : BackgroundPlaybackState::PLAY_ALLOWED_WHEN_SUSPENDED; Should this include IsBackgroundSuspendResumeEnabled()? https://codereview.chromium.org/1766783003/diff/20001/media/blink/webmediapla... media/blink/webmediaplayer_impl.cc:1455: (delegate_ && !delegate_->IsHidden()); Should this include IsBackgroundSuspendResumeEnabled()? https://codereview.chromium.org/1766783003/diff/20001/media/blink/webmediapla... File media/blink/webmediaplayer_impl.h (right): https://codereview.chromium.org/1766783003/diff/20001/media/blink/webmediapla... media/blink/webmediaplayer_impl.h:428: // Indicates if playback may be started from the background via a to play from nit: "via a to play from JavaScript" might be missing "call"? (and maybe "play" should be "play()")
Will wait for other reviewers to way in before uploading a new patch set. Just replying comments. https://codereview.chromium.org/1766783003/diff/20001/content/renderer/media/... File content/renderer/media/renderer_webmediaplayer_delegate.cc (right): https://codereview.chromium.org/1766783003/diff/20001/content/renderer/media/... content/renderer/media/renderer_webmediaplayer_delegate.cc:158: id_map_.Lookup(kv.first)->OnSuspend(false); On 2016/03/05 at 22:37:36, Mounir Lamouri wrote: > Why did this changed from OnHidden(true) to OnSuspend(false)? |must_suspend| requires calling PlayerGone(), which we don't want here since a MediaSession might be active. https://codereview.chromium.org/1766783003/diff/20001/media/blink/webmediapla... File media/blink/webmediaplayer_impl.cc (right): https://codereview.chromium.org/1766783003/diff/20001/media/blink/webmediapla... media/blink/webmediaplayer_impl.cc:1455: (delegate_ && !delegate_->IsHidden()); On 2016/03/05 at 22:37:36, Mounir Lamouri wrote: > Should this include IsBackgroundSuspendResumeEnabled()? No, I tried to make it clear that IsBackgroundSuspendResumeEnabled() is _only_ for enabling automatic suspension (with clearing of MediaSession) on backgrounding. I.e., it's not for suspending idle players as the opportunity arises. Let me know if there's any way I can make this clearer. Thanks!
Didn't review everything yet. Just a few questions on the API. https://chromiumcodereview.appspot.com/1766783003/diff/20001/media/blink/webm... File media/blink/webmediaplayer_delegate.h (right): https://chromiumcodereview.appspot.com/1766783003/diff/20001/media/blink/webm... media/blink/webmediaplayer_delegate.h:27: // players must call WebMediaPlayerDelegate::PlayerGone(). It seems PlayerGone() may also be called in OnHidden() or OnSuspend(false). We should make this more clear. https://chromiumcodereview.appspot.com/1766783003/diff/20001/media/blink/webm... media/blink/webmediaplayer_delegate.h:28: virtual void OnSuspend(bool must_suspend) = 0; It seems there's some overlap between OnHidden() and OnSuspend(). For example, when a player enters background and we want to suspend video but keep playing background audio, should we call OnHidden(), or OnSuspend(false)? https://chromiumcodereview.appspot.com/1766783003/diff/20001/media/blink/webm... File media/blink/webmediaplayer_impl.h (right): https://chromiumcodereview.appspot.com/1766783003/diff/20001/media/blink/webm... media/blink/webmediaplayer_impl.h:431: PLAY_FORBIDDEN_NEVER_CALLED_IN_FOREGROUND, Can you provide more comments around these states? It's not super obvious to me, especially the "FORBIDDEN" part here.
https://chromiumcodereview.appspot.com/1766783003/diff/20001/media/blink/webm... File media/blink/webmediaplayer_delegate.h (right): https://chromiumcodereview.appspot.com/1766783003/diff/20001/media/blink/webm... media/blink/webmediaplayer_delegate.h:28: virtual void OnSuspend(bool must_suspend) = 0; On 2016/03/07 at 19:13:58, xhwang wrote: > It seems there's some overlap between OnHidden() and OnSuspend(). For example, when a player enters background and we want to suspend video but keep playing background audio, should we call OnHidden(), or OnSuspend(false)? With this change OnHidden() should only ever be called for transitions into the background. Likewise, OnSuspend() will be called only in cases where we're requesting suspend for release of idle resources or requiring suspend for a pending undoable tab closure. sandersd@ suggested some time ago that instead of having a bool we have an enum like SuspendReason { OPTIONAL_FOR_IDLE_RESOURCES, MANDATORY_FOR_PENDING_DESTRUCTION } I couldn't come up with good names so I kept the bool instead, but I'm open to suggestions :) https://chromiumcodereview.appspot.com/1766783003/diff/20001/media/blink/webm... File media/blink/webmediaplayer_impl.h (right): https://chromiumcodereview.appspot.com/1766783003/diff/20001/media/blink/webm... media/blink/webmediaplayer_impl.h:431: PLAY_FORBIDDEN_NEVER_CALLED_IN_FOREGROUND, On 2016/03/07 at 19:13:58, xhwang wrote: > Can you provide more comments around these states? It's not super obvious to me, especially the "FORBIDDEN" part here. Yeah my names are bad :( I was trying to capture "playback has never been started in the foreground before, so don't let autoplay / js actually start playback in the background." This is similar to what we do where we never load the media until the page is in the foreground unless playback has ever started in that tab before. The others are essentially "don't play video in the background" and "play whatever you want in response to autoplay / js starting playback."
https://chromiumcodereview.appspot.com/1766783003/diff/20001/media/blink/webm... File media/blink/webmediaplayer_delegate.h (right): https://chromiumcodereview.appspot.com/1766783003/diff/20001/media/blink/webm... media/blink/webmediaplayer_delegate.h:28: virtual void OnSuspend(bool must_suspend) = 0; On 2016/03/07 at 19:13:58, xhwang wrote: > It seems there's some overlap between OnHidden() and OnSuspend(). For example, when a player enters background and we want to suspend video but keep playing background audio, should we call OnHidden(), or OnSuspend(false)? With this change OnHidden() should only ever be called for transitions into the background. Likewise, OnSuspend() will be called only in cases where we're requesting suspend for release of idle resources or requiring suspend for a pending undoable tab closure. sandersd@ suggested some time ago that instead of having a bool we have an enum like SuspendReason { OPTIONAL_FOR_IDLE_RESOURCES, MANDATORY_FOR_PENDING_DESTRUCTION } I couldn't come up with good names so I kept the bool instead, but I'm open to suggestions :) https://chromiumcodereview.appspot.com/1766783003/diff/20001/media/blink/webm... File media/blink/webmediaplayer_impl.h (right): https://chromiumcodereview.appspot.com/1766783003/diff/20001/media/blink/webm... media/blink/webmediaplayer_impl.h:431: PLAY_FORBIDDEN_NEVER_CALLED_IN_FOREGROUND, On 2016/03/07 at 19:13:58, xhwang wrote: > Can you provide more comments around these states? It's not super obvious to me, especially the "FORBIDDEN" part here. Yeah my names are bad :( I was trying to capture "playback has never been started in the foreground before, so don't let autoplay / js actually start playback in the background." This is similar to what we do where we never load the media until the page is in the foreground unless playback has ever started in that tab before. The others are essentially "don't play video in the background" and "play whatever you want in response to autoplay / js starting playback."
https://codereview.chromium.org/1766783003/diff/20001/media/blink/webmediapla... File media/blink/webmediaplayer_impl.cc (right): https://codereview.chromium.org/1766783003/diff/20001/media/blink/webmediapla... media/blink/webmediaplayer_impl.cc:366: BackgroundPlaybackState::PLAY_FORBIDDEN_NEVER_CALLED_IN_FOREGROUND && The existence of this check isn't immediately necessary from the names, which probably means that the enum names need work. (It's still true in this case that play() was never called in the foreground, so why would it change?) I'm no better at naming than you are, though. https://codereview.chromium.org/1766783003/diff/20001/media/blink/webmediapla... media/blink/webmediaplayer_impl.cc:374: NotifyPlaybackStarted(); Add a comment here that this method won't do anything when suspended. I get the feeling it's not obvious due to the name, but I'm not going to ask for a rename in this CL. https://codereview.chromium.org/1766783003/diff/20001/media/blink/webmediapla... File media/blink/webmediaplayer_impl.h (right): https://codereview.chromium.org/1766783003/diff/20001/media/blink/webmediapla... media/blink/webmediaplayer_impl.h:172: void OnSuspend(bool must_suspend) override; I keep getting tripped up on this bool, so +1 for an enum here. https://codereview.chromium.org/1766783003/diff/20001/media/blink/webmediapla... media/blink/webmediaplayer_impl.h:283: // Indicates if Resume() or Suspend() are allowed to be called respectively. I'm not clear on this comment given that there is only one method. Perhaps better would be "Indicates if automatic resumption is allowed in the current state", or something similar that indicates when this method is called. (Without that the name sounds like it should be const.)
Description was changed from ========== Add OnSuspend() instead of overloading OnHidden() for WMP delegates. The overload of OnHidden() for forced suspend operations prevented us from suspending players which may have a media session attached; it was also a bit ugly. By separating forced suspends into a new method we can always suspend idle (paused, ended, never started) players without worrying about if they have a media session or not. If they do and the user interacts with the session, we'll resume the player dynamically. This CL also enables suspension of idle players on the desktop with the same timers as used on Android: 15 seconds idle (paused, ended, or never started) polled every 5 seconds. BUG=590099 TEST=updated unittest manual testing. ========== to ========== Expand suspension of idle media players to all platforms. This CL enables suspension of idle players on the desktop with the same timers as used on Android: 15 seconds idle (paused, ended, never started), polled every 5 seconds. It also replaces the overloaded OnHidden() method with a more appropriate OnSuspendRequested() method for handling both forced and idle suspensions; OnHidden() continues to handle suspensions upon backgrounding on Android. By separating suspends into a new method we can always suspend idle players without worrying about if they have a media session or not. If they do and the user interacts with the session, we'll resume the player dynamically. BUG=590099 TEST=updated unittest, manual testing. ==========
PTAL. I've simplified the approach a lot and updated the CL description to match. https://codereview.chromium.org/1766783003/diff/20001/media/blink/webmediapla... File media/blink/webmediaplayer_delegate.h (right): https://codereview.chromium.org/1766783003/diff/20001/media/blink/webmediapla... media/blink/webmediaplayer_delegate.h:27: // players must call WebMediaPlayerDelegate::PlayerGone(). On 2016/03/07 at 19:13:58, xhwang wrote: > It seems PlayerGone() may also be called in OnHidden() or OnSuspend(false). We should make this more clear. Done. https://codereview.chromium.org/1766783003/diff/20001/media/blink/webmediapla... File media/blink/webmediaplayer_impl.cc (right): https://codereview.chromium.org/1766783003/diff/20001/media/blink/webmediapla... media/blink/webmediaplayer_impl.cc:366: BackgroundPlaybackState::PLAY_FORBIDDEN_NEVER_CALLED_IN_FOREGROUND && On 2016/03/07 at 23:27:51, sandersd wrote: > The existence of this check isn't immediately necessary from the names, which probably means that the enum names need work. (It's still true in this case that play() was never called in the foreground, so why would it change?) > > I'm no better at naming than you are, though. Removed. https://codereview.chromium.org/1766783003/diff/20001/media/blink/webmediapla... media/blink/webmediaplayer_impl.cc:370: : BackgroundPlaybackState::PLAY_ALLOWED_WHEN_SUSPENDED; On 2016/03/05 at 22:37:36, Mounir Lamouri wrote: > Should this include IsBackgroundSuspendResumeEnabled()? No, as indicated below. I've also renamed it to IsSuspendUponHiddenEnabled() for clarity. https://codereview.chromium.org/1766783003/diff/20001/media/blink/webmediapla... File media/blink/webmediaplayer_impl.h (right): https://codereview.chromium.org/1766783003/diff/20001/media/blink/webmediapla... media/blink/webmediaplayer_impl.h:172: void OnSuspend(bool must_suspend) override; On 2016/03/07 at 23:27:51, sandersd wrote: > I keep getting tripped up on this bool, so +1 for an enum here. I kept going around on this and can't find names I liked. So instead I've kept the bool but renamed this to OnSuspendRequested(). https://codereview.chromium.org/1766783003/diff/20001/media/blink/webmediapla... media/blink/webmediaplayer_impl.h:283: // Indicates if Resume() or Suspend() are allowed to be called respectively. On 2016/03/07 at 23:27:51, sandersd wrote: > I'm not clear on this comment given that there is only one method. > > Perhaps better would be "Indicates if automatic resumption is allowed in the current state", or something similar that indicates when this method is called. (Without that the name sounds like it should be const.) Done. https://codereview.chromium.org/1766783003/diff/20001/media/blink/webmediapla... media/blink/webmediaplayer_impl.h:431: PLAY_FORBIDDEN_NEVER_CALLED_IN_FOREGROUND, On 2016/03/07 at 19:23:07, DaleCurtis wrote: > On 2016/03/07 at 19:13:58, xhwang wrote: > > Can you provide more comments around these states? It's not super obvious to me, especially the "FORBIDDEN" part here. > > Yeah my names are bad :( I was trying to capture "playback has never been started in the foreground before, so don't let autoplay / js actually start playback in the background." This is similar to what we do where we never load the media until the page is in the foreground unless playback has ever started in that tab before. > > The others are essentially "don't play video in the background" and "play whatever you want in response to autoplay / js starting playback." After some more thought, I don't need these values. Removed.
This is much better, thanks! lgtm https://codereview.chromium.org/1766783003/diff/40001/content/renderer/media/... File content/renderer/media/android/webmediaplayer_android.cc (right): https://codereview.chromium.org/1766783003/diff/40001/content/renderer/media/... content/renderer/media/android/webmediaplayer_android.cc:1527: // players are allowed to continue playing in the background unless indicated "background" isn't implicitly the correct word here anymore.
Looking much better! I have a few API suggestions, and a discussion on potential cleanup opportunity around |delegate_|. https://chromiumcodereview.appspot.com/1766783003/diff/40001/content/renderer... File content/renderer/media/renderer_webmediaplayer_delegate.cc (right): https://chromiumcodereview.appspot.com/1766783003/diff/40001/content/renderer... content/renderer/media/renderer_webmediaplayer_delegate.cc:156: for (auto& kv : idle_delegate_map_) { Not from this CL... but what does "kv" stand for? Maybe a better name for people not familiar with this code, e.g. me? (after 15 seconds) Okay, I got it. It's key-value... Still, better name would be nicer. https://chromiumcodereview.appspot.com/1766783003/diff/40001/media/blink/webm... File media/blink/webmediaplayer_delegate.h (right): https://chromiumcodereview.appspot.com/1766783003/diff/40001/media/blink/webm... media/blink/webmediaplayer_delegate.h:29: virtual void OnSuspendRequested(bool must_suspend) = 0; One question for API. Is it legit that a WMP implementation wants to force releasing resources upon hidden? If so, it seems calling OnHidden() is not enough. But calling OnSuspendRequested(true) only seems wrong as well because we may miss something in OnHidden(). Calling both doesn't look nice either. Can we say in this case we should always call OnHidden() and must not call OnSuspendRequested()? https://chromiumcodereview.appspot.com/1766783003/diff/40001/media/blink/webm... File media/blink/webmediaplayer_impl.cc (right): https://chromiumcodereview.appspot.com/1766783003/diff/40001/media/blink/webm... media/blink/webmediaplayer_impl.cc:1034: if (delegate_) One comment about the |delegate_|: We are storing the |delegate_| as a weak pointer and checking it everywhere we use it. This is probably not necessary. The delegate is kinda owned by RenderFrameImpl [1]. And HTMLMediaElement is an ActiveDOMObject. ActiveDOMObject::stop() is guaranteed to be called before a frame is detached, so it's impossible for WMPI to outlive the frame and dereference a destructed delegate. You can see a similar discussion at [2]. If that makes sense, we can do some clean up around the |delegate_|: 1. Pass and store |delegate_| as a raw pointer. 2. Remove all the if(delegate_) check in WMP implementations. 3. (optional) Drop the RenderFrameObserver base class for RendererWebMediaPlayerDelegate and store |media_player_delegate_| as a normal scoped_ptr member in RenderFrameImpl. [1] https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/r... [2] https://codereview.chromium.org/1464183002/#msg42 https://chromiumcodereview.appspot.com/1766783003/diff/40001/media/blink/webm... media/blink/webmediaplayer_impl.cc:1068: // player has now been suspended. The comment on the API says: // Requests a WebMediaPlayer instance to release all idle resources. If // |must_suspend| is false, the player may ignore the request. However, if // |must_suspend| is true, the player must stop playback, release all // resources, and finally call WebMediaPlayerDelegate::PlayerGone(). It's not clear whether we should call PlayerGone() when |must_suspend| is false. Then the comment on PlayerGone() says: // The specified player was destroyed or suspended... In this case, the player IS suspended, so it seems we should call PlayerGone() in all cases... I kinda feel we need some more clarification on the API.
https://chromiumcodereview.appspot.com/1766783003/diff/40001/content/renderer... File content/renderer/media/android/webmediaplayer_android.cc (right): https://chromiumcodereview.appspot.com/1766783003/diff/40001/content/renderer... content/renderer/media/android/webmediaplayer_android.cc:1527: // players are allowed to continue playing in the background unless indicated On 2016/03/10 at 19:37:00, sandersd wrote: > "background" isn't implicitly the correct word here anymore. Removed. https://chromiumcodereview.appspot.com/1766783003/diff/40001/content/renderer... File content/renderer/media/renderer_webmediaplayer_delegate.cc (right): https://chromiumcodereview.appspot.com/1766783003/diff/40001/content/renderer... content/renderer/media/renderer_webmediaplayer_delegate.cc:156: for (auto& kv : idle_delegate_map_) { On 2016/03/10 at 21:51:06, xhwang wrote: > Not from this CL... but what does "kv" stand for? Maybe a better name for people not familiar with this code, e.g. me? > > (after 15 seconds) > > Okay, I got it. It's key-value... Still, better name would be nicer. Removed, though it's starting to become more common: https://code.google.com/p/chromium/codesearch#search/&q=%22%20kv%20:%20%22&sq... https://chromiumcodereview.appspot.com/1766783003/diff/40001/media/blink/webm... File media/blink/webmediaplayer_delegate.h (right): https://chromiumcodereview.appspot.com/1766783003/diff/40001/media/blink/webm... media/blink/webmediaplayer_delegate.h:29: virtual void OnSuspendRequested(bool must_suspend) = 0; On 2016/03/10 at 21:51:06, xhwang wrote: > One question for API. Is it legit that a WMP implementation wants to force releasing resources upon hidden? If so, it seems calling OnHidden() is not enough. But calling OnSuspendRequested(true) only seems wrong as well because we may miss something in OnHidden(). Calling both doesn't look nice either. > > Can we say in this case we should always call OnHidden() and must not call OnSuspendRequested()? What case are you talking about? It seems your proposing a hypothetical and assuming it happens in the code today? To be clear, there is no WMP implementation that wants to release ALL resource types when hidden. I feel that is a detail of the WMP implementation too. Further in that case it's actually sufficient to call OnSuspendRequested(true) so I'm not sure exactly what you're asking here :) https://chromiumcodereview.appspot.com/1766783003/diff/40001/media/blink/webm... File media/blink/webmediaplayer_impl.cc (right): https://chromiumcodereview.appspot.com/1766783003/diff/40001/media/blink/webm... media/blink/webmediaplayer_impl.cc:1034: if (delegate_) On 2016/03/10 at 21:51:06, xhwang wrote: > One comment about the |delegate_|: > > We are storing the |delegate_| as a weak pointer and checking it everywhere we use it. This is probably not necessary. > > The delegate is kinda owned by RenderFrameImpl [1]. And HTMLMediaElement is an ActiveDOMObject. ActiveDOMObject::stop() is guaranteed to be called before a frame is detached, so it's impossible for WMPI to outlive the frame and dereference a destructed delegate. You can see a similar discussion at [2]. > > If that makes sense, we can do some clean up around the |delegate_|: > 1. Pass and store |delegate_| as a raw pointer. > 2. Remove all the if(delegate_) check in WMP implementations. > 3. (optional) Drop the RenderFrameObserver base class for RendererWebMediaPlayerDelegate and store |media_player_delegate_| as a normal scoped_ptr member in RenderFrameImpl. > > [1] https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/r... > [2] https://codereview.chromium.org/1464183002/#msg42 Yay! Thanks for this, I was hoping this was possible, but hadn't dug into it deep enough yet. I'll do that in a follow up CL. Filed http://crbug.com/593932 https://chromiumcodereview.appspot.com/1766783003/diff/40001/media/blink/webm... media/blink/webmediaplayer_impl.cc:1068: // player has now been suspended. On 2016/03/10 at 21:51:06, xhwang wrote: > The comment on the API says: > > // Requests a WebMediaPlayer instance to release all idle resources. If > // |must_suspend| is false, the player may ignore the request. However, if > // |must_suspend| is true, the player must stop playback, release all > // resources, and finally call WebMediaPlayerDelegate::PlayerGone(). > > It's not clear whether we should call PlayerGone() when |must_suspend| is false. > > Then the comment on PlayerGone() says: > > // The specified player was destroyed or suspended... > > In this case, the player IS suspended, so it seems we should call PlayerGone() in all cases... > > I kinda feel we need some more clarification on the API. I thought it was pretty clear, but I've filled out the comments a bit more :)
LGTM with bikeshedding on API :) https://chromiumcodereview.appspot.com/1766783003/diff/40001/media/blink/webm... File media/blink/webmediaplayer_delegate.h (right): https://chromiumcodereview.appspot.com/1766783003/diff/40001/media/blink/webm... media/blink/webmediaplayer_delegate.h:29: virtual void OnSuspendRequested(bool must_suspend) = 0; On 2016/03/10 23:11:36, DaleCurtis wrote: > On 2016/03/10 at 21:51:06, xhwang wrote: > > One question for API. Is it legit that a WMP implementation wants to force > releasing resources upon hidden? If so, it seems calling OnHidden() is not > enough. But calling OnSuspendRequested(true) only seems wrong as well because we > may miss something in OnHidden(). Calling both doesn't look nice either. > > > > Can we say in this case we should always call OnHidden() and must not call > OnSuspendRequested()? > > What case are you talking about? It seems your proposing a hypothetical and > assuming it happens in the code today? To be clear, there is no WMP > implementation that wants to release ALL resource types when hidden. I feel that > is a detail of the WMP implementation too. Further in that case it's actually > sufficient to call OnSuspendRequested(true) so I'm not sure exactly what you're > asking here :) Agreed that this is a hypothetical case. As you said, in that case it should be sufficient to call OnSuspendRequested(true). But this isn't clear in the API. People can easily assume that whenever a player is hidden they have to call OnHidden().
mlamouri: Did you want to weigh in? https://chromiumcodereview.appspot.com/1766783003/diff/40001/media/blink/webm... File media/blink/webmediaplayer_delegate.h (right): https://chromiumcodereview.appspot.com/1766783003/diff/40001/media/blink/webm... media/blink/webmediaplayer_delegate.h:29: virtual void OnSuspendRequested(bool must_suspend) = 0; On 2016/03/11 at 00:09:19, xhwang wrote: > On 2016/03/10 23:11:36, DaleCurtis wrote: > > On 2016/03/10 at 21:51:06, xhwang wrote: > > > One question for API. Is it legit that a WMP implementation wants to force > > releasing resources upon hidden? If so, it seems calling OnHidden() is not > > enough. But calling OnSuspendRequested(true) only seems wrong as well because we > > may miss something in OnHidden(). Calling both doesn't look nice either. > > > > > > Can we say in this case we should always call OnHidden() and must not call > > OnSuspendRequested()? > > > > What case are you talking about? It seems your proposing a hypothetical and > > assuming it happens in the code today? To be clear, there is no WMP > > implementation that wants to release ALL resource types when hidden. I feel that > > is a detail of the WMP implementation too. Further in that case it's actually > > sufficient to call OnSuspendRequested(true) so I'm not sure exactly what you're > > asking here :) > > Agreed that this is a hypothetical case. > > As you said, in that case it should be sufficient to call OnSuspendRequested(true). But this isn't clear in the API. People can easily assume that whenever a player is hidden they have to call OnHidden(). I think that's a correct assumption though :) Hidden will always be delivered when the render frame is backgrounded. The alternative is going back to a single mega-method like OnSuspend(REASON_HIDDEN_BG_AUDIO_OPTIONAL | REASON_FORCED_SUSPEND_MANDATORY | REASON_IDLE_OPTIONAL). Which I don't like as much as the separate methods we have now. Not the least of which because my names stink :)
The CQ bit was checked by dalecurtis@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/1766783003/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1766783003/60001
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 mlamouri@chromium.org
lgtm
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/1766783003/#ps60001 (title: "Fixup API comments.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1766783003/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1766783003/60001
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Expand suspension of idle media players to all platforms. This CL enables suspension of idle players on the desktop with the same timers as used on Android: 15 seconds idle (paused, ended, never started), polled every 5 seconds. It also replaces the overloaded OnHidden() method with a more appropriate OnSuspendRequested() method for handling both forced and idle suspensions; OnHidden() continues to handle suspensions upon backgrounding on Android. By separating suspends into a new method we can always suspend idle players without worrying about if they have a media session or not. If they do and the user interacts with the session, we'll resume the player dynamically. BUG=590099 TEST=updated unittest, manual testing. ========== to ========== Expand suspension of idle media players to all platforms. This CL enables suspension of idle players on the desktop with the same timers as used on Android: 15 seconds idle (paused, ended, never started), polled every 5 seconds. It also replaces the overloaded OnHidden() method with a more appropriate OnSuspendRequested() method for handling both forced and idle suspensions; OnHidden() continues to handle suspensions upon backgrounding on Android. By separating suspends into a new method we can always suspend idle players without worrying about if they have a media session or not. If they do and the user interacts with the session, we'll resume the player dynamically. BUG=590099 TEST=updated unittest, manual testing. Committed: https://crrev.com/0431cbf89bc6bc50c9ad2ce0cc4ac5117e38b1d9 Cr-Commit-Position: refs/heads/master@{#380819} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/0431cbf89bc6bc50c9ad2ce0cc4ac5117e38b1d9 Cr-Commit-Position: refs/heads/master@{#380819} |
