Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2696663002:
Media Remoting: Don't auto suspend the media pipeline. (Closed)
|
Created:
3 years, 10 months ago by xjz Modified:
3 years, 10 months ago CC:
chromium-reviews, mlamouri+watch-content_chromium.org, creis+watch_chromium.org, nasko+codewatch_chromium.org, jam, feature-media-reviews_chromium.org, darin-cc_chromium.org, apacible+watch_chromium.org, xjz+watch_chromium.org, miu+watch_chromium.org, erickung+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMedia Remoting: Don't auto suspend the media pipeline.
Disable the optimization to auto suspend media pipeline for background video or
idle pipeline in media remoting to prevent the end of the session while still
in remoting.
BUG=691673
Review-Url: https://codereview.chromium.org/2696663002
Cr-Commit-Position: refs/heads/master@{#450805}
Committed: https://chromium.googlesource.com/chromium/src/+/4e5d4bf3631742de169cea750db2d19d56d474a1
Patch Set 1 #Patch Set 2 : Disable other auto suspend for optimization. #
Total comments: 10
Patch Set 3 : Addressed comments. #
Total comments: 6
Patch Set 4 : Addressed comments. #Patch Set 5 : Fix tests. #
Messages
Total messages: 38 (25 generated)
The CQ bit was checked by xjz@chromium.org to run a CQ dry run
Description was changed from ========== Media Remoting: Don't suspend the media pipeline on idle. Media pipeline should not be suspended on idle for media remotign to prevent the switching back to mirroring. BUG=691673 ========== to ========== Media Remoting: Don't suspend the media pipeline on idle. Media pipeline should not be suspended on idle for media remotign to prevent the switching back to mirroring. BUG=691673 ==========
xjz@chromium.org changed reviewers: + miu@chromium.org, sandersd@chromium.org
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
sandersd@: This CL adds a logic to disable pipeline suspend on idle for media remoting. Also replaces the three callbacks with MediaObserverClient interface. PTAL media/base/* and media/blink/*. Thanks! miu@: PTAL all. :) Thanks!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2017/02/13 19:42:09, xjz wrote: > sandersd@: This CL adds a logic to disable pipeline suspend on idle for media > remoting. Also replaces the three callbacks with MediaObserverClient interface. > PTAL media/base/* and media/blink/*. Thanks! > > miu@: PTAL all. :) Thanks! Can you help me understand the intended behavior? I suspect there is a simpler change that will support all the different use cases properly. In particular: - In what cases do you want to suspend remoted media? I'm assuming the answer is 'only when backgrounded on mobile', but maybe it's 'never'. - What are the interactions with other optimizations? (Background video track disable, background pause.) How do we control those in this case?
On 2017/02/13 22:31:42, sandersd wrote: > On 2017/02/13 19:42:09, xjz wrote: > > sandersd@: This CL adds a logic to disable pipeline suspend on idle for media > > remoting. Also replaces the three callbacks with MediaObserverClient > interface. > > PTAL media/base/* and media/blink/*. Thanks! > > > > miu@: PTAL all. :) Thanks! > > Can you help me understand the intended behavior? I suspect there is a simpler > change that will support all the different use cases properly. > > In particular: > - In what cases do you want to suspend remoted media? I'm assuming the answer > is 'only when backgrounded on mobile', but maybe it's 'never'. Never on desktop, and never on mobile. Currently, Media Remoting isn't launching for mobile; but Anton will likely work with us to do that in the next few quarters. So, it would make sense (feature-wise) to not suspend in any case. > - What are the interactions with other optimizations? (Background video track > disable, background pause.) How do we control those in this case? Similar answer. IIRC, Anton has made changes to explicitly not engage video track disable during remoting (in his recent changes).
The CQ bit was checked by xjz@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Media Remoting: Don't suspend the media pipeline on idle. Media pipeline should not be suspended on idle for media remotign to prevent the switching back to mirroring. BUG=691673 ========== to ========== Media Remoting: Don't auto suspend the media pipeline on idle. Disable the optimization to auto suspend media pipeline for background video or idle pipeline in media remoting to prevent the end of the session while still in remoting. BUG=691673 ==========
Description was changed from ========== Media Remoting: Don't auto suspend the media pipeline on idle. Disable the optimization to auto suspend media pipeline for background video or idle pipeline in media remoting to prevent the end of the session while still in remoting. BUG=691673 ========== to ========== Media Remoting: Don't auto suspend the media pipeline. Disable the optimization to auto suspend media pipeline for background video or idle pipeline in media remoting to prevent the end of the session while still in remoting. BUG=691673 ==========
Per discussion, disable all optimization that could suspend the pipeline for media remoting. PTAL. Thanks!
Comments on PS2: https://codereview.chromium.org/2696663002/diff/20001/media/base/media_observ... File media/base/media_observer.h (right): https://codereview.chromium.org/2696663002/diff/20001/media/base/media_observ... media/base/media_observer.h:20: virtual void SwitchRenderer() = 0; Observation: It seems both SwitchRenderer() and DisablePipelineAutoSuspend() are always called together. Suggestion: How about just have one SwitchRenderer() method that takes a "bool disable_auto_suspend" argument, rather than these two separate methods? This would simplify code in both RendererController and WMPI. https://codereview.chromium.org/2696663002/diff/20001/media/blink/webmediapla... File media/blink/webmediaplayer_impl.cc (right): https://codereview.chromium.org/2696663002/diff/20001/media/blink/webmediapla... media/blink/webmediaplayer_impl.cc:1844: bool is_streaming = IsStreaming() || disable_pipeline_auto_suspend_; This |is_streaming| variable should probably be renamed to something like |can_auto_suspend|: bool can_auto_suspend = !disable_pipeline_auto_suspend_ && !IsStreaming(); (and, of course, that means inverting the true/false everywhere downsteream). Also, s/is_streaming/can_auto_suspend/ in the args of UpdatePlayState_ComputePlayState() and inverting the meaning there too. https://codereview.chromium.org/2696663002/diff/20001/media/remoting/renderer... File media/remoting/renderer_controller.cc (right): https://codereview.chromium.org/2696663002/diff/20001/media/remoting/renderer... media/remoting/renderer_controller.cc:37: client_->DisablePipelineAutoSuspend(true); The disable should happen before the SwitchRenderer() call here, to make sure the "optimization" can't engage immediately when renderers are switched. https://codereview.chromium.org/2696663002/diff/20001/media/remoting/renderer... media/remoting/renderer_controller.cc:476: nit: Add: DCHECK(client) and DCHECK(!client_), since the intention here is to set once and never unset. https://codereview.chromium.org/2696663002/diff/20001/media/remoting/renderer... media/remoting/renderer_controller.cc:478: if (client_) nit: And then, no need to null-check here because |client_| was set to a non-null value one line above.
Addressed miu's comments. PTAL again. Thanks! https://codereview.chromium.org/2696663002/diff/20001/media/base/media_observ... File media/base/media_observer.h (right): https://codereview.chromium.org/2696663002/diff/20001/media/base/media_observ... media/base/media_observer.h:20: virtual void SwitchRenderer() = 0; On 2017/02/13 23:36:08, miu wrote: > Observation: It seems both SwitchRenderer() and DisablePipelineAutoSuspend() are > always called together. > > Suggestion: How about just have one SwitchRenderer() method that takes a "bool > disable_auto_suspend" argument, rather than these two separate methods? This > would simplify code in both RendererController and WMPI. Done. I did observe this too. But I was not sure whether we should combine these two in the interface design, since these two are not necessarily always called together. After an offline discussion, I agree with this suggestion, since they are now always called together and can both control the pipeline. https://codereview.chromium.org/2696663002/diff/20001/media/blink/webmediapla... File media/blink/webmediaplayer_impl.cc (right): https://codereview.chromium.org/2696663002/diff/20001/media/blink/webmediapla... media/blink/webmediaplayer_impl.cc:1844: bool is_streaming = IsStreaming() || disable_pipeline_auto_suspend_; On 2017/02/13 23:36:08, miu wrote: > This |is_streaming| variable should probably be renamed to something like > |can_auto_suspend|: > > bool can_auto_suspend = !disable_pipeline_auto_suspend_ && !IsStreaming(); > > (and, of course, that means inverting the true/false everywhere downsteream). > > Also, s/is_streaming/can_auto_suspend/ in the args of > UpdatePlayState_ComputePlayState() and inverting the meaning there too. Done. https://codereview.chromium.org/2696663002/diff/20001/media/remoting/renderer... File media/remoting/renderer_controller.cc (right): https://codereview.chromium.org/2696663002/diff/20001/media/remoting/renderer... media/remoting/renderer_controller.cc:37: client_->DisablePipelineAutoSuspend(true); On 2017/02/13 23:36:08, miu wrote: > The disable should happen before the SwitchRenderer() call here, to make sure > the "optimization" can't engage immediately when renderers are switched. Done. https://codereview.chromium.org/2696663002/diff/20001/media/remoting/renderer... media/remoting/renderer_controller.cc:476: On 2017/02/13 23:36:08, miu wrote: > nit: Add: DCHECK(client) and DCHECK(!client_), since the intention here is to > set once and never unset. Done. https://codereview.chromium.org/2696663002/diff/20001/media/remoting/renderer... media/remoting/renderer_controller.cc:478: if (client_) On 2017/02/13 23:36:08, miu wrote: > nit: And then, no need to null-check here because |client_| was set to a > non-null value one line above. Done.
//media/blink lgtm. I'm not sure about the name (I would prefer |is_suspend_allowed| to |can_auto_suspend|), but I see how your choice is potentially less confusing. Overall improves on the |is_streaming| logic too!
xjz@chromium.org changed reviewers: + nasko@chromium.org
nasko@: PTAL change on content/renderer/render_frame_impl.cc. Thanks!
The CQ bit was checked by xjz@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
PS3 lgtm % nits: https://codereview.chromium.org/2696663002/diff/40001/media/base/media_observ... File media/base/media_observer.h (right): https://codereview.chromium.org/2696663002/diff/40001/media/base/media_observ... media/base/media_observer.h:20: // the optimization that might suspend media pipeline. nit: s/the optimization/any optimizations/ https://codereview.chromium.org/2696663002/diff/40001/media/base/media_observ... media/base/media_observer.h:20: // the optimization that might suspend media pipeline. nit: s/suspend media/suspend the media/ https://codereview.chromium.org/2696663002/diff/40001/media/remoting/renderer... File media/remoting/renderer_controller.cc (right): https://codereview.chromium.org/2696663002/diff/40001/media/remoting/renderer... media/remoting/renderer_controller.cc:474: DCHECK(client); nit: After this, I'd recommend: DCHECK(!client_) That way you can ensure |client_| is only set once, which seems to be an assumption all the code that uses it has. :)
The CQ bit was checked by xjz@chromium.org to run a CQ dry run
Thanks for reviewing. Addressed comments. https://codereview.chromium.org/2696663002/diff/40001/media/base/media_observ... File media/base/media_observer.h (right): https://codereview.chromium.org/2696663002/diff/40001/media/base/media_observ... media/base/media_observer.h:20: // the optimization that might suspend media pipeline. On 2017/02/15 02:00:43, miu wrote: > nit: s/suspend media/suspend the media/ Done. https://codereview.chromium.org/2696663002/diff/40001/media/base/media_observ... media/base/media_observer.h:20: // the optimization that might suspend media pipeline. On 2017/02/15 02:00:43, miu wrote: > nit: s/the optimization/any optimizations/ Done. https://codereview.chromium.org/2696663002/diff/40001/media/remoting/renderer... File media/remoting/renderer_controller.cc (right): https://codereview.chromium.org/2696663002/diff/40001/media/remoting/renderer... media/remoting/renderer_controller.cc:474: DCHECK(client); On 2017/02/15 02:00:43, miu wrote: > nit: After this, I'd recommend: DCHECK(!client_) That way you can ensure > |client_| is only set once, which seems to be an assumption all the code that > uses it has. :) Done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
content/renderer/render_frame_impl.cc rubberstamp LGTM.
The CQ bit was checked by xjz@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by xjz@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from miu@chromium.org, sandersd@chromium.org, nasko@chromium.org Link to the patchset: https://codereview.chromium.org/2696663002/#ps80001 (title: "Fix tests.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 80001, "attempt_start_ts": 1487188274546220,
"parent_rev": "252e09263e39d4e06b2a9dc094ea30ca140e0a4d", "commit_rev":
"4e5d4bf3631742de169cea750db2d19d56d474a1"}
Message was sent while issue was closed.
Description was changed from ========== Media Remoting: Don't auto suspend the media pipeline. Disable the optimization to auto suspend media pipeline for background video or idle pipeline in media remoting to prevent the end of the session while still in remoting. BUG=691673 ========== to ========== Media Remoting: Don't auto suspend the media pipeline. Disable the optimization to auto suspend media pipeline for background video or idle pipeline in media remoting to prevent the end of the session while still in remoting. BUG=691673 Review-Url: https://codereview.chromium.org/2696663002 Cr-Commit-Position: refs/heads/master@{#450805} Committed: https://chromium.googlesource.com/chromium/src/+/4e5d4bf3631742de169cea750db2... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/chromium/src/+/4e5d4bf3631742de169cea750db2... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
