Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2618883002:
[Media, Video] Enable the video track for a new renderer. (Closed)
|
Created:
3 years, 11 months ago by whywhat Modified:
3 years, 11 months ago Reviewers:
sandersd (OOO until July 31) CC:
apacible+watch_chromium.org, chromium-reviews, erickung+watch_chromium.org, feature-media-reviews_chromium.org, miu+watch_chromium.org, posciak+watch_chromium.org, xjz+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[Media, Video] Enable the video track for a new renderer.
Video track needs to be reenabled before the new renderer is
created so that the callbacks attach properly. For that, add
two new callbacks to PipelineController for when the pipeline
is about to resume and is resumed.
This adds a potential problem when the video is shown or hidden
in between the two events. So don't change the video track if
the pipeline is resuming or seeking but check for the necessary
conditions in the resumed callback.
BUG=678374
TEST=media_[blink_]unittests + manual testing
Review-Url: https://codereview.chromium.org/2618883002
Cr-Commit-Position: refs/heads/master@{#443404}
Committed: https://chromium.googlesource.com/chromium/src/+/2135a640a1547f44b62828bac84dc713d39652c6
Patch Set 1 #Patch Set 2 : Reselect the video track before suspending #Patch Set 3 : Added ResumedCB to PipelineController #
Total comments: 4
Patch Set 4 : Immediate resume_cb_, fixed unittests #
Total comments: 4
Patch Set 5 : Added OnBeforeResume callback #
Total comments: 14
Patch Set 6 : Addressed comments #Patch Set 7 : Added unittest for ShouldDisableVideoWhenHidden #Patch Set 8 : Remove OnHidden() from start #Patch Set 9 : Fixed debug tests #
Dependent Patchsets: Messages
Total messages: 52 (28 generated)
Reselect the video track before suspending
avayvod@chromium.org changed reviewers: + sandersd@chromium.org
Forgot to publish this yesterday Seems like we agreed offline on having a separate callback for post-resume and keep the TODOs to remove this once we have a better way to enforce dependency between the enabled tracks and the renderer.
On 2017/01/06 22:08:18, whywhat wrote: > Forgot to publish this yesterday > > Seems like we agreed offline on having a separate callback for post-resume and > keep the TODOs to remove this once we have a better way to enforce dependency > between the enabled tracks and the renderer. Exactly. Add some new PipelineController callbacks to avoid overloading the seek one.
Added ResumedCB to PipelineController
The CQ bit was checked by avayvod@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: 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_...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
https://codereview.chromium.org/2618883002/diff/40001/media/blink/webmediapla... File media/blink/webmediaplayer_impl.cc (right): https://codereview.chromium.org/2618883002/diff/40001/media/blink/webmediapla... media/blink/webmediaplayer_impl.cc:1069: if (seeking_) { Shouldn't be needed anymore? https://codereview.chromium.org/2618883002/diff/40001/media/filters/pipeline_... File media/filters/pipeline_controller.cc (right): https://codereview.chromium.org/2618883002/diff/40001/media/filters/pipeline_... media/filters/pipeline_controller.cc:240: resumed_cb_.Run(); This won't be called if |pending_seeked_cb_| is true. The cleanest alternative I can think of is to make it an immediate callback instead of a stable state callback. I'm not sure which semantics are better for you, but moving this to OnPipelineStatus() means we won't have to worry about the reentrancy of |seeked_cb_|. It's probably okay to just move it above the seek callback as well. We can require that the resume CB is non-reentrant as a matter of API documentation.
Immediate resume_cb_, fixed unittests
https://codereview.chromium.org/2618883002/diff/40001/media/blink/webmediapla... File media/blink/webmediaplayer_impl.cc (right): https://codereview.chromium.org/2618883002/diff/40001/media/blink/webmediapla... media/blink/webmediaplayer_impl.cc:1069: if (seeking_) { On 2017/01/07 at 00:56:17, sandersd wrote: > Shouldn't be needed anymore? Done. https://codereview.chromium.org/2618883002/diff/40001/media/filters/pipeline_... File media/filters/pipeline_controller.cc (right): https://codereview.chromium.org/2618883002/diff/40001/media/filters/pipeline_... media/filters/pipeline_controller.cc:240: resumed_cb_.Run(); On 2017/01/07 at 00:56:17, sandersd wrote: > This won't be called if |pending_seeked_cb_| is true. > > The cleanest alternative I can think of is to make it an immediate callback instead of a stable state callback. I'm not sure which semantics are better for you, but moving this to OnPipelineStatus() means we won't have to worry about the reentrancy of |seeked_cb_|. > > It's probably okay to just move it above the seek callback as well. We can require that the resume CB is non-reentrant as a matter of API documentation. Done. Would checking for state_ to be RESUMING and state be PLAYING in OnPipelineStatus be a good enough condition so I could eliminate the pending_resumed_cb_ var? As long as the RendererImpl calls InitializeVideoRenderer by this moment (and I think it does), that should be enough for the bug I'm fixing.
The CQ bit was checked by avayvod@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.
This is looking quite nice, it's barely a hack at this point. https://codereview.chromium.org/2618883002/diff/60001/media/blink/webmediapla... File media/blink/webmediaplayer_impl.cc (right): https://codereview.chromium.org/2618883002/diff/60001/media/blink/webmediapla... media/blink/webmediaplayer_impl.cc:483: if (video_track_disabled_) { I think this would be better in an OnBeforeResume() callback, it's not trivially obvious that doing it in pause() is always enough. https://codereview.chromium.org/2618883002/diff/60001/media/blink/webmediapla... media/blink/webmediaplayer_impl.cc:1394: delegate_state_ == DelegateState::PLAYING) { Is |delegate_state_| the best thing to check for this? Do we even need this with OnBeforeResume()?
https://codereview.chromium.org/2618883002/diff/60001/media/blink/webmediapla... File media/blink/webmediaplayer_impl.cc (right): https://codereview.chromium.org/2618883002/diff/60001/media/blink/webmediapla... media/blink/webmediaplayer_impl.cc:483: if (video_track_disabled_) { On 2017/01/09 at 20:12:49, sandersd wrote: > I think this would be better in an OnBeforeResume() callback, it's not trivially obvious that doing it in pause() is always enough. Sure. I guess the fix would be reduced to enabling the video track back in OnBeforeResume (OnResuming? OnResumeRequested?) and disabling in OnResumed() https://codereview.chromium.org/2618883002/diff/60001/media/blink/webmediapla... media/blink/webmediaplayer_impl.cc:1394: delegate_state_ == DelegateState::PLAYING) { On 2017/01/09 at 20:12:49, sandersd wrote: > Is |delegate_state_| the best thing to check for this? Do we even need this with OnBeforeResume()? Will try. I saw this member is gone in your refactor :)
Added OnBeforeResume callback
Description was changed from
==========
[Video] Disable video track only when/after playing
If we disable background video track only when the renderer is alive -
the track will update the renderer when enabled upon foregrounding. Also
need to enable the video track back before the renderer is destroyed.
BUG=678374
==========
to
==========
[Media, Video] Enable the video track for a new renderer.
Video track needs to be reenabled before the new renderer is
created so that the callbacks attach properly. For that, add
two new callbacks to PipelineController for when the pipeline
is about to resume and is resumed.
This adds a potential problem when the video is shown or hidden
in between the two events. So don't change the video track if
the pipeline is resuming or seeking but check for the necessary
conditions in the resumed callback.
Also apply the video optimizations when the pipeline seeks - that
is when the playback starts since the initial state always assumes
being shown which is not true on desktop.
BUG=678374
TEST=media_unittests + manual testing
==========
The CQ bit was checked by avayvod@chromium.org to run a CQ dry run
PTAL
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.
https://codereview.chromium.org/2618883002/diff/80001/media/blink/webmediapla... File media/blink/webmediaplayer_impl.cc (right): https://codereview.chromium.org/2618883002/diff/80001/media/blink/webmediapla... media/blink/webmediaplayer_impl.cc:1092: OnHidden(); OnHidden() does a bit more than what is requested, please factor out the relevant part. https://codereview.chromium.org/2618883002/diff/80001/media/blink/webmediapla... media/blink/webmediaplayer_impl.cc:1134: return; Nit: This may be a case where early return is more confusing than an else branch. (I'd go further and say this should be part of UpdatePlayState() but since the plan is to rip it all out I won't recommend that.) https://codereview.chromium.org/2618883002/diff/80001/media/blink/webmediapla... media/blink/webmediaplayer_impl.cc:1382: if (!paused_when_hidden_ && ShouldPauseWhenHidden()) { As a matter of style, I prefer idempotency wherever possible. Since in this case running this code twice will actually result in disabling the video track, it would be better to make the |!paused_when_hidden_| condition one level deeper. Relatedly, ShouldDisableVideoWhenHidden() should probably return false if ShouldPauseWhenHidden() is true. https://codereview.chromium.org/2618883002/diff/80001/media/blink/webmediapla... media/blink/webmediaplayer_impl.cc:1409: if (paused_when_hidden_) { By the same reasoning, this should go right before UpdatePlayState() so that all the other state changes are idempotent. (And |must_suspend_| is important to update in any case.) https://codereview.chromium.org/2618883002/diff/80001/media/blink/webmediapla... media/blink/webmediaplayer_impl.cc:2113: return hasVideo() && !hasAudio(); Probably worth a comment somewhere: these conditions imply that ShouldPauseWHenHidden() and ShouldDisableVideoWhenHidden() return false before Start() is called. They don't quite imply that they return false until Start() has finished, but |seeking_| does. https://codereview.chromium.org/2618883002/diff/80001/media/blink/webmediapla... File media/blink/webmediaplayer_impl.h (right): https://codereview.chromium.org/2618883002/diff/80001/media/blink/webmediapla... media/blink/webmediaplayer_impl.h:392: // https://crbug.com/678374. Somewhat confusing, in particular because resume is listed twice. It's also not true that (as I understand it) to add the track back for a seek; only for start (but please comment that |seeking_| is used as a proxy for starting). It's probably true that we don't want to change the active tracks during a seek, so I won't recommend changing to testing for 'starting' explicitly. (But if you do want it to be more explicit, you can rename and set |is_pipeline_resuming_| during startup.)
Addressed comments
The CQ bit was checked by avayvod@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.
https://codereview.chromium.org/2618883002/diff/80001/media/blink/webmediapla... File media/blink/webmediaplayer_impl.cc (right): https://codereview.chromium.org/2618883002/diff/80001/media/blink/webmediapla... media/blink/webmediaplayer_impl.cc:1092: OnHidden(); On 2017/01/10 at 21:34:37, sandersd wrote: > OnHidden() does a bit more than what is requested, please factor out the relevant part. I think that's the point of it, no? We initialize the player as if it's shown despite it's being hidden so when it starts and we're hidden we want all of it - update play state, schedule idle pause and so on. I think it shouldn't change the existing behavior with the flag being off since the media is playing at this point. https://codereview.chromium.org/2618883002/diff/80001/media/blink/webmediapla... media/blink/webmediaplayer_impl.cc:1134: return; On 2017/01/10 at 21:34:37, sandersd wrote: > Nit: This may be a case where early return is more confusing than an else branch. > > (I'd go further and say this should be part of UpdatePlayState() but since the plan is to rip it all out I won't recommend that.) Done. For some reason, I expect lint to complain though. https://codereview.chromium.org/2618883002/diff/80001/media/blink/webmediapla... media/blink/webmediaplayer_impl.cc:1382: if (!paused_when_hidden_ && ShouldPauseWhenHidden()) { On 2017/01/10 at 21:34:37, sandersd wrote: > As a matter of style, I prefer idempotency wherever possible. Since in this case running this code twice will actually result in disabling the video track, it would be better to make the |!paused_when_hidden_| condition one level deeper. I think it won't because ShouldPauseWhenHidden and ShouldDisableWhenHidden are mutually exclusive via hasAudio(). Done though. > > Relatedly, ShouldDisableVideoWhenHidden() should probably return false if ShouldPauseWhenHidden() is true. That's already true. https://codereview.chromium.org/2618883002/diff/80001/media/blink/webmediapla... media/blink/webmediaplayer_impl.cc:1409: if (paused_when_hidden_) { On 2017/01/10 at 21:34:37, sandersd wrote: > By the same reasoning, this should go right before UpdatePlayState() so that all the other state changes are idempotent. (And |must_suspend_| is important to update in any case.) Moved the state update above the new logic. I don't want to check if the video "should be paused/disabled" here though since it may change since it has been paused/disabled and we must unpause/enable it anyway. (not sure if you meant that or not). We could have a hidden state enum instead of two bools to enforce exclusiveness of being either paused or without video track but not both. https://codereview.chromium.org/2618883002/diff/80001/media/blink/webmediapla... media/blink/webmediaplayer_impl.cc:2113: return hasVideo() && !hasAudio(); On 2017/01/10 at 21:34:37, sandersd wrote: > Probably worth a comment somewhere: these conditions imply that ShouldPauseWHenHidden() and ShouldDisableVideoWhenHidden() return false before Start() is called. Should I make an even stronger comment that they will be false until the demuxer is initialized and OnMetadata is called? Or is it the same thing essentially? > > They don't quite imply that they return false until Start() has finished, but |seeking_| does. |seeking_| implies that tracks won't be disabled in the methods below, but these methods can return true already if OnMetadata was called, right? Added comments in the header. Didn't find anything I could easily DCHECK. https://codereview.chromium.org/2618883002/diff/80001/media/blink/webmediapla... File media/blink/webmediaplayer_impl.h (right): https://codereview.chromium.org/2618883002/diff/80001/media/blink/webmediapla... media/blink/webmediaplayer_impl.h:392: // https://crbug.com/678374. On 2017/01/10 at 21:34:37, sandersd wrote: > Somewhat confusing, in particular because resume is listed twice. > > It's also not true that (as I understand it) to add the track back for a seek; only for start (but please comment that |seeking_| is used as a proxy for starting). > > It's probably true that we don't want to change the active tracks during a seek, so I won't recommend changing to testing for 'starting' explicitly. (But if you do want it to be more explicit, you can rename and set |is_pipeline_resuming_| during startup.) Rephrased the comments, hopefully they are less confusing now.
Added unittest for ShouldDisableVideoWhenHidden
lgtm https://codereview.chromium.org/2618883002/diff/80001/media/blink/webmediapla... File media/blink/webmediaplayer_impl.cc (right): https://codereview.chromium.org/2618883002/diff/80001/media/blink/webmediapla... media/blink/webmediaplayer_impl.cc:1092: OnHidden(); On 2017/01/11 19:41:30, whywhat wrote: > On 2017/01/10 at 21:34:37, sandersd wrote: > > OnHidden() does a bit more than what is requested, please factor out the > relevant part. > > I think that's the point of it, no? We initialize the player as if it's shown > despite it's being hidden so when it starts and we're hidden we want all of it - > update play state, schedule idle pause and so on. I think it shouldn't change > the existing behavior with the flag being off since the media is playing at this > point. The target state for this code is that OnHidden() calls UpdatePlayState() and does nothing else; I don't want to have to think about every possible transition that WMPI can go through. If there is something that is behaving incorrectly with regards to that, it is a bug. Looking at what's already in there: - WatchTimeReporter is constructed in the correct state. - Selected video track is this CL. - ScheduleIdlePauseTimer() is probably buggy. I'm not fully up to date on autoplay cases so I'm not sure what behavior we want, but this should move into UpdatePlayState() eventually. https://codereview.chromium.org/2618883002/diff/80001/media/blink/webmediapla... media/blink/webmediaplayer_impl.cc:2113: return hasVideo() && !hasAudio(); On 2017/01/11 19:41:30, whywhat wrote: > On 2017/01/10 at 21:34:37, sandersd wrote: > > Probably worth a comment somewhere: these conditions imply that > ShouldPauseWHenHidden() and ShouldDisableVideoWhenHidden() return false before > Start() is called. > > Should I make an even stronger comment that they will be false until the demuxer > is initialized and OnMetadata is called? Or is it the same thing essentially? > > > > > They don't quite imply that they return false until Start() has finished, but > |seeking_| does. > > |seeking_| implies that tracks won't be disabled in the methods below, but these > methods can return true already if OnMetadata was called, right? > > Added comments in the header. > Didn't find anything I could easily DCHECK. OnMetadata is called after demuxer initialization, seeking completes after decoder initialization. Exactly what to document depends on what is relevant to track changes: I assumed it was decoder, but that may be stricter than necessary.
Remove OnHidden() from start
Description was changed from
==========
[Media, Video] Enable the video track for a new renderer.
Video track needs to be reenabled before the new renderer is
created so that the callbacks attach properly. For that, add
two new callbacks to PipelineController for when the pipeline
is about to resume and is resumed.
This adds a potential problem when the video is shown or hidden
in between the two events. So don't change the video track if
the pipeline is resuming or seeking but check for the necessary
conditions in the resumed callback.
Also apply the video optimizations when the pipeline seeks - that
is when the playback starts since the initial state always assumes
being shown which is not true on desktop.
BUG=678374
TEST=media_unittests + manual testing
==========
to
==========
[Media, Video] Enable the video track for a new renderer.
Video track needs to be reenabled before the new renderer is
created so that the callbacks attach properly. For that, add
two new callbacks to PipelineController for when the pipeline
is about to resume and is resumed.
This adds a potential problem when the video is shown or hidden
in between the two events. So don't change the video track if
the pipeline is resuming or seeking but check for the necessary
conditions in the resumed callback.
BUG=678374
TEST=media_[blink_]unittests + manual testing
==========
The CQ bit was checked by avayvod@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/2618883002/#ps140001 (title: "Remove OnHidden() from start")
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
Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
Fixed debug tests
The CQ bit was checked by avayvod@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/2618883002/#ps160001 (title: "Fixed debug tests")
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
Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by avayvod@chromium.org
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": 160001, "attempt_start_ts": 1484264590477480,
"parent_rev": "d6d88b91c89eb59510f0628b0945e5cd65df3871", "commit_rev":
"2135a640a1547f44b62828bac84dc713d39652c6"}
Message was sent while issue was closed.
Description was changed from
==========
[Media, Video] Enable the video track for a new renderer.
Video track needs to be reenabled before the new renderer is
created so that the callbacks attach properly. For that, add
two new callbacks to PipelineController for when the pipeline
is about to resume and is resumed.
This adds a potential problem when the video is shown or hidden
in between the two events. So don't change the video track if
the pipeline is resuming or seeking but check for the necessary
conditions in the resumed callback.
BUG=678374
TEST=media_[blink_]unittests + manual testing
==========
to
==========
[Media, Video] Enable the video track for a new renderer.
Video track needs to be reenabled before the new renderer is
created so that the callbacks attach properly. For that, add
two new callbacks to PipelineController for when the pipeline
is about to resume and is resumed.
This adds a potential problem when the video is shown or hidden
in between the two events. So don't change the video track if
the pipeline is resuming or seeking but check for the necessary
conditions in the resumed callback.
BUG=678374
TEST=media_[blink_]unittests + manual testing
Review-Url: https://codereview.chromium.org/2618883002
Cr-Commit-Position: refs/heads/master@{#443404}
Committed:
https://chromium.googlesource.com/chromium/src/+/2135a640a1547f44b62828bac84d...
==========
Message was sent while issue was closed.
Committed patchset #9 (id:160001) as https://chromium.googlesource.com/chromium/src/+/2135a640a1547f44b62828bac84d... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
