|
|
Created:
4 years, 5 months ago by AndyWu Modified:
4 years, 5 months ago Reviewers:
Tima Vaisburd, watk, Simeon, boliu, liberato (no reviews please), halliwell, DaleCurtis, brettw CC:
alokp+watch_chromium.org, chromium-reviews, feature-media-reviews_chromium.org, halliwell+watch_chromium.org, lcwu+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[Cast for ATV] Enable UMP on Cast for ATV
1. Enable unified media pepeline on Cast for ATV.
2. Add a commend parameter to force WMPI starting in fullscreen
mode so that it would use SurfaceView instead of SurfaceTexture on
Android.
BUG=internal b/29463340
Committed: https://crrev.com/ee562e963c4fd7bdc4e885ee13050c97adfc9f3a
Cr-Commit-Position: refs/heads/master@{#406615}
Patch Set 1 #
Total comments: 7
Patch Set 2 : [Cast for ATV] Enable UMP on Cast for ATV #
Total comments: 2
Patch Set 3 : [Cast for ATV] Enable UMP on Cast for ATV #Patch Set 4 : [Cast for ATV] Enable UMP on Cast for ATV #
Total comments: 12
Patch Set 5 : Make changes per comments on PS 4 #Patch Set 6 : fix typos #Patch Set 7 : rebase #Patch Set 8 : fix unit tests #Patch Set 9 : fix unit tests #
Total comments: 1
Patch Set 10 : Fix unit tests #
Messages
Total messages: 52 (29 generated)
Description was changed from ========== [Cast for ATV] Enable UMP on Cast for ATV 1. Enable unified media pepeline on Cast for ATV. 2. Add a commend parameter to force WMPI starting in fullscreen mode so that it would use SurfaceView instead of SurfaceTexture on Android. BUG=internal b/29463340 ========== to ========== [Cast for ATV] Enable UMP on Cast for ATV 1. Enable unified media pepeline on Cast for ATV. 2. Add a commend parameter to force WMPI starting in fullscreen mode so that it would use SurfaceView instead of SurfaceTexture on Android. BUG=internal b/29463340 ==========
tsunghung@chromium.org changed reviewers: + dalecurtis@chromium.org, liberato@chromium.org, watk@chromium.org
tsunghung@chromium.org changed reviewers: + sanfin@chromium.org
kForceMediaPlayerStartingInFullscreenMode will be turned on in chromecast internal code.
On 2016/07/12 23:46:52, AndyWu wrote: > kForceMediaPlayerStartingInFullscreenMode will be turned on in chromecast > internal code. lgtm
https://codereview.chromium.org/2141293002/diff/20001/media/blink/webmediapla... File media/blink/webmediaplayer_impl.cc (right): https://codereview.chromium.org/2141293002/diff/20001/media/blink/webmediapla... media/blink/webmediaplayer_impl.cc:1334: enteredFullscreen(); what happens if blink really does enter / exit full screen later? we probably want enter/exit full screen to do nothing in those cases.
halliwell@chromium.org changed reviewers: + halliwell@chromium.org
https://codereview.chromium.org/2141293002/diff/1/build/config/features.gni File build/config/features.gni (right): https://codereview.chromium.org/2141293002/diff/1/build/config/features.gni#n... build/config/features.gni:68: enable_video_hole = false is android webview still using this? (a search for VIDEO_HOLE suggests so?) i.e. should this condition use is_chromecast? https://codereview.chromium.org/2141293002/diff/1/media/blink/webmediaplayer_... File media/blink/webmediaplayer_impl.cc (right): https://codereview.chromium.org/2141293002/diff/1/media/blink/webmediaplayer_... media/blink/webmediaplayer_impl.cc:215: switches::kForceMediaPlayerStartingInFullscreenMode); I'm concerned that what we want on ATV is not technically to be full-screen, we just want to activate the overlay codepath. It could be confusing, and could potentially lead to other unwanted behaviour if there are other side effects of fullscreen mode. Is it worth doing some renaming/refactoring so that it's clear that we're just opting in to using overlays?
liberato@chromium.org changed reviewers: + timav@chromium.org
+timav: can you comment feature.gni ? thanks -fl https://codereview.chromium.org/2141293002/diff/1/build/config/features.gni File build/config/features.gni (right): https://codereview.chromium.org/2141293002/diff/1/build/config/features.gni#n... build/config/features.gni:68: enable_video_hole = false On 2016/07/14 16:37:35, halliwell wrote: > is android webview still using this? (a search for VIDEO_HOLE suggests so?) > i.e. should this condition use is_chromecast? +timav, but i believe that the answer recently became 'no'. https://codereview.chromium.org/2141293002/diff/1/media/blink/webmediaplayer_... File media/blink/webmediaplayer_impl.cc (right): https://codereview.chromium.org/2141293002/diff/1/media/blink/webmediaplayer_... media/blink/webmediaplayer_impl.cc:215: switches::kForceMediaPlayerStartingInFullscreenMode); On 2016/07/14 16:37:35, halliwell wrote: > I'm concerned that what we want on ATV is not technically to be full-screen, we > just want to activate the overlay codepath. > > It could be confusing, and could potentially lead to other unwanted behaviour if > there are other side effects of fullscreen mode. > > Is it worth doing some renaming/refactoring so that it's clear that we're just > opting in to using overlays? +1, similar to my comment in PS2 -- we shoudln't overload enter/exitFullscreen.
timav@chromium.org changed reviewers: + boliu@chromium.org
https://codereview.chromium.org/2141293002/diff/1/build/config/features.gni File build/config/features.gni (right): https://codereview.chromium.org/2141293002/diff/1/build/config/features.gni#n... build/config/features.gni:68: enable_video_hole = false On 2016/07/14 16:53:36, liberato wrote: > On 2016/07/14 16:37:35, halliwell wrote: > > is android webview still using this? (a search for VIDEO_HOLE suggests so?) > > i.e. should this condition use is_chromecast? > > +timav, but i believe that the answer recently became 'no'. WebView does not currently use VIDEO_HOLE. I think it's ok to set it to false. Bo?
https://codereview.chromium.org/2141293002/diff/1/build/config/features.gni File build/config/features.gni (right): https://codereview.chromium.org/2141293002/diff/1/build/config/features.gni#n... build/config/features.gni:68: enable_video_hole = false On 2016/07/14 18:23:16, Tima Vaisburd wrote: > On 2016/07/14 16:53:36, liberato wrote: > > On 2016/07/14 16:37:35, halliwell wrote: > > > is android webview still using this? (a search for VIDEO_HOLE suggests so?) > > > > i.e. should this condition use is_chromecast? > > > > +timav, but i believe that the answer recently became 'no'. > > WebView does not currently use VIDEO_HOLE. I think it's ok to set it to false. > Bo? Just to be safe, I take the suggestion from halliwell. We will remove this anyway once the hole punching is deprecated. https://codereview.chromium.org/2141293002/diff/1/media/blink/webmediaplayer_... File media/blink/webmediaplayer_impl.cc (right): https://codereview.chromium.org/2141293002/diff/1/media/blink/webmediaplayer_... media/blink/webmediaplayer_impl.cc:215: switches::kForceMediaPlayerStartingInFullscreenMode); On 2016/07/14 16:53:36, liberato wrote: > On 2016/07/14 16:37:35, halliwell wrote: > > I'm concerned that what we want on ATV is not technically to be full-screen, > we > > just want to activate the overlay codepath. > > > > It could be confusing, and could potentially lead to other unwanted behaviour > if > > there are other side effects of fullscreen mode. > > > > Is it worth doing some renaming/refactoring so that it's clear that we're just > > opting in to using overlays? > > +1, similar to my comment in PS2 -- we shoudln't overload enter/exitFullscreen. Totally agree, what we need is overlay codepath, not necessary to be fullscreen. Rename some variables from "fullscreen" to "overlay". https://codereview.chromium.org/2141293002/diff/20001/media/blink/webmediapla... File media/blink/webmediaplayer_impl.cc (right): https://codereview.chromium.org/2141293002/diff/20001/media/blink/webmediapla... media/blink/webmediaplayer_impl.cc:1334: enteredFullscreen(); On 2016/07/14 16:26:58, liberato wrote: > what happens if blink really does enter / exit full screen later? > > we probably want enter/exit full screen to do nothing in those cases. Done. Doing nothing in enter/exit full screen is not going to help much if blink enables this flag by accident. Because the position/size of the video hole in graphic surface would not match that of video surface.
On 2016/07/14 19:02:09, AndyWu wrote: > https://codereview.chromium.org/2141293002/diff/1/build/config/features.gni > File build/config/features.gni (right): > > https://codereview.chromium.org/2141293002/diff/1/build/config/features.gni#n... > build/config/features.gni:68: enable_video_hole = false > On 2016/07/14 18:23:16, Tima Vaisburd wrote: > > On 2016/07/14 16:53:36, liberato wrote: > > > On 2016/07/14 16:37:35, halliwell wrote: > > > > is android webview still using this? (a search for VIDEO_HOLE suggests > so?) > > > > > > i.e. should this condition use is_chromecast? > > > > > > +timav, but i believe that the answer recently became 'no'. > > > > WebView does not currently use VIDEO_HOLE. I think it's ok to set it to false. > > Bo? > > Just to be safe, I take the suggestion from halliwell. We will remove this > anyway once the hole punching is deprecated. For webview, I'd want to wait until m52 goes to stable and make sure no one complains, before not building VIDEO_HOLE > > https://codereview.chromium.org/2141293002/diff/1/media/blink/webmediaplayer_... > File media/blink/webmediaplayer_impl.cc (right): > > https://codereview.chromium.org/2141293002/diff/1/media/blink/webmediaplayer_... > media/blink/webmediaplayer_impl.cc:215: > switches::kForceMediaPlayerStartingInFullscreenMode); > On 2016/07/14 16:53:36, liberato wrote: > > On 2016/07/14 16:37:35, halliwell wrote: > > > I'm concerned that what we want on ATV is not technically to be full-screen, > > we > > > just want to activate the overlay codepath. > > > > > > It could be confusing, and could potentially lead to other unwanted > behaviour > > if > > > there are other side effects of fullscreen mode. > > > > > > Is it worth doing some renaming/refactoring so that it's clear that we're > just > > > opting in to using overlays? > > > > +1, similar to my comment in PS2 -- we shoudln't overload > enter/exitFullscreen. > > Totally agree, what we need is overlay codepath, not necessary to be fullscreen. > Rename some variables from "fullscreen" to "overlay". > > https://codereview.chromium.org/2141293002/diff/20001/media/blink/webmediapla... > File media/blink/webmediaplayer_impl.cc (right): > > https://codereview.chromium.org/2141293002/diff/20001/media/blink/webmediapla... > media/blink/webmediaplayer_impl.cc:1334: enteredFullscreen(); > On 2016/07/14 16:26:58, liberato wrote: > > what happens if blink really does enter / exit full screen later? > > > > we probably want enter/exit full screen to do nothing in those cases. > > Done. > Doing nothing in enter/exit full screen is not going to help much if blink > enables this flag by accident. Because the position/size of the video hole in > graphic surface would not match that of video surface.
Please help to review patch set #4, thanks.
https://codereview.chromium.org/2141293002/diff/60001/media/base/media_switch... File media/base/media_switches.h (right): https://codereview.chromium.org/2141293002/diff/60001/media/base/media_switch... media/base/media_switches.h:70: MEDIA_EXPORT extern const char kForceMediaPlayerUsingOverlay[]; What about kForceVideoOverlays? https://codereview.chromium.org/2141293002/diff/60001/media/blink/webmediapla... File media/blink/webmediaplayer_impl.cc (right): https://codereview.chromium.org/2141293002/diff/60001/media/blink/webmediapla... media/blink/webmediaplayer_impl.cc:1350: EnableOverlay(); Is it possible for there to be more than one media player? Because this only works if there's only one.
https://codereview.chromium.org/2141293002/diff/60001/media/blink/webmediapla... File media/blink/webmediaplayer_impl.cc (right): https://codereview.chromium.org/2141293002/diff/60001/media/blink/webmediapla... media/blink/webmediaplayer_impl.cc:299: void WebMediaPlayerImpl::DisableOverlay() { DCHECK(overlay_enabled_) https://codereview.chromium.org/2141293002/diff/60001/media/blink/webmediapla... media/blink/webmediaplayer_impl.cc:1310: void WebMediaPlayerImpl::OnSurfaceCreated(int surface_id) { you might want to handle error cases here. what if we get kNoSurfaceId on ATV? in chrome, it's not really as big of a deal -- we'll use SurfaceTexture and video will play fine, although with higher power. we don't really know how often this fails. https://codereview.chromium.org/2141293002/diff/60001/media/blink/webmediapla... File media/blink/webmediaplayer_impl.h (right): https://codereview.chromium.org/2141293002/diff/60001/media/blink/webmediapla... media/blink/webmediaplayer_impl.h:237: void DisableOverlay(); EnableOverlay actually allocates the overlay, but DisableOverlay doesn't really do anything outside of managing the internal state. in fact, calling DisableOverlay while a codec is using the overlay might result in weird behavior -- we won't update its natural size anymore, for example. the codec might continue to use the overlay indefinitely, however. it might be better to include the restart logic in these as well. i believe that it's safe -- no restart should happen if we EnableOverlay() before the codec has requested one (i.e., CreateRenderer). https://codereview.chromium.org/2141293002/diff/60001/media/blink/webmediapla... media/blink/webmediaplayer_impl.h:520: bool use_overlay_only_; please comment. also, since it's just a cached copy of the switch, might be better to name it more similarly to the switch.
https://codereview.chromium.org/2141293002/diff/60001/media/base/media_switch... File media/base/media_switches.h (right): https://codereview.chromium.org/2141293002/diff/60001/media/base/media_switch... media/base/media_switches.h:70: MEDIA_EXPORT extern const char kForceMediaPlayerUsingOverlay[]; On 2016/07/18 20:03:02, watk wrote: > What about kForceVideoOverlays? Done, thanks. https://codereview.chromium.org/2141293002/diff/60001/media/blink/webmediapla... File media/blink/webmediaplayer_impl.cc (right): https://codereview.chromium.org/2141293002/diff/60001/media/blink/webmediapla... media/blink/webmediaplayer_impl.cc:299: void WebMediaPlayerImpl::DisableOverlay() { On 2016/07/18 20:19:58, liberato wrote: > DCHECK(overlay_enabled_) Done, thanks. https://codereview.chromium.org/2141293002/diff/60001/media/blink/webmediapla... media/blink/webmediaplayer_impl.cc:1310: void WebMediaPlayerImpl::OnSurfaceCreated(int surface_id) { On 2016/07/18 20:19:58, liberato wrote: > you might want to handle error cases here. what if we get kNoSurfaceId on ATV? > in chrome, it's not really as big of a deal -- we'll use SurfaceTexture and > video will play fine, although with higher power. > > we don't really know how often this fails. If the content is DRM protected, it will failed when initializing the video decoder. If the content is clean, it's fine to fallback to SurfaceTexture. https://codereview.chromium.org/2141293002/diff/60001/media/blink/webmediapla... media/blink/webmediaplayer_impl.cc:1350: EnableOverlay(); On 2016/07/18 20:03:02, watk wrote: > Is it possible for there to be more than one media player? Because this only > works if there's only one. In Cast for ATV, there is only one media player. Why dose this not work on multiple media players? Is there any issue other than position/size/z-order of multiple video surfaces? https://codereview.chromium.org/2141293002/diff/60001/media/blink/webmediapla... File media/blink/webmediaplayer_impl.h (right): https://codereview.chromium.org/2141293002/diff/60001/media/blink/webmediapla... media/blink/webmediaplayer_impl.h:237: void DisableOverlay(); On 2016/07/18 20:19:58, liberato wrote: > EnableOverlay actually allocates the overlay, but DisableOverlay doesn't really > do anything outside of managing the internal state. in fact, calling > DisableOverlay while a codec is using the overlay might result in weird behavior > -- we won't update its natural size anymore, for example. the codec might > continue to use the overlay indefinitely, however. > > it might be better to include the restart logic in these as well. i believe > that it's safe -- no restart should happen if we EnableOverlay() before the > codec has requested one (i.e., CreateRenderer). Done, thanks a lot. https://codereview.chromium.org/2141293002/diff/60001/media/blink/webmediapla... media/blink/webmediaplayer_impl.h:520: bool use_overlay_only_; On 2016/07/18 20:19:58, liberato wrote: > please comment. also, since it's just a cached copy of the switch, might be > better to name it more similarly to the switch. Done, thanks.
lgtm
The CQ bit was checked by tsunghung@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: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-gn on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-gn on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-gn/...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) 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 tsunghung@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: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) 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 tsunghung@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_...)
switch is fine with me for testing purposes now though, lgtm https://codereview.chromium.org/2141293002/diff/160001/media/base/media_switc... File media/base/media_switches.h (right): https://codereview.chromium.org/2141293002/diff/160001/media/base/media_switc... media/base/media_switches.h:68: MEDIA_EXPORT extern const char kForceVideoOverlays[]; Last time I did something similar the cast/content folks preferred we use a ContentRendererClient setting: https://cs.chromium.org/chromium/src/content/public/renderer/content_renderer...
The CQ bit was checked by tsunghung@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 tsunghung@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sanfin@chromium.org, liberato@chromium.org, dalecurtis@chromium.org Link to the patchset: https://codereview.chromium.org/2141293002/#ps180001 (title: "Fix unit 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: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
tsunghung@chromium.org changed reviewers: + brettw@chromium.org
Hi Brett, please help to review build/config/features.gni. Hi Luke, please help to review chromecast/browser/cast_browser_main_parts.cc.
On 2016/07/19 22:58:29, AndyWu wrote: > Hi Brett, please help to review build/config/features.gni. > Hi Luke, please help to review chromecast/browser/cast_browser_main_parts.cc. Oops, forgot this one. chromecast/ lgtm
lgtm
The CQ bit was checked by tsunghung@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== [Cast for ATV] Enable UMP on Cast for ATV 1. Enable unified media pepeline on Cast for ATV. 2. Add a commend parameter to force WMPI starting in fullscreen mode so that it would use SurfaceView instead of SurfaceTexture on Android. BUG=internal b/29463340 ========== to ========== [Cast for ATV] Enable UMP on Cast for ATV 1. Enable unified media pepeline on Cast for ATV. 2. Add a commend parameter to force WMPI starting in fullscreen mode so that it would use SurfaceView instead of SurfaceTexture on Android. BUG=internal b/29463340 ==========
Message was sent while issue was closed.
Committed patchset #10 (id:180001)
Message was sent while issue was closed.
CQ bit was unchecked.
Message was sent while issue was closed.
Description was changed from ========== [Cast for ATV] Enable UMP on Cast for ATV 1. Enable unified media pepeline on Cast for ATV. 2. Add a commend parameter to force WMPI starting in fullscreen mode so that it would use SurfaceView instead of SurfaceTexture on Android. BUG=internal b/29463340 ========== to ========== [Cast for ATV] Enable UMP on Cast for ATV 1. Enable unified media pepeline on Cast for ATV. 2. Add a commend parameter to force WMPI starting in fullscreen mode so that it would use SurfaceView instead of SurfaceTexture on Android. BUG=internal b/29463340 Committed: https://crrev.com/ee562e963c4fd7bdc4e885ee13050c97adfc9f3a Cr-Commit-Position: refs/heads/master@{#406615} ==========
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/ee562e963c4fd7bdc4e885ee13050c97adfc9f3a Cr-Commit-Position: refs/heads/master@{#406615} |