Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1043233003:
ozone: Add post message loop initialization hook (Closed)
|
Created:
5 years, 8 months ago by vignatti (out of this project) Modified:
5 years, 8 months ago CC:
chromium-reviews, ozone-reviews_chromium.org, darin-cc_chromium.org, jam, kalyank, piman+watch_chromium.org, Paweł Hajdan Jr., Sergiy Byelozyorov Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptionozone: Add post message loop initialization hook
InitializeForGpuPostMainLoop initializes the subsystems/resources necessary for
the GPU process, after the message loop is started. This is used mainly for
initialization tasks that need to take a reference to the UI thread
MessageLoopProxy. InitializeForGPU() necessarily needs to be called first.
Main motivation for such change is to fix those unit tests that needs to
initialize GL without yet message loop has been initialized. So we broke the
Ozone GPU process initialization in two now, post and pre message loop
initialized.
BUG=450919
TEST=gpu_unittests, content_unittests, gl_tests (need small changes tho, coming
next) chrome and ozone_demo on GBM platform
Patch Set 1 #
Total comments: 4
Patch Set 2 : address spang's comments #Patch Set 3 : rebase #Patch Set 4 : fix typo #Patch Set 5 : init at GLSurface::InitializeOneOffInternal #Patch Set 6 : add DCHECK #Patch Set 7 : return without failing #
Messages
Total messages: 33 (6 generated)
tiago.vignatti@intel.com changed reviewers: + dnicoara@chromium.org, spang@chromium.org
spang@, dnicoara@ PTAL. In the public API, I left the UI part out for now cause I haven't seen the usage for such. In this CL I've also included the ozone_demo and chrome (content) changes to make that work with GBM. With those changes, the other targets won't call anymore the objects that are now inside InitializeGPUPostMainLoop (DrmGpuDisplayManager, GbmDeviceGenerator, DrmGpuPlatformSupport). I guess this is fine for those targets that don't need display anything on the screen such as gl_tests, gpu_unittests, etc.
Do the tests pass now? Also, do we really need two different calls inside content/gpu? https://codereview.chromium.org/1043233003/diff/1/ui/ozone/public/ozone_platf... File ui/ozone/public/ozone_platform.cc (right): https://codereview.chromium.org/1043233003/diff/1/ui/ozone/public/ozone_platf... ui/ozone/public/ozone_platform.cc:61: << "OzonePlatform is not initialized"; DCHECK per the style guide (it says never use CHECK except in a narrow set of exceptions). Probably just DCHECK(g_platform_initialized_gpu) - doesn't seem like the other part is needed. https://codereview.chromium.org/1043233003/diff/1/ui/ozone/public/ozone_platf... File ui/ozone/public/ozone_platform.h (right): https://codereview.chromium.org/1043233003/diff/1/ui/ozone/public/ozone_platf... ui/ozone/public/ozone_platform.h:79: virtual void InitializeGPUPostMainLoop() = 0; Can you capitalize it as 'Gpu'. We could change the other one as well. There's a lot more of the 'Gpu' style than 'GPU' in the codebase and it is easier to read. I think we should standardize on that.
On 2015/03/31 20:58:49, spang wrote: > Do the tests pass now? Your CL description does a good job of saying "what" but doesn't say "why". It should probably say that some tests are initializing GL without a message loop, and that's the reason we need to do this. > > Also, do we really need two different calls inside content/gpu? > > https://codereview.chromium.org/1043233003/diff/1/ui/ozone/public/ozone_platf... > File ui/ozone/public/ozone_platform.cc (right): > > https://codereview.chromium.org/1043233003/diff/1/ui/ozone/public/ozone_platf... > ui/ozone/public/ozone_platform.cc:61: << "OzonePlatform is not initialized"; > DCHECK per the style guide (it says never use CHECK except in a narrow set of > exceptions). > > Probably just DCHECK(g_platform_initialized_gpu) - doesn't seem like the other > part is needed. > > https://codereview.chromium.org/1043233003/diff/1/ui/ozone/public/ozone_platf... > File ui/ozone/public/ozone_platform.h (right): > > https://codereview.chromium.org/1043233003/diff/1/ui/ozone/public/ozone_platf... > ui/ozone/public/ozone_platform.h:79: virtual void InitializeGPUPostMainLoop() = > 0; > Can you capitalize it as 'Gpu'. We could change the other one as well. > > There's a lot more of the 'Gpu' style than 'GPU' in the codebase and it is > easier to read. I think we should standardize on that.
On 2015/03/31 20:58:49, spang wrote: > Do the tests pass now? > > Also, do we really need two different calls inside content/gpu? we need to tackle both the single-processed and the multi-processed cases inside content/gpu. That said, I haven't found a common place where I could insert only once the InitializeGpuPostMainLoop and cover both cases. The unit tests now mostly pass and we won't need those ugly things like I suggested in CL 1040873002. Besides, I've also edited the description of the CL. PTAL spang@. Thank you!
tiago.vignatti@intel.com changed reviewers: + kbr@chromium.org
kbr@ PTAL for content/gpu https://codereview.chromium.org/1043233003/diff/1/ui/ozone/public/ozone_platf... File ui/ozone/public/ozone_platform.cc (right): https://codereview.chromium.org/1043233003/diff/1/ui/ozone/public/ozone_platf... ui/ozone/public/ozone_platform.cc:61: << "OzonePlatform is not initialized"; On 2015/03/31 20:58:49, spang wrote: > DCHECK per the style guide (it says never use CHECK except in a narrow set of > exceptions). > > Probably just DCHECK(g_platform_initialized_gpu) - doesn't seem like the other > part is needed. Done. https://codereview.chromium.org/1043233003/diff/1/ui/ozone/public/ozone_platf... File ui/ozone/public/ozone_platform.h (right): https://codereview.chromium.org/1043233003/diff/1/ui/ozone/public/ozone_platf... ui/ozone/public/ozone_platform.h:79: virtual void InitializeGPUPostMainLoop() = 0; On 2015/03/31 20:58:49, spang wrote: > Can you capitalize it as 'Gpu'. We could change the other one as well. > > There's a lot more of the 'Gpu' style than 'GPU' in the codebase and it is > easier to read. I think we should standardize on that. Done.
lgtm
tiago.vignatti@intel.com changed reviewers: + piman@chromium.org
adding piman@ for content/gpu review (who might be more aware about this changes than kbr@). PTAL.
The #ifdefs sprinkled in content/gpu are unfortunate. Could you fold this into the implementation of gfx::GLSurface::InitializeOneOff?
On Wed, Apr 1, 2015 at 2:21 PM, <kbr@chromium.org> wrote: > The #ifdefs sprinkled in content/gpu are unfortunate. Could you fold this > into > the implementation of gfx::GLSurface::InitializeOneOff? > +1, that would seem a little nicer. > > > https://codereview.chromium.org/1043233003/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2015/04/01 22:29:49, piman (Very slow to review) wrote: > On Wed, Apr 1, 2015 at 2:21 PM, <mailto:kbr@chromium.org> wrote: > > > The #ifdefs sprinkled in content/gpu are unfortunate. Could you fold this > > into > > the implementation of gfx::GLSurface::InitializeOneOff? > > > > +1, that would seem a little nicer. unfortunately we cannot because we can't guarantee that targets will call gfx::GLSurface::InitializeOneOff after the message loop is initialized. E.g gl_tests is one of this counter-examples.
On Wed, Apr 1, 2015 at 5:21 PM, <tiago.vignatti@intel.com> wrote: > On 2015/04/01 22:29:49, piman (Very slow to review) wrote: > >> On Wed, Apr 1, 2015 at 2:21 PM, <mailto:kbr@chromium.org> wrote: >> > > > The #ifdefs sprinkled in content/gpu are unfortunate. Could you fold >> this >> > into >> > the implementation of gfx::GLSurface::InitializeOneOff? >> > >> > > +1, that would seem a little nicer. >> > > unfortunately we cannot because we can't guarantee that targets will call > gfx::GLSurface::InitializeOneOff after the message loop is initialized. > E.g > gl_tests is one of this counter-examples. > Can we fix it? > > https://codereview.chromium.org/1043233003/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2015/04/02 00:25:34, piman (Very slow to review) wrote: > On Wed, Apr 1, 2015 at 5:21 PM, <mailto:tiago.vignatti@intel.com> wrote: > > > On 2015/04/01 22:29:49, piman (Very slow to review) wrote: > > > >> On Wed, Apr 1, 2015 at 2:21 PM, <mailto:kbr@chromium.org> wrote: > >> > > > > > The #ifdefs sprinkled in content/gpu are unfortunate. Could you fold > >> this > >> > into > >> > the implementation of gfx::GLSurface::InitializeOneOff? > >> > > >> > > > > +1, that would seem a little nicer. > >> > > > > unfortunately we cannot because we can't guarantee that targets will call > > gfx::GLSurface::InitializeOneOff after the message loop is initialized. > > E.g > > gl_tests is one of this counter-examples. > > > > Can we fix it? Hm, do we know why gl_tests wants to create a new message loop for each test? It would fix the problem if we created just one loop at the beginning, before InitializeOneOff().
On 2015/04/02 00:44:23, spang wrote: > On 2015/04/02 00:25:34, piman (Very slow to review) wrote: > > On Wed, Apr 1, 2015 at 5:21 PM, <mailto:tiago.vignatti@intel.com> wrote: > > > > > On 2015/04/01 22:29:49, piman (Very slow to review) wrote: > > > > > >> On Wed, Apr 1, 2015 at 2:21 PM, <mailto:kbr@chromium.org> wrote: > > >> > > > > > > > The #ifdefs sprinkled in content/gpu are unfortunate. Could you fold > > >> this > > >> > into > > >> > the implementation of gfx::GLSurface::InitializeOneOff? > > >> > > > >> > > > > > > +1, that would seem a little nicer. > > >> > > > > > > unfortunately we cannot because we can't guarantee that targets will call > > > gfx::GLSurface::InitializeOneOff after the message loop is initialized. > > > E.g > > > gl_tests is one of this counter-examples. > > > > > > > Can we fix it? > > Hm, do we know why gl_tests wants to create a new message loop for each test? It > would fix the problem if we created just one loop at the beginning, before > InitializeOneOff(). It's tied into how base's test launcher works. See crbug.com/426897 and check the logs of gl_tests_main.cc for backgroun. Maybe you could figure out a different structure that would work? +phajdan.jr and sergiyb.
On 2015/04/02 00:58:39, Ken Russell wrote: > On 2015/04/02 00:44:23, spang wrote: > > > > Hm, do we know why gl_tests wants to create a new message loop for each test? > It > > would fix the problem if we created just one loop at the beginning, before > > InitializeOneOff(). > > It's tied into how base's test launcher works. See crbug.com/426897 and check > the logs of gl_tests_main.cc for backgroun. Maybe you could figure out a > different structure that would work? +phajdan.jr and sergiyb. I've just uploaded another patch set now that calls InitializeForGpuPostMainLoop always in Ozone's GLSurface::InitializeOneOffInternal. That avoids the targets needing to call the new hook explicitly like I suggested before. PTAL guys. This solution is reasonable and the unit tests passed just fine with that. The solution is a bit error-prone though, given that we need to guarantee ourselves that the message loop is created before InitializeOneOffInternal is called. So we'll need to reorganize the order of those functions in gl_tests and gpu_unittests, just like I'm suggesting in #1025523005.
LGTM, you can add a DCHECK(base::MessageLoop::current()) to check that a message loop has indeed been created.
LGTM too. +1 on adding the DCHECK. The bots build with dcheck_always_on=1 so they should catch regressions. Hopefully there's an Ozone bot on the CQ.
The CQ bit was checked by tiago.vignatti@intel.com
The patchset sent to the CQ was uploaded after l-g-t-m from spang@chromium.org, kbr@chromium.org, piman@chromium.org Link to the patchset: https://codereview.chromium.org/1043233003/#ps100001 (title: "add DCHECK")
On 2015/04/02 19:29:26, Ken Russell wrote: > LGTM too. +1 on adding the DCHECK. The bots build with dcheck_always_on=1 so > they should catch regressions. Hopefully there's an Ozone bot on the CQ. DCHECK will spot the problems. Good idea. Thank you!
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1043233003/100001
The CQ bit was unchecked by spang@chromium.org
On 2015/04/02 19:43:04, I haz the power (commit-bot) wrote: > CQ is trying da patch. Follow status at > https://chromium-cq-status.appspot.com/patch-status/1043233003/100001 Please wait a minute. I'm confused by the latest change. InitializeForGPU() is currently called from InitializeOneOff(). For some tests, it crashes because there's no message loop. How does it help to add the 2nd initializer to InitializeOneOff()?
On 2015/04/02 20:27:14, spang wrote: > On 2015/04/02 19:43:04, I haz the power (commit-bot) wrote: > > CQ is trying da patch. Follow status at > > https://chromium-cq-status.appspot.com/patch-status/1043233003/100001 > > Please wait a minute. I'm confused by the latest change. > > InitializeForGPU() is currently called from InitializeOneOff(). > > For some tests, it crashes because there's no message loop. > > How does it help to add the 2nd initializer to InitializeOneOff()? that won't help either I think. But thanks for stopping the CQ spang@. I'm seeing now in the bots a bunch of other tests failing due the changes I'm trying to land here. To be honest, thinking better now, this solution won't fly because there's no sense in adding "fake" message loops in those targets just to see this new hook be called in. We have to think something else...
one possible idea is just to return without failing in the new hook when the message loop is not present. It's like "I'm trying to initialize a platform specific stuff that depends on the message loop but that doesn't exist yet. So if I really want it then I'll do it myself later" :) Seems fine from this point of view.
On 2015/04/02 21:05:48, vignatti wrote: > one possible idea is just to return without failing in the new hook when the > message loop is not present. It's like "I'm trying to initialize a platform > specific stuff that depends on the message loop but that doesn't exist yet. So > if I really want it then I'll do it myself later" :) Seems fine from this point > of view. That seems fragile. I'd think you'd want a strict contract for how this initializer should behave. If we need to fix how various tests are set up, let's do that.
On 2015/04/02 21:22:50, Ken Russell wrote: > On 2015/04/02 21:05:48, vignatti wrote: > > one possible idea is just to return without failing in the new hook when the > > message loop is not present. It's like "I'm trying to initialize a platform > > specific stuff that depends on the message loop but that doesn't exist yet. So > > if I really want it then I'll do it myself later" :) Seems fine from this > point > > of view. > > That seems fragile. I'd think you'd want a strict contract for how this > initializer should behave. > > If we need to fix how various tests are set up, let's do that. I'm okay with fixing all the tests, but will be tricky to convince everyone that we need a message loop just for Ozone that, well, are not actually used for most of the tests (at least at the moment)... these discussions is what will happen: https://codereview.chromium.org/1040873002/ are we fine with that?
On 2015/04/02 21:28:37, vignatti wrote: > On 2015/04/02 21:22:50, Ken Russell wrote: > > On 2015/04/02 21:05:48, vignatti wrote: > > > one possible idea is just to return without failing in the new hook when the > > > message loop is not present. It's like "I'm trying to initialize a platform > > > specific stuff that depends on the message loop but that doesn't exist yet. > So > > > if I really want it then I'll do it myself later" :) Seems fine from this > > point > > > of view. > > > > That seems fragile. I'd think you'd want a strict contract for how this > > initializer should behave. > > > > If we need to fix how various tests are set up, let's do that. > > I'm okay with fixing all the tests, but will be tricky to convince everyone that > we need a message loop just for Ozone that, well, are not actually used for most > of the tests (at least at the moment)... these discussions is what will happen: > > https://codereview.chromium.org/1040873002/ > > are we fine with that? sky@ has completely valid points that the creation of the MessageLoop that early in content_test_suite is going to cause problems for tests based on that suite which instantiate MessageLoops themselves. I don't know what to advise. There are 13 callers of gfx::GLSurface::InitializeOneOff() in the code base and your original fix will require all of them to be duplicated, which is bad from a maintenance standpoint. As you mentioned before, maybe you should figure out a way to make the initialization which has to occur after MessageLoop initialization idempotent and do it lazily. Not sure. Please give this more thought.
On 2015/04/02 21:40:55, Ken Russell wrote: > > sky@ has completely valid points that the creation of the MessageLoop that early > in content_test_suite is going to cause problems for tests based on that suite > which instantiate MessageLoops themselves. > > I don't know what to advise. There are 13 callers of > gfx::GLSurface::InitializeOneOff() in the code base and your original fix will > require all of them to be duplicated, which is bad from a maintenance > standpoint. As you mentioned before, maybe you should figure out a way to make > the initialization which has to occur after MessageLoop initialization > idempotent and do it lazily. Not sure. Please give this more thought. Hi back. Per offline discussions I had with spang@ (summarized in https://code.google.com/p/chromium/issues/detail?id=471261#c3), I'll withdraw my original motivation on adding the InitializeForGpuPostMainLoop here. I.e. we don't want this new hook for fixing the unit tests above mentioned actually. spang@, right now I'm failing to see a real use case for InitializeForGpuPostMainLoop. Do you think we should pursue this still (or the UIPostMainLoop) for any other reason? If I'm not mistake chrome target using dri platform fails for this kind of issue but I'm not sure. Any other idea for fixing up crbug.com/450919?
On 2015/04/08 21:53:33, vignatti wrote: > On 2015/04/02 21:40:55, Ken Russell wrote: > > > > sky@ has completely valid points that the creation of the MessageLoop that > early > > in content_test_suite is going to cause problems for tests based on that suite > > which instantiate MessageLoops themselves. > > > > I don't know what to advise. There are 13 callers of > > gfx::GLSurface::InitializeOneOff() in the code base and your original fix will > > require all of them to be duplicated, which is bad from a maintenance > > standpoint. As you mentioned before, maybe you should figure out a way to make > > the initialization which has to occur after MessageLoop initialization > > idempotent and do it lazily. Not sure. Please give this more thought. > > Hi back. Per offline discussions I had with spang@ (summarized in > https://code.google.com/p/chromium/issues/detail?id=471261#c3), I'll withdraw my > original motivation on adding the InitializeForGpuPostMainLoop here. I.e. we > don't want this new hook for fixing the unit tests above mentioned actually. > > spang@, right now I'm failing to see a real use case for > InitializeForGpuPostMainLoop. Do you think we should pursue this still (or the > UIPostMainLoop) for any other reason? If I'm not mistake chrome target using dri > platform fails for this kind of issue but I'm not sure. Any other idea for > fixing up crbug.com/450919? Bug 450919 was originally meant for the browser process UI thread which has different issues than you're fixing here. I would leave it.
On 2015/04/08 22:22:52, spang wrote: > On 2015/04/08 21:53:33, vignatti wrote: > > On 2015/04/02 21:40:55, Ken Russell wrote: > > > > > > sky@ has completely valid points that the creation of the MessageLoop that > > early > > > in content_test_suite is going to cause problems for tests based on that > suite > > > which instantiate MessageLoops themselves. > > > > > > I don't know what to advise. There are 13 callers of > > > gfx::GLSurface::InitializeOneOff() in the code base and your original fix > will > > > require all of them to be duplicated, which is bad from a maintenance > > > standpoint. As you mentioned before, maybe you should figure out a way to > make > > > the initialization which has to occur after MessageLoop initialization > > > idempotent and do it lazily. Not sure. Please give this more thought. > > > > Hi back. Per offline discussions I had with spang@ (summarized in > > https://code.google.com/p/chromium/issues/detail?id=471261#c3), I'll withdraw > my > > original motivation on adding the InitializeForGpuPostMainLoop here. I.e. we > > don't want this new hook for fixing the unit tests above mentioned actually. > > > > spang@, right now I'm failing to see a real use case for > > InitializeForGpuPostMainLoop. Do you think we should pursue this still (or the > > UIPostMainLoop) for any other reason? If I'm not mistake chrome target using > dri > > platform fails for this kind of issue but I'm not sure. Any other idea for > > fixing up crbug.com/450919? > > Bug 450919 was originally meant for the browser process UI thread which has > different issues than you're fixing here. > > I would leave it. ok, thanks. I'm closing this for now then. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
