Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1478753003:
[Chromecast] Support software rendering when disable-gpu used (Closed)
DescriptionCurrently, Cast ozone only works with a GPU process. In particular, this prevents certain tests from running on a headless machine (they can't use headless ozone platform either because it doesn't have the Cast overlay support).
This also means that in future, we could also consider switching Cast audio platforms to use cast ozone platform.
BUG=internal b/25871861
Committed: https://crrev.com/82dee6e429ccf6775024a2dcfa10e745518ca566
Cr-Commit-Position: refs/heads/master@{#364170}
Patch Set 1 #Patch Set 2 : Use disable-gpu to enable software canvas #
Total comments: 11
Patch Set 3 : Address review comments #
Total comments: 2
Patch Set 4 : Add comment on switch #
Messages
Total messages: 18 (6 generated)
Description was changed from ========== DO NOT SUBMIT: Work in progress on software support for cast ozone BUG= ========== to ========== Currently, Cast ozone only works with a GPU process. In particular, this prevents certain tests from running on a headless machine (they can't use headless ozone platform either because it doesn't have the Cast overlay support). This also means that in future, we could also consider switching Cast audio platforms to use cast ozone platform. BUG=internal b/25871861 ==========
halliwell@chromium.org changed reviewers: + derekjchow@chromium.org, slan@chromium.org
https://codereview.chromium.org/1478753003/diff/20001/ui/ozone/platform/cast/... File ui/ozone/platform/cast/ozone_platform_cast.cc (right): https://codereview.chromium.org/1478753003/diff/20001/ui/ozone/platform/cast/... ui/ozone/platform/cast/ozone_platform_cast.cc:85: if (base::CommandLine::ForCurrentProcess()->HasSwitch("disable-gpu")) define kDisableGpu https://codereview.chromium.org/1478753003/diff/20001/ui/ozone/platform/cast/... File ui/ozone/platform/cast/surface_factory_cast.cc (right): https://codereview.chromium.org/1478753003/diff/20001/ui/ozone/platform/cast/... ui/ozone/platform/cast/surface_factory_cast.cc:68: SurfaceFactoryCast::SurfaceFactoryCast() Can this call the constructor on line 76 and pass in nullptr for |egl_platform|?
https://codereview.chromium.org/1478753003/diff/20001/ui/ozone/platform/cast/... File ui/ozone/platform/cast/ozone_platform_cast.cc (right): https://codereview.chromium.org/1478753003/diff/20001/ui/ozone/platform/cast/... ui/ozone/platform/cast/ozone_platform_cast.cc:85: if (base::CommandLine::ForCurrentProcess()->HasSwitch("disable-gpu")) On 2015/12/08 22:03:15, derekjchow1 wrote: > define kDisableGpu +1 Although I'm afraid we might be overloading the switch. Do you intend to use the switch declared here: src/content/public/common/content_switches.h If not, we should change this string. If so, I wish we could create a dependency here, but that is almost certainly a layering violation... https://codereview.chromium.org/1478753003/diff/20001/ui/ozone/platform/cast/... File ui/ozone/platform/cast/surface_factory_cast.cc (right): https://codereview.chromium.org/1478753003/diff/20001/ui/ozone/platform/cast/... ui/ozone/platform/cast/surface_factory_cast.cc:62: private: nit: DISALLOW.... https://codereview.chromium.org/1478753003/diff/20001/ui/ozone/platform/cast/... ui/ozone/platform/cast/surface_factory_cast.cc:68: SurfaceFactoryCast::SurfaceFactoryCast() On 2015/12/08 22:03:15, derekjchow1 wrote: > Can this call the constructor on line 76 and pass in nullptr for |egl_platform|? +1, now valid in chrome to forward. https://codereview.chromium.org/1478753003/diff/20001/ui/ozone/platform/cast/... File ui/ozone/platform/cast/surface_factory_cast.h (right): https://codereview.chromium.org/1478753003/diff/20001/ui/ozone/platform/cast/... ui/ozone/platform/cast/surface_factory_cast.h:28: gfx::AcceleratedWidget w) override; nit: I see that using "w" as a param name has precedent, but can we change that? Espcially considering the base class uses "widget"
https://codereview.chromium.org/1478753003/diff/20001/ui/ozone/platform/cast/... File ui/ozone/platform/cast/ozone_platform_cast.cc (right): https://codereview.chromium.org/1478753003/diff/20001/ui/ozone/platform/cast/... ui/ozone/platform/cast/ozone_platform_cast.cc:85: if (base::CommandLine::ForCurrentProcess()->HasSwitch("disable-gpu")) On 2015/12/08 22:40:25, slan wrote: > On 2015/12/08 22:03:15, derekjchow1 wrote: > > define kDisableGpu > > +1 > > Although I'm afraid we might be overloading the switch. Do you intend to use the > switch declared here: > > src/content/public/common/content_switches.h > > If not, we should change this string. If so, I wish we could create a dependency > here, but that is almost certainly a layering violation... Indeed, I'm using the switch from content, but can't include the header :( https://codereview.chromium.org/1478753003/diff/20001/ui/ozone/platform/cast/... File ui/ozone/platform/cast/surface_factory_cast.cc (right): https://codereview.chromium.org/1478753003/diff/20001/ui/ozone/platform/cast/... ui/ozone/platform/cast/surface_factory_cast.cc:62: private: On 2015/12/08 22:40:25, slan wrote: > nit: DISALLOW.... Done. https://codereview.chromium.org/1478753003/diff/20001/ui/ozone/platform/cast/... ui/ozone/platform/cast/surface_factory_cast.cc:68: SurfaceFactoryCast::SurfaceFactoryCast() On 2015/12/08 22:40:25, slan wrote: > On 2015/12/08 22:03:15, derekjchow1 wrote: > > Can this call the constructor on line 76 and pass in nullptr for > |egl_platform|? > > +1, now valid in chrome to forward. Done. https://codereview.chromium.org/1478753003/diff/20001/ui/ozone/platform/cast/... File ui/ozone/platform/cast/surface_factory_cast.h (right): https://codereview.chromium.org/1478753003/diff/20001/ui/ozone/platform/cast/... ui/ozone/platform/cast/surface_factory_cast.h:28: gfx::AcceleratedWidget w) override; On 2015/12/08 22:40:25, slan wrote: > nit: I see that using "w" as a param name has precedent, but can we change that? > Espcially considering the base class uses "widget" Done.
On 2015/12/08 23:27:57, halliwell wrote: > https://codereview.chromium.org/1478753003/diff/20001/ui/ozone/platform/cast/... > File ui/ozone/platform/cast/ozone_platform_cast.cc (right): > > https://codereview.chromium.org/1478753003/diff/20001/ui/ozone/platform/cast/... > ui/ozone/platform/cast/ozone_platform_cast.cc:85: if > (base::CommandLine::ForCurrentProcess()->HasSwitch("disable-gpu")) > On 2015/12/08 22:40:25, slan wrote: > > On 2015/12/08 22:03:15, derekjchow1 wrote: > > > define kDisableGpu > > > > +1 > > > > Although I'm afraid we might be overloading the switch. Do you intend to use > the > > switch declared here: > > > > src/content/public/common/content_switches.h > > > > If not, we should change this string. If so, I wish we could create a > dependency > > here, but that is almost certainly a layering violation... > > Indeed, I'm using the switch from content, but can't include the header :( > > https://codereview.chromium.org/1478753003/diff/20001/ui/ozone/platform/cast/... > File ui/ozone/platform/cast/surface_factory_cast.cc (right): > > https://codereview.chromium.org/1478753003/diff/20001/ui/ozone/platform/cast/... > ui/ozone/platform/cast/surface_factory_cast.cc:62: private: > On 2015/12/08 22:40:25, slan wrote: > > nit: DISALLOW.... > > Done. > > https://codereview.chromium.org/1478753003/diff/20001/ui/ozone/platform/cast/... > ui/ozone/platform/cast/surface_factory_cast.cc:68: > SurfaceFactoryCast::SurfaceFactoryCast() > On 2015/12/08 22:40:25, slan wrote: > > On 2015/12/08 22:03:15, derekjchow1 wrote: > > > Can this call the constructor on line 76 and pass in nullptr for > > |egl_platform|? > > > > +1, now valid in chrome to forward. > > Done. > > https://codereview.chromium.org/1478753003/diff/20001/ui/ozone/platform/cast/... > File ui/ozone/platform/cast/surface_factory_cast.h (right): > > https://codereview.chromium.org/1478753003/diff/20001/ui/ozone/platform/cast/... > ui/ozone/platform/cast/surface_factory_cast.h:28: gfx::AcceleratedWidget w) > override; > On 2015/12/08 22:40:25, slan wrote: > > nit: I see that using "w" as a param name has precedent, but can we change > that? > > Espcially considering the base class uses "widget" > > Done. ping on this.
lgtm % one comment https://codereview.chromium.org/1478753003/diff/20001/ui/ozone/platform/cast/... File ui/ozone/platform/cast/ozone_platform_cast.cc (right): https://codereview.chromium.org/1478753003/diff/20001/ui/ozone/platform/cast/... ui/ozone/platform/cast/ozone_platform_cast.cc:85: if (base::CommandLine::ForCurrentProcess()->HasSwitch("disable-gpu")) On 2015/12/08 23:27:56, halliwell wrote: > On 2015/12/08 22:40:25, slan wrote: > > On 2015/12/08 22:03:15, derekjchow1 wrote: > > > define kDisableGpu > > > > +1 > > > > Although I'm afraid we might be overloading the switch. Do you intend to use > the > > switch declared here: > > > > src/content/public/common/content_switches.h > > > > If not, we should change this string. If so, I wish we could create a > dependency > > here, but that is almost certainly a layering violation... > > Indeed, I'm using the switch from content, but can't include the header :( Please redefine in a local anonymous namespace.
lgtm Just have one clarification question but you are good to submit. https://codereview.chromium.org/1478753003/diff/40001/ui/ozone/platform/cast/... File ui/ozone/platform/cast/ozone_platform_cast.cc (right): https://codereview.chromium.org/1478753003/diff/40001/ui/ozone/platform/cast/... ui/ozone/platform/cast/ozone_platform_cast.cc:88: void InitializeGPU() override { I assume that InitializeGPU won't be called when --diable-gpu is passed? Maybe we should DCHECK(!surface_factory_). Not strictly necessary though.
On 2015/12/09 21:19:34, derekjchow1 wrote: > lgtm % one comment > > https://codereview.chromium.org/1478753003/diff/20001/ui/ozone/platform/cast/... > File ui/ozone/platform/cast/ozone_platform_cast.cc (right): > > https://codereview.chromium.org/1478753003/diff/20001/ui/ozone/platform/cast/... > ui/ozone/platform/cast/ozone_platform_cast.cc:85: if > (base::CommandLine::ForCurrentProcess()->HasSwitch("disable-gpu")) > On 2015/12/08 23:27:56, halliwell wrote: > > On 2015/12/08 22:40:25, slan wrote: > > > On 2015/12/08 22:03:15, derekjchow1 wrote: > > > > define kDisableGpu > > > > > > +1 > > > > > > Although I'm afraid we might be overloading the switch. Do you intend to use > > the > > > switch declared here: > > > > > > src/content/public/common/content_switches.h > > > > > > If not, we should change this string. If so, I wish we could create a > > dependency > > > here, but that is almost certainly a layering violation... > > > > Indeed, I'm using the switch from content, but can't include the header :( > > Please redefine in a local anonymous namespace. I kinda disagree ... the point of naming constants is to have a single place of definition when you use them more than once, or to document something whose intent is unclear. In this case, the intent is arguably unclear, but giving it a constant name doesn't actually help. So I just added a comment.
https://codereview.chromium.org/1478753003/diff/40001/ui/ozone/platform/cast/... File ui/ozone/platform/cast/ozone_platform_cast.cc (right): https://codereview.chromium.org/1478753003/diff/40001/ui/ozone/platform/cast/... ui/ozone/platform/cast/ozone_platform_cast.cc:88: void InitializeGPU() override { On 2015/12/09 21:22:06, slan wrote: > I assume that InitializeGPU won't be called when --diable-gpu is passed? Maybe > we should DCHECK(!surface_factory_). Not strictly necessary though. Not necessary. InitializeUI and InitializeGPU are mutually exclusive: InitializeUI is called when using this class in browser process, InitializeGPU in GPU process. I don't like this much, wish they'd have different classes for the two cases :)
The CQ bit was checked by halliwell@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from derekjchow@chromium.org, slan@chromium.org Link to the patchset: https://codereview.chromium.org/1478753003/#ps60001 (title: "Add comment on switch")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1478753003/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1478753003/60001
Message was sent while issue was closed.
Description was changed from ========== Currently, Cast ozone only works with a GPU process. In particular, this prevents certain tests from running on a headless machine (they can't use headless ozone platform either because it doesn't have the Cast overlay support). This also means that in future, we could also consider switching Cast audio platforms to use cast ozone platform. BUG=internal b/25871861 ========== to ========== Currently, Cast ozone only works with a GPU process. In particular, this prevents certain tests from running on a headless machine (they can't use headless ozone platform either because it doesn't have the Cast overlay support). This also means that in future, we could also consider switching Cast audio platforms to use cast ozone platform. BUG=internal b/25871861 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Currently, Cast ozone only works with a GPU process. In particular, this prevents certain tests from running on a headless machine (they can't use headless ozone platform either because it doesn't have the Cast overlay support). This also means that in future, we could also consider switching Cast audio platforms to use cast ozone platform. BUG=internal b/25871861 ========== to ========== Currently, Cast ozone only works with a GPU process. In particular, this prevents certain tests from running on a headless machine (they can't use headless ozone platform either because it doesn't have the Cast overlay support). This also means that in future, we could also consider switching Cast audio platforms to use cast ozone platform. BUG=internal b/25871861 Committed: https://crrev.com/82dee6e429ccf6775024a2dcfa10e745518ca566 Cr-Commit-Position: refs/heads/master@{#364170} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/82dee6e429ccf6775024a2dcfa10e745518ca566 Cr-Commit-Position: refs/heads/master@{#364170} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
