Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2439543003:
Do not use overlays when VR shell is enabled. (Closed)
|
Created:
4 years, 2 months ago by amp Modified:
4 years 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, agrieve+watch_chromium.org, bajones Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionDo not use overlays when VR shell is enabled.
Fixes VR Shell showing black screen for video when fullscreening.
BUG=654851
Committed: https://crrev.com/2ff7bf42dc2fac16bf7a63507be68e655f3ccfcc
Cr-Commit-Position: refs/heads/master@{#439241}
Patch Set 1 #
Total comments: 1
Patch Set 2 : Added missing ipc struct updates. #Patch Set 3 : Rebase onto vr display decoupling patch. #Patch Set 4 : Remove logging and unneeded files #Patch Set 5 : Rebased against recent display changes and updated naming. #Patch Set 6 : Minor style adjustments #
Total comments: 2
Patch Set 7 : Simplify to just check vr shell flag. #Patch Set 8 : Added TODO to re-enable overlays when ready. #
Total comments: 15
Patch Set 9 : Pulled out both feature checks from WMPI, inverted logic to enable instead of disable #
Total comments: 8
Patch Set 10 : Switch to using constructor arg instead of setter #Patch Set 11 : back to a setter #Patch Set 12 : move overlay feature check back into WMPI #Patch Set 13 : rebase #
Total comments: 10
Patch Set 14 : Address ddorwin comments, move new methods out of ANDROID build check so they compile on all platfo… #
Total comments: 8
Patch Set 15 : address final comments (and a rebase) #
Messages
Total messages: 53 (22 generated)
Description was changed from ========== Do not use overlays when entering fullscreen in VR mode. BUG=654851 ========== to ========== Do not use overlays when entering fullscreen in VR shell. Fixes VR Shell showing black screen for video when fullscreening. BUG=654851 ==========
amp@chromium.org changed reviewers: + liberato@chromium.org, mthiesse@chromium.org
I'm looking for initial feedback, I'll expand out the review to OWNERS after a pass or two. Tagging a display with 'virtual' is possibly not the best way to accomplish this. I'm open to other solutions but I was not able to figure out any others on my own.
On 2016/12/06 00:40:54, amp wrote: > I'm looking for initial feedback, I'll expand out the review to OWNERS after a > pass or two. > > Tagging a display with 'virtual' is possibly not the best way to accomplish > this. > > I'm open to other solutions but I was not able to figure out any others on my > own. You're going to get a lot of pushback for exposing virtual outside of ui/. This is also not a good way to detect whether we're in VR. Instead you should probably add new ViewMsgs for entering and exiting VR, if the renderer needs to know about VR-ness.
general approach in webmediaplayer_impl seems fine % changes to VR-ness after construction that you've noted in render_frame_impl. +1 on sending messages for the transition instead of attaching it to the screen info. thanks -fl https://codereview.chromium.org/2439543003/diff/1/media/blink/webmediaplayer_... File media/blink/webmediaplayer_impl.cc (right): https://codereview.chromium.org/2439543003/diff/1/media/blink/webmediaplayer_... media/blink/webmediaplayer_impl.cc:1404: in_vr_ = in_vr; this might need to change the overlay state immediately. as in, if one enters vr while in full screen, then one should DisableOverlay() here. https://codereview.chromium.org/2439543003/diff/100001/media/blink/webmediapl... File media/blink/webmediaplayer_impl.cc (right): https://codereview.chromium.org/2439543003/diff/100001/media/blink/webmediapl... media/blink/webmediaplayer_impl.cc:1477: void WebMediaPlayerImpl::SetInVr(bool in_vr) { might want EnteredVr / ExitedVr that handle turning off overlays if needed, per your comment elsewhere. mirrors how we handle full screen.
Thanks for the comments. I guess I was thinking I was being less intrusive by trying to use existing dependencies instead of creating new paths etc. But I agree that a direct approach makes more sense (and would solve the problem of detecting entering vr after the player has already been created). I'll start trying to wire that up... https://codereview.chromium.org/2439543003/diff/100001/media/blink/webmediapl... File media/blink/webmediaplayer_impl.cc (right): https://codereview.chromium.org/2439543003/diff/100001/media/blink/webmediapl... media/blink/webmediaplayer_impl.cc:1477: void WebMediaPlayerImpl::SetInVr(bool in_vr) { On 2016/12/06 17:54:43, liberato wrote: > might want EnteredVr / ExitedVr that handle turning off overlays if needed, per > your comment elsewhere. mirrors how we handle full screen. Yea that sounds better.
On 2016/12/06 18:08:16, amp wrote: > Thanks for the comments. > > I guess I was thinking I was being less intrusive by trying to use existing > dependencies instead of creating new paths etc. > > But I agree that a direct approach makes more sense (and would solve the problem > of detecting entering vr after the player has already been created). > > I'll start trying to wire that up... > > https://codereview.chromium.org/2439543003/diff/100001/media/blink/webmediapl... > File media/blink/webmediaplayer_impl.cc (right): > > https://codereview.chromium.org/2439543003/diff/100001/media/blink/webmediapl... > media/blink/webmediaplayer_impl.cc:1477: void WebMediaPlayerImpl::SetInVr(bool > in_vr) { > On 2016/12/06 17:54:43, liberato wrote: > > might want EnteredVr / ExitedVr that handle turning off overlays if needed, > per > > your comment elsewhere. mirrors how we handle full screen. > > Yea that sounds better. I'm not sure I can make this all into pure messages and handlers. When we enter vr if the player already exists then messages would be able to notify it to not use overlays (this is the case that the current patch set doesn't handle). However, if no player exists when we enter vr (navigating to a page with a video while in vr will hit this scenario), then there will be no 'enteredVR' message sent to the player when it is created (unless there is some way to queue up messages that I'm not aware of). The current patch set handles this by adding that state into the display. Wouldn't we need to keep track of whether we are in vr or not for that scenario? (it doesn't necessarily need to be in the display, but it is convenient). I guess I need both (messages and current state) to cover all scenarios. WDYT?
On 2016/12/06 18:54:02, amp wrote: > On 2016/12/06 18:08:16, amp wrote: > > Thanks for the comments. > > > > I guess I was thinking I was being less intrusive by trying to use existing > > dependencies instead of creating new paths etc. > > > > But I agree that a direct approach makes more sense (and would solve the > problem > > of detecting entering vr after the player has already been created). > > > > I'll start trying to wire that up... > > > > > https://codereview.chromium.org/2439543003/diff/100001/media/blink/webmediapl... > > File media/blink/webmediaplayer_impl.cc (right): > > > > > https://codereview.chromium.org/2439543003/diff/100001/media/blink/webmediapl... > > media/blink/webmediaplayer_impl.cc:1477: void WebMediaPlayerImpl::SetInVr(bool > > in_vr) { > > On 2016/12/06 17:54:43, liberato wrote: > > > might want EnteredVr / ExitedVr that handle turning off overlays if needed, > > per > > > your comment elsewhere. mirrors how we handle full screen. > > > > Yea that sounds better. > > > > I'm not sure I can make this all into pure messages and handlers. When we enter > vr if the player already exists then messages would be able to notify it to not > use overlays (this is the case that the current patch set doesn't handle). > > However, if no player exists when we enter vr (navigating to a page with a video > while in vr will hit this scenario), then there will be no 'enteredVR' message > sent to the player when it is created (unless there is some way to queue up > messages that I'm not aware of). The current patch set handles this by adding > that state into the display. > > Wouldn't we need to keep track of whether we are in vr or not for that scenario? > (it doesn't necessarily need to be in the display, but it is convenient). > > > I guess I need both (messages and current state) to cover all scenarios. WDYT? sgtm, whatever's creating the media player would need to store the VR state. And definitely don't do it in the display :P
On 2016/12/06 18:58:06, mthiesse wrote: > On 2016/12/06 18:54:02, amp wrote: > > On 2016/12/06 18:08:16, amp wrote: > > > Thanks for the comments. > > > > > > I guess I was thinking I was being less intrusive by trying to use existing > > > dependencies instead of creating new paths etc. > > > > > > But I agree that a direct approach makes more sense (and would solve the > > problem > > > of detecting entering vr after the player has already been created). > > > > > > I'll start trying to wire that up... > > > > > > > > > https://codereview.chromium.org/2439543003/diff/100001/media/blink/webmediapl... > > > File media/blink/webmediaplayer_impl.cc (right): > > > > > > > > > https://codereview.chromium.org/2439543003/diff/100001/media/blink/webmediapl... > > > media/blink/webmediaplayer_impl.cc:1477: void > WebMediaPlayerImpl::SetInVr(bool > > > in_vr) { > > > On 2016/12/06 17:54:43, liberato wrote: > > > > might want EnteredVr / ExitedVr that handle turning off overlays if > needed, > > > per > > > > your comment elsewhere. mirrors how we handle full screen. > > > > > > Yea that sounds better. > > > > > > > > I'm not sure I can make this all into pure messages and handlers. When we > enter > > vr if the player already exists then messages would be able to notify it to > not > > use overlays (this is the case that the current patch set doesn't handle). > > > > However, if no player exists when we enter vr (navigating to a page with a > video > > while in vr will hit this scenario), then there will be no 'enteredVR' message > > sent to the player when it is created (unless there is some way to queue up > > messages that I'm not aware of). The current patch set handles this by adding > > that state into the display. > > > > Wouldn't we need to keep track of whether we are in vr or not for that > scenario? > > (it doesn't necessarily need to be in the display, but it is convenient). > > > > > > I guess I need both (messages and current state) to cover all scenarios. > WDYT? > > sgtm, whatever's creating the media player would need to store the VR state. And > definitely don't do it in the display :P The html media element is what is creating the media player. It won't exist at the time of navigation, so there needs to be some way to query if we are in vr or not after it loads. Do we have any way of doing that now?
On 2016/12/06 19:00:03, amp wrote: > On 2016/12/06 18:58:06, mthiesse wrote: > > On 2016/12/06 18:54:02, amp wrote: > > > On 2016/12/06 18:08:16, amp wrote: > > > > Thanks for the comments. > > > > > > > > I guess I was thinking I was being less intrusive by trying to use > existing > > > > dependencies instead of creating new paths etc. > > > > > > > > But I agree that a direct approach makes more sense (and would solve the > > > problem > > > > of detecting entering vr after the player has already been created). > > > > > > > > I'll start trying to wire that up... > > > > > > > > > > > > > > https://codereview.chromium.org/2439543003/diff/100001/media/blink/webmediapl... > > > > File media/blink/webmediaplayer_impl.cc (right): > > > > > > > > > > > > > > https://codereview.chromium.org/2439543003/diff/100001/media/blink/webmediapl... > > > > media/blink/webmediaplayer_impl.cc:1477: void > > WebMediaPlayerImpl::SetInVr(bool > > > > in_vr) { > > > > On 2016/12/06 17:54:43, liberato wrote: > > > > > might want EnteredVr / ExitedVr that handle turning off overlays if > > needed, > > > > per > > > > > your comment elsewhere. mirrors how we handle full screen. > > > > > > > > Yea that sounds better. > > > > > > > > > > > > I'm not sure I can make this all into pure messages and handlers. When we > > enter > > > vr if the player already exists then messages would be able to notify it to > > not > > > use overlays (this is the case that the current patch set doesn't handle). > > > > > > However, if no player exists when we enter vr (navigating to a page with a > > video > > > while in vr will hit this scenario), then there will be no 'enteredVR' > message > > > sent to the player when it is created (unless there is some way to queue up > > > messages that I'm not aware of). The current patch set handles this by > adding > > > that state into the display. > > > > > > Wouldn't we need to keep track of whether we are in vr or not for that > > scenario? > > > (it doesn't necessarily need to be in the display, but it is convenient). > > > > > > > > > I guess I need both (messages and current state) to cover all scenarios. > > WDYT? > > > > sgtm, whatever's creating the media player would need to store the VR state. > And > > definitely don't do it in the display :P > > The html media element is what is creating the media player. > > It won't exist at the time of navigation, so there needs to be some way to query > if we are in vr or not after it loads. > > Do we have any way of doing that now? I don't think we currently do. Perhaps Brandon knows the right place to store VR state?
PTAL. After some more research it turns out the proper way to handle this will probably be using GVR reprojection video surfaces (see issue 673886). In the meantime I'm going to fix the immediate black screen issue by tying disabling web media player overlays to the vr shell flag. This isn't something we want long term when we start turning on vr shell for all users, but it should be fine for the small number of users who actually enable the vr shell flag manually.
ddorwin@chromium.org changed reviewers: + ddorwin@chromium.org
https://codereview.chromium.org/2439543003/diff/140001/content/renderer/rende... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/2439543003/diff/140001/content/renderer/rende... content/renderer/render_frame_impl.cc:2863: // TODO(crbug/673886): Enable overlays in VR shell after video reprojection s/in/when ... is enabled/. This affects all videos, regardless of whether we are in VR shell. https://codereview.chromium.org/2439543003/diff/140001/content/renderer/rende... content/renderer/render_frame_impl.cc:2864: // surface is supported. Also update the CL description. https://codereview.chromium.org/2439543003/diff/140001/media/blink/webmediapl... File media/blink/webmediaplayer_impl.cc (right): https://codereview.chromium.org/2439543003/diff/140001/media/blink/webmediapl... media/blink/webmediaplayer_impl.cc:254: !base::FeatureList::IsEnabled(media::kOverlayFullscreenVideo); I think it would be better if media/ didn't rely on flags. Since we're already adding a setter, should we move this too? https://codereview.chromium.org/2439543003/diff/140001/media/blink/webmediapl... media/blink/webmediaplayer_impl.cc:1469: void WebMediaPlayerImpl::SetDisableFullscreenOverlays(bool disable_overlays) { It's strange to set a disable with a bool. I suggest inverting the logic. We should also invert the member name. Most of the logic around it is negative anyway.
https://codereview.chromium.org/2439543003/diff/140001/content/renderer/rende... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/2439543003/diff/140001/content/renderer/rende... content/renderer/render_frame_impl.cc:2866: base::FeatureList::IsEnabled(features::kVrShell)); You're disabling FullscreenOverlays when VrShell is enabled at all? Why not only when currently in VrShell?
https://codereview.chromium.org/2439543003/diff/140001/content/renderer/rende... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/2439543003/diff/140001/content/renderer/rende... content/renderer/render_frame_impl.cc:2865: media_player->SetDisableFullscreenOverlays( this will override entirely what's in the constructor. probably something more like this: if (IsEnabled) wmpi->DisableFullscreenOverlays(); // or wmpi->SetAllowFullscreenOverlays(false); https://codereview.chromium.org/2439543003/diff/140001/media/blink/webmediapl... File media/blink/webmediaplayer_impl.h (right): https://codereview.chromium.org/2439543003/diff/140001/media/blink/webmediapl... media/blink/webmediaplayer_impl.h:210: void SetDisableFullscreenOverlays(bool disable_overlays); SetDisable is a little confusing. SetAllowFullscreenOverlays(bool) might be better. also worth a comment that this is set by default during construction, and one probably doesn't want to call it except for VR. it'll screw up the experiment results that are attached to kForceVideoOverlays if it's called too much. actually, maybe DisableFullScreenOverlaysForVR() is a better name, so it's super clear that one should not call this unless one is you. :)
Description was changed from ========== Do not use overlays when entering fullscreen in VR shell. Fixes VR Shell showing black screen for video when fullscreening. BUG=654851 ========== to ========== Do not use overlays when VR shell is enabled. Fixes VR Shell showing black screen for video when fullscreening. BUG=654851 ==========
https://codereview.chromium.org/2439543003/diff/140001/content/renderer/rende... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/2439543003/diff/140001/content/renderer/rende... content/renderer/render_frame_impl.cc:2863: // TODO(crbug/673886): Enable overlays in VR shell after video reprojection On 2016/12/13 22:54:49, ddorwin wrote: > s/in/when ... is enabled/. Done. > > This affects all videos, regardless of whether we are in VR shell. Yes it does and so is less desirable, but easier to implement. https://codereview.chromium.org/2439543003/diff/140001/content/renderer/rende... content/renderer/render_frame_impl.cc:2864: // surface is supported. On 2016/12/13 22:54:49, ddorwin wrote: > Also update the CL description. Done. https://codereview.chromium.org/2439543003/diff/140001/content/renderer/rende... content/renderer/render_frame_impl.cc:2865: media_player->SetDisableFullscreenOverlays( On 2016/12/13 23:03:29, liberato wrote: > this will override entirely what's in the constructor. probably something more > like this: > > if (IsEnabled) > wmpi->DisableFullscreenOverlays(); > // or > wmpi->SetAllowFullscreenOverlays(false); Should we pull out the media flag check currently in the WMPI constructor? https://codereview.chromium.org/2439543003/diff/140001/content/renderer/rende... content/renderer/render_frame_impl.cc:2866: base::FeatureList::IsEnabled(features::kVrShell)); On 2016/12/13 23:03:08, mthiesse wrote: > You're disabling FullscreenOverlays when VrShell is enabled at all? Why not only > when currently in VrShell? Detecting when we are in vr shell was getting messy any of the various ways I tried to do it. The proper thing to do seems to be to not worry about any of this and just properly support the gvr surface reprojection. This fix is just a stop gap until that is implemented so that full screen video can work at all in vr. https://codereview.chromium.org/2439543003/diff/140001/media/blink/webmediapl... File media/blink/webmediaplayer_impl.cc (right): https://codereview.chromium.org/2439543003/diff/140001/media/blink/webmediapl... media/blink/webmediaplayer_impl.cc:254: !base::FeatureList::IsEnabled(media::kOverlayFullscreenVideo); On 2016/12/13 22:54:49, ddorwin wrote: > I think it would be better if media/ didn't rely on flags. Since we're already > adding a setter, should we move this too? That wouldn't be too hard to do and would maybe allow adjusting the finch experiment logic (still looking into if that is possible) to not pollute the experiment data when vr shell is enabled. https://codereview.chromium.org/2439543003/diff/140001/media/blink/webmediapl... media/blink/webmediaplayer_impl.cc:1469: void WebMediaPlayerImpl::SetDisableFullscreenOverlays(bool disable_overlays) { On 2016/12/13 22:54:49, ddorwin wrote: > It's strange to set a disable with a bool. I suggest inverting the logic. > > We should also invert the member name. Most of the logic around it is negative > anyway. It was hard to follow when I first looked at it so I think I agree. I'll look at doing that if I move the other flag out as well, assuming the finch experiment still makes sense with the inversion. https://codereview.chromium.org/2439543003/diff/140001/media/blink/webmediapl... File media/blink/webmediaplayer_impl.h (right): https://codereview.chromium.org/2439543003/diff/140001/media/blink/webmediapl... media/blink/webmediaplayer_impl.h:210: void SetDisableFullscreenOverlays(bool disable_overlays); On 2016/12/13 23:03:29, liberato wrote: > SetDisable is a little confusing. SetAllowFullscreenOverlays(bool) might be > better. > > also worth a comment that this is set by default during construction, and one > probably doesn't want to call it except for VR. it'll screw up the experiment > results that are attached to kForceVideoOverlays if it's called too much. > > actually, maybe DisableFullScreenOverlaysForVR() is a better name, so it's super > clear that one should not call this unless one is you. :) I'm leaning towards SetAllowFullscreenOverlays and checking both features instead of doing anything in the constructor. I'm looking into if we can signal finch when the experiment data is invalid because of vr shell.
https://codereview.chromium.org/2439543003/diff/140001/media/blink/webmediapl... File media/blink/webmediaplayer_impl.h (right): https://codereview.chromium.org/2439543003/diff/140001/media/blink/webmediapl... media/blink/webmediaplayer_impl.h:210: void SetDisableFullscreenOverlays(bool disable_overlays); On 2016/12/13 23:40:02, amp wrote: > On 2016/12/13 23:03:29, liberato wrote: > > SetDisable is a little confusing. SetAllowFullscreenOverlays(bool) might be > > better. > > > > also worth a comment that this is set by default during construction, and one > > probably doesn't want to call it except for VR. it'll screw up the experiment > > results that are attached to kForceVideoOverlays if it's called too much. > > > > actually, maybe DisableFullScreenOverlaysForVR() is a better name, so it's > super > > clear that one should not call this unless one is you. :) > > I'm leaning towards SetAllowFullscreenOverlays and checking both features > instead of doing anything in the constructor. > > I'm looking into if we can signal finch when the experiment data is invalid > because of vr shell. Yes it is possible to make finch ignore data when vr shell is enabled. See cl/141952159
PTAL. Updated per latest comments and we should be able to address the ongoing experiment concerns as well (with a server side finch config update).
https://codereview.chromium.org/2439543003/diff/160001/content/renderer/rende... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/2439543003/diff/160001/content/renderer/rende... content/renderer/render_frame_impl.cc:2863: // TODO(crbug/673886): Enable overlays for VR shell when video reprojection For clarity, how about: Re-enable overlays with VR Shell enabled when VR Shell's video reprojection surface is enabled. https://codereview.chromium.org/2439543003/diff/160001/media/blink/webmediapl... File media/blink/webmediaplayer_impl.h (right): https://codereview.chromium.org/2439543003/diff/160001/media/blink/webmediapl... media/blink/webmediaplayer_impl.h:210: void SetEnableFullscreenOverlays(bool disable_overlays); Does it makes sense to have a function for this? Looks like it's only ever set once on creation, so maybe it should be part of the constructor?
https://codereview.chromium.org/2439543003/diff/160001/content/renderer/rende... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/2439543003/diff/160001/content/renderer/rende... content/renderer/render_frame_impl.cc:2863: // TODO(crbug/673886): Enable overlays for VR shell when video reprojection On 2016/12/14 16:08:19, mthiesse wrote: > For clarity, how about: Re-enable overlays with VR Shell enabled when VR Shell's > video reprojection surface is enabled. Sounds good. I was trying to find a good way to word it and coming up short. Done. https://codereview.chromium.org/2439543003/diff/160001/media/blink/webmediapl... File media/blink/webmediaplayer_impl.h (right): https://codereview.chromium.org/2439543003/diff/160001/media/blink/webmediapl... media/blink/webmediaplayer_impl.h:210: void SetEnableFullscreenOverlays(bool disable_overlays); On 2016/12/14 16:08:19, mthiesse wrote: > Does it makes sense to have a function for this? Looks like it's only ever set > once on creation, so maybe it should be part of the constructor? After the inversion and pulling out the other feature check it never changes so it could work in the constructor. @liberato, WDYT?
On 2016/12/14 18:15:05, amp wrote: > https://codereview.chromium.org/2439543003/diff/160001/content/renderer/rende... > File content/renderer/render_frame_impl.cc (right): > > https://codereview.chromium.org/2439543003/diff/160001/content/renderer/rende... > content/renderer/render_frame_impl.cc:2863: // TODO(crbug/673886): Enable > overlays for VR shell when video reprojection > On 2016/12/14 16:08:19, mthiesse wrote: > > For clarity, how about: Re-enable overlays with VR Shell enabled when VR > Shell's > > video reprojection surface is enabled. > > Sounds good. I was trying to find a good way to word it and coming up short. > > Done. > > https://codereview.chromium.org/2439543003/diff/160001/media/blink/webmediapl... > File media/blink/webmediaplayer_impl.h (right): > > https://codereview.chromium.org/2439543003/diff/160001/media/blink/webmediapl... > media/blink/webmediaplayer_impl.h:210: void SetEnableFullscreenOverlays(bool > disable_overlays); > On 2016/12/14 16:08:19, mthiesse wrote: > > Does it makes sense to have a function for this? Looks like it's only ever set > > once on creation, so maybe it should be part of the constructor? > > After the inversion and pulling out the other feature check it never changes so > it could work in the constructor. > > @liberato, WDYT? Patch set 10 has it in the constructor, but I can switch back to Patch set 9 if there is some reason to not do it in the constructor.
lgtm % nits. thanks -fl https://codereview.chromium.org/2439543003/diff/160001/content/renderer/rende... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/2439543003/diff/160001/content/renderer/rende... content/renderer/render_frame_impl.cc:2863: // TODO(crbug/673886): Enable overlays for VR shell when video reprojection On 2016/12/14 18:15:04, amp wrote: > On 2016/12/14 16:08:19, mthiesse wrote: > > For clarity, how about: Re-enable overlays with VR Shell enabled when VR > Shell's > > video reprojection surface is enabled. > > Sounds good. I was trying to find a good way to word it and coming up short. > > Done. please also include in the TODO (or the bug) to move the kOverlayFullscreenVideo check back into the constructor, and remove SetEnableFullscreenOverlays entirely. it's less burden on whoever instantiates WMPI, since it's an implementation detail. it's only exposed because getting the VR state into WMPI directly turned out to be even worse. https://codereview.chromium.org/2439543003/diff/160001/media/blink/webmediapl... File media/blink/webmediaplayer_impl.h (right): https://codereview.chromium.org/2439543003/diff/160001/media/blink/webmediapl... media/blink/webmediaplayer_impl.h:210: void SetEnableFullscreenOverlays(bool disable_overlays); On 2016/12/14 18:15:04, amp wrote: > On 2016/12/14 16:08:19, mthiesse wrote: > > Does it makes sense to have a function for this? Looks like it's only ever set > > once on creation, so maybe it should be part of the constructor? > > After the inversion and pulling out the other feature check it never changes so > it could work in the constructor. > > @liberato, WDYT? i find adding booleans in the constructor to be error-prone. since we already have plenty of setters that are used in render_frame_impl, i'd prefer to keep this explicit too. to be honest, i preferred when WMPI figured it out for itself. Once we remove this flag for VR, we'll probably want to put it back that way. also, you've got |disable_overlays| instead of |enable_overlays| here.
lgtm, my nits can be resolved either way :P
On 2016/12/14 19:39:07, liberato wrote: > lgtm % nits. > > thanks > -fl > > https://codereview.chromium.org/2439543003/diff/160001/content/renderer/rende... > File content/renderer/render_frame_impl.cc (right): > > https://codereview.chromium.org/2439543003/diff/160001/content/renderer/rende... > content/renderer/render_frame_impl.cc:2863: // TODO(crbug/673886): Enable > overlays for VR shell when video reprojection > On 2016/12/14 18:15:04, amp wrote: > > On 2016/12/14 16:08:19, mthiesse wrote: > > > For clarity, how about: Re-enable overlays with VR Shell enabled when VR > > Shell's > > > video reprojection surface is enabled. > > > > Sounds good. I was trying to find a good way to word it and coming up short. > > > > Done. > > please also include in the TODO (or the bug) to move the kOverlayFullscreenVideo > check back into the constructor, and remove SetEnableFullscreenOverlays > entirely. it's less burden on whoever instantiates WMPI, since it's an > implementation detail. it's only exposed because getting the VR state into WMPI > directly turned out to be even worse. > > https://codereview.chromium.org/2439543003/diff/160001/media/blink/webmediapl... > File media/blink/webmediaplayer_impl.h (right): > > https://codereview.chromium.org/2439543003/diff/160001/media/blink/webmediapl... > media/blink/webmediaplayer_impl.h:210: void SetEnableFullscreenOverlays(bool > disable_overlays); > On 2016/12/14 18:15:04, amp wrote: > > On 2016/12/14 16:08:19, mthiesse wrote: > > > Does it makes sense to have a function for this? Looks like it's only ever > set > > > once on creation, so maybe it should be part of the constructor? > > > > After the inversion and pulling out the other feature check it never changes > so > > it could work in the constructor. > > > > @liberato, WDYT? > > i find adding booleans in the constructor to be error-prone. since we already > have plenty of setters that are used in render_frame_impl, i'd prefer to keep > this explicit too. > > to be honest, i preferred when WMPI figured it out for itself. Once we remove > this flag for VR, we'll probably want to put it back that way. > > also, you've got |disable_overlays| instead of |enable_overlays| here. If we are just going to put the feature check back in the constructor then I think I prefer leaving it as is and disabling it through a setter like I had in the previous patch. @ddorwin, you had suggested pulling out the feature check from WMPI. Do you see it living here in render_frame_impl indefinetly? I agree with liberato that it feels like internal details of WMPI and should be there if possible. (VrShell isn't accessible from inside WMPI so it has to be external to WMPI, but it should be fairly short lived).
The CQ bit was checked by amp@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...
amp@chromium.org changed reviewers: + nasko@chromium.org
+nasko for content/ OWNER
https://codereview.chromium.org/2439543003/diff/160001/content/renderer/rende... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/2439543003/diff/160001/content/renderer/rende... content/renderer/render_frame_impl.cc:2863: // TODO(crbug/673886): Enable overlays for VR shell when video reprojection On 2016/12/14 19:39:06, liberato wrote: > On 2016/12/14 18:15:04, amp wrote: > > On 2016/12/14 16:08:19, mthiesse wrote: > > > For clarity, how about: Re-enable overlays with VR Shell enabled when VR > > Shell's > > > video reprojection surface is enabled. > > > > Sounds good. I was trying to find a good way to word it and coming up short. > > > > Done. > > please also include in the TODO (or the bug) to move the kOverlayFullscreenVideo > check back into the constructor, and remove SetEnableFullscreenOverlays > entirely. it's less burden on whoever instantiates WMPI, since it's an > implementation detail. it's only exposed because getting the VR state into WMPI > directly turned out to be even worse. I moved the kOverlayFullscreenVideo back into the constructor. I don't think there is a strong reason to pull it out now if we are just going to put it back later. https://codereview.chromium.org/2439543003/diff/160001/media/blink/webmediapl... File media/blink/webmediaplayer_impl.h (right): https://codereview.chromium.org/2439543003/diff/160001/media/blink/webmediapl... media/blink/webmediaplayer_impl.h:210: void SetEnableFullscreenOverlays(bool disable_overlays); On 2016/12/14 19:39:07, liberato wrote: > On 2016/12/14 18:15:04, amp wrote: > > On 2016/12/14 16:08:19, mthiesse wrote: > > > Does it makes sense to have a function for this? Looks like it's only ever > set > > > once on creation, so maybe it should be part of the constructor? > > > > After the inversion and pulling out the other feature check it never changes > so > > it could work in the constructor. > > > > @liberato, WDYT? > > i find adding booleans in the constructor to be error-prone. since we already > have plenty of setters that are used in render_frame_impl, i'd prefer to keep > this explicit too. > > to be honest, i preferred when WMPI figured it out for itself. Once we remove > this flag for VR, we'll probably want to put it back that way. > > also, you've got |disable_overlays| instead of |enable_overlays| here. Done. Going with a setter in latest patch.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
The CQ bit was checked by amp@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...
Rubberstamp LGTM
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...) win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...) win_clang on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)
https://codereview.chromium.org/2439543003/diff/240001/content/renderer/rende... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/2439543003/diff/240001/content/renderer/rende... content/renderer/render_frame_impl.cc:2861: // TODO(crbug/673886): Re-enable overlays with VR shell enabled when VR crbug.com so it is clickable. https://codereview.chromium.org/2439543003/diff/240001/content/renderer/rende... content/renderer/render_frame_impl.cc:2863: media_player->SetEnableFullscreenOverlays( This forcibly overrides the current (default) setting and potentially future changes (none exist at the moment) regardless of the VrShell flag. To fix this, we need to Set(false) iff VrShell is enabled. However, such subtleties are a good argument for having all the logic for this setting in exactly one place. That would also make it clear that we need to correctly handle the feature flag metrics. https://codereview.chromium.org/2439543003/diff/240001/media/blink/webmediapl... File media/blink/webmediaplayer_impl.cc (right): https://codereview.chromium.org/2439543003/diff/240001/media/blink/webmediapl... media/blink/webmediaplayer_impl.cc:250: force_video_overlays_ = base::CommandLine::ForCurrentProcess()->HasSwitch( Even more so than base:Features, command line switches should not be accessed in media/. I filed https://bugs.chromium.org/p/chromium/issues/detail?id=674349 to switch this one. If we had all of this set in one place, we could use this being true to cause enable_fullscreen_video_overlays_ to be set to true and ignore all other logic for setting that value. That would allow us to simplify lines 348 and 355 and maybe simplify the member description. https://codereview.chromium.org/2439543003/diff/240001/media/blink/webmediapl... media/blink/webmediaplayer_impl.cc:1573: if (force_video_overlays_) This is the only instance that is not related to enable_fullscreen_video_overlays_. https://codereview.chromium.org/2439543003/diff/240001/media/blink/webmediapl... File media/blink/webmediaplayer_impl.h (right): https://codereview.chromium.org/2439543003/diff/240001/media/blink/webmediapl... media/blink/webmediaplayer_impl.h:539: // Allow use of SurfaceView on Android. (Ignored when s/Allow use of/Use/ The comment is very Android-specific, which is not clear from the variable name nor is the code Android-specific. Perhaps there should be a general comment with a separate sentence that says, "On Android, ..." This "Ignored" relationship is a bit confusing and fragile. It would be nice to clean that up. See my comments in the .cc file.
https://codereview.chromium.org/2439543003/diff/240001/content/renderer/rende... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/2439543003/diff/240001/content/renderer/rende... content/renderer/render_frame_impl.cc:2861: // TODO(crbug/673886): Re-enable overlays with VR shell enabled when VR On 2016/12/15 01:23:52, ddorwin wrote: > http://crbug.com so it is clickable. Done. https://codereview.chromium.org/2439543003/diff/240001/content/renderer/rende... content/renderer/render_frame_impl.cc:2863: media_player->SetEnableFullscreenOverlays( On 2016/12/15 01:23:52, ddorwin wrote: > This forcibly overrides the current (default) setting and potentially future > changes (none exist at the moment) regardless of the VrShell flag. > > To fix this, we need to Set(false) iff VrShell is enabled. > > However, such subtleties are a good argument for having all the logic for this > setting in exactly one place. That would also make it clear that we need to > correctly handle the feature flag metrics. Ok. I'll pull back out the overlay feature check here and do the logic for making sure we have the right value being passed into the setter. I'll also update the TODO that we should remove this after vr shell is fixed and move the overlay feature check back into constructor (per an ealier comment form liberato@). https://codereview.chromium.org/2439543003/diff/240001/media/blink/webmediapl... File media/blink/webmediaplayer_impl.cc (right): https://codereview.chromium.org/2439543003/diff/240001/media/blink/webmediapl... media/blink/webmediaplayer_impl.cc:250: force_video_overlays_ = base::CommandLine::ForCurrentProcess()->HasSwitch( On 2016/12/15 01:23:52, ddorwin wrote: > Even more so than base:Features, command line switches should not be accessed in > media/. > > I filed https://bugs.chromium.org/p/chromium/issues/detail?id=674349 to switch > this one. > > If we had all of this set in one place, we could use this being true to cause > enable_fullscreen_video_overlays_ to be set to true and ignore all other logic > for setting that value. That would allow us to simplify lines 348 and 355 and > maybe simplify the member description. Ack. It seems to operate slightly differently by creating the surface up front (on line 1569 in the 'CreateRenderer' method) and then stops creating and removing surface on lines 348 and 355. I agree it is confusing and a few of my patches probably break it (I think the latest few are fine). So I'll leave cleaning up the flag to a future change. :) https://codereview.chromium.org/2439543003/diff/240001/media/blink/webmediapl... media/blink/webmediaplayer_impl.cc:1573: if (force_video_overlays_) On 2016/12/15 01:23:52, ddorwin wrote: > This is the only instance that is not related to > enable_fullscreen_video_overlays_. Yea, the EnableOverlay call will create a surface and get a callback with the id. The check here just does it at initialization and then makes sure no other overlay methods are called again for the lifetime of WMPI so it can use the one surface the whole time. https://codereview.chromium.org/2439543003/diff/240001/media/blink/webmediapl... File media/blink/webmediaplayer_impl.h (right): https://codereview.chromium.org/2439543003/diff/240001/media/blink/webmediapl... media/blink/webmediaplayer_impl.h:539: // Allow use of SurfaceView on Android. (Ignored when On 2016/12/15 01:23:52, ddorwin wrote: > s/Allow use of/Use/ > > The comment is very Android-specific, which is not clear from the variable name > nor is the code Android-specific. Perhaps there should be a general comment with > a separate sentence that says, "On Android, ..." > > This "Ignored" relationship is a bit confusing and fragile. It would be nice to > clean that up. See my comments in the .cc file. I made an attempt at cleaning up the comments. Per cc comments I'll leave the force flag for a later change (but I did update it's comments a bit).
The CQ bit was checked by amp@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_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
LGTM with two nits. I think we should clean up the overlay logic in WMPI.cc in a separate CL. https://codereview.chromium.org/2439543003/diff/260001/content/renderer/rende... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/2439543003/diff/260001/content/renderer/rende... content/renderer/render_frame_impl.cc:2860: // TODO(http://crbug.com/673886): Re-enable overlays with VR shell enabled when VR `>80 chars https://codereview.chromium.org/2439543003/diff/260001/media/blink/webmediapl... File media/blink/webmediaplayer_impl.cc (right): https://codereview.chromium.org/2439543003/diff/260001/media/blink/webmediapl... media/blink/webmediaplayer_impl.cc:349: if (!force_video_overlays_ && enable_fullscreen_video_overlays_) Separate from this CL: This logic is not obvious. Specifically, why we would not call this when force_video_overlays_ is true. I believe it is because we have already called EnableOverlay() in CreateRenderer(). But this is inferring the current state from a cause. It would seem to make more sense to check the state here, and there is already a member tracking this. A) Thus, I wonder if this shouldn't be: if (!overlay_enabled_ && ... B) However, this would be better if EnableOverlay() handled this internally rather than requiring all call sites to handle it. Thus, we could remove the first half of the condition and add if(overlay_enabled_) return; to EnableOverlay(). https://codereview.chromium.org/2439543003/diff/260001/media/blink/webmediapl... media/blink/webmediaplayer_impl.cc:356: if (!force_video_overlays_ && enable_fullscreen_video_overlays_) Similarly, enable_fullscreen_video_overlays_ is the cause rather than the state. A) And this would be if (!force_video_overlays_ && overlay_enabled_). B) If we went with this option above, this could probably just be a call to DisableOverlay() in all cases. That method would return early if ~overlay_enabled_ || force_video_overlays_. https://codereview.chromium.org/2439543003/diff/260001/media/blink/webmediapl... File media/blink/webmediaplayer_impl.h (right): https://codereview.chromium.org/2439543003/diff/260001/media/blink/webmediapl... media/blink/webmediaplayer_impl.h:539: // Force using video overlays for all uses of WMPI. nit: WMPI is the class, so "this class" might be better. However, "all uses" may not be correct. It's not correct for audio-only and maybe not even all video (e.g., off screen). Maybe "Use overlays for all video."
https://codereview.chromium.org/2439543003/diff/260001/content/renderer/rende... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/2439543003/diff/260001/content/renderer/rende... content/renderer/render_frame_impl.cc:2860: // TODO(http://crbug.com/673886): Re-enable overlays with VR shell enabled when VR On 2016/12/16 19:15:18, ddorwin wrote: > `>80 chars Done. Too many rewordings. :) I'm surprised git cl upload didn't catch it. https://codereview.chromium.org/2439543003/diff/260001/media/blink/webmediapl... File media/blink/webmediaplayer_impl.cc (right): https://codereview.chromium.org/2439543003/diff/260001/media/blink/webmediapl... media/blink/webmediaplayer_impl.cc:349: if (!force_video_overlays_ && enable_fullscreen_video_overlays_) On 2016/12/16 19:15:18, ddorwin wrote: > Separate from this CL: > > This logic is not obvious. Specifically, why we would not call this when > force_video_overlays_ is true. I believe it is because we have already called > EnableOverlay() in CreateRenderer(). But this is inferring the current state > from a cause. > > It would seem to make more sense to check the state here, and there is already a > member tracking this. > > A) > Thus, I wonder if this shouldn't be: > if (!overlay_enabled_ && ... > > B) > However, this would be better if EnableOverlay() handled this internally rather > than requiring all call sites to handle it. Thus, we could remove the first half > of the condition and add if(overlay_enabled_) return; to EnableOverlay(). Acknowledged. https://codereview.chromium.org/2439543003/diff/260001/media/blink/webmediapl... media/blink/webmediaplayer_impl.cc:356: if (!force_video_overlays_ && enable_fullscreen_video_overlays_) On 2016/12/16 19:15:18, ddorwin wrote: > Similarly, enable_fullscreen_video_overlays_ is the cause rather than the state. > > A) > And this would be if (!force_video_overlays_ && overlay_enabled_). > > B) > If we went with this option above, this could probably just be a call to > DisableOverlay() in all cases. That method would return early if > ~overlay_enabled_ || force_video_overlays_. Acknowledged. https://codereview.chromium.org/2439543003/diff/260001/media/blink/webmediapl... File media/blink/webmediaplayer_impl.h (right): https://codereview.chromium.org/2439543003/diff/260001/media/blink/webmediapl... media/blink/webmediaplayer_impl.h:539: // Force using video overlays for all uses of WMPI. On 2016/12/16 19:15:18, ddorwin wrote: > nit: WMPI is the class, so "this class" might be better. > > However, "all uses" may not be correct. It's not correct for audio-only and > maybe not even all video (e.g., off screen). > > Maybe "Use overlays for all video." Done.
The CQ bit was checked by amp@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mthiesse@chromium.org, liberato@chromium.org, nasko@chromium.org, ddorwin@chromium.org Link to the patchset: https://codereview.chromium.org/2439543003/#ps280001 (title: "address final comments (and a rebase)")
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": 280001, "attempt_start_ts": 1481916310151280,
"parent_rev": "9f1dbe5310af54f1b7975e5887cd16aad7ecf9fc", "commit_rev":
"1dd4559b2eefa25c3df2c8fe3e218fecc7a2848a"}
Message was sent while issue was closed.
Description was changed from ========== Do not use overlays when VR shell is enabled. Fixes VR Shell showing black screen for video when fullscreening. BUG=654851 ========== to ========== Do not use overlays when VR shell is enabled. Fixes VR Shell showing black screen for video when fullscreening. BUG=654851 Review-Url: https://codereview.chromium.org/2439543003 ==========
Message was sent while issue was closed.
Committed patchset #15 (id:280001)
Message was sent while issue was closed.
Description was changed from ========== Do not use overlays when VR shell is enabled. Fixes VR Shell showing black screen for video when fullscreening. BUG=654851 Review-Url: https://codereview.chromium.org/2439543003 ========== to ========== Do not use overlays when VR shell is enabled. Fixes VR Shell showing black screen for video when fullscreening. BUG=654851 Committed: https://crrev.com/2ff7bf42dc2fac16bf7a63507be68e655f3ccfcc Cr-Commit-Position: refs/heads/master@{#439241} ==========
Message was sent while issue was closed.
Patchset 15 (id:??) landed as https://crrev.com/2ff7bf42dc2fac16bf7a63507be68e655f3ccfcc Cr-Commit-Position: refs/heads/master@{#439241} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
