Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
835653005:
gpu: media: RenderingHelper: wait for the display & window to be ready (Closed)
|
Created:
5 years, 11 months ago by llandwerlin-old Modified:
5 years, 10 months ago CC:
chromium-reviews, posciak+watch_chromium.org, jam, mcasas+watch_chromium.org, feature-media-reviews_chromium.org, darin-cc_chromium.org, piman+watch_chromium.org, wjia+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptiongpu: media: RenderingHelper: wait for the display & window to be ready
With the display configuration now asynchronous, we can't immediately read the display size. We need to wait until the hardware is probed.
Also the VSyncProvider cannot trigger the given callback until the PlatformWindow is properly setup with the display. We need to wait for it the actually be resized.
BUG=447798
TEST=run video_decode_accelerator_unittest on freon
Committed: https://crrev.com/7710e0b3a96c2d49cfe1146e3e4ba8c9e2405678
Cr-Commit-Position: refs/heads/master@{#313473}
Patch Set 1 #
Total comments: 14
Patch Set 2 : Update after Owen's and Pawel's comments #
Total comments: 6
Patch Set 3 : Update after dnicoara's review #
Total comments: 9
Patch Set 4 : Move platform window creation off the rendering thread #
Total comments: 5
Patch Set 5 : Initialize GPU process code in the right thread #Patch Set 6 : Use RunUntilIdle to wait for window resize #
Total comments: 6
Patch Set 7 : Update after Owen's comments #
Total comments: 2
Patch Set 8 : Pawel's nits #
Messages
Total messages: 40 (4 generated)
lionel.g.landwerlin@intel.com changed reviewers: + owenlin@chromium.org, posciak@chromium.org
Owen, Pawel, this is making the RenderingHelper wait for the display to come up with a size for its panel, and for the window to be actually resized. Without the window resized, the VSyncProvider can't find the HardwareController associated to the bounds of the window and we end up waiting forever a vsync signal.
On 2015/01/20 14:13:43, llandwerlin wrote: > Owen, Pawel, this is making the RenderingHelper wait for the display to come up > with a size for its panel, and for the window to be actually resized. > Without the window resized, the VSyncProvider can't find the HardwareController > associated to the bounds of the window and we end up waiting forever a vsync > signal. Can you put more description in this issue ? In addition, it sounds like a bug in VSyncProvider. In the case you described, why can't the vsync provider do the wait and tell us the vsync info when it is ready ?
https://codereview.chromium.org/835653005/diff/1/content/common/gpu/media/ren... File content/common/gpu/media/rendering_helper.cc (right): https://codereview.chromium.org/835653005/diff/1/content/common/gpu/media/ren... content/common/gpu/media/rendering_helper.cc:87: ui::MultipleDisplayState failed_new_state) override {} Crash the program here ? LOG(FATAL) https://codereview.chromium.org/835653005/diff/1/content/common/gpu/media/ren... content/common/gpu/media/rendering_helper.cc:255: scoped_ptr<base::RunLoop> window_loop(new base::RunLoop); Can we just use a local variable? base::RunLoop window_loop; Same for others. How about renaming it to wait_resize_loop here and in the StubOzoneDelegate to show the only purpose of the loop. https://codereview.chromium.org/835653005/diff/1/content/common/gpu/media/ren... content/common/gpu/media/rendering_helper.cc:266: scoped_ptr<base::RunLoop> display_loop(new base::RunLoop); Same. How about renaming it to wait_mode_change_loop. https://codereview.chromium.org/835653005/diff/1/content/common/gpu/media/ren... content/common/gpu/media/rendering_helper.cc:273: display_configurator_->SetDisplayMode(ui::MULTIPLE_DISPLAY_STATE_SINGLE); Are we sure the "OnDisplayModeChanged" would be called? What if the current mode is SINGLE already. https://codereview.chromium.org/835653005/diff/1/content/common/gpu/media/ren... content/common/gpu/media/rendering_helper.cc:282: // Wait for the window to be resized. Here is even more vague. Why we are sure the window will be resized ?
https://codereview.chromium.org/835653005/diff/1/content/common/gpu/media/ren... File content/common/gpu/media/rendering_helper.cc (right): https://codereview.chromium.org/835653005/diff/1/content/common/gpu/media/ren... content/common/gpu/media/rendering_helper.cc:104: void OnBoundsChanged(const gfx::Rect& new_bounds) override { loop_->Quit(); } Does this pass git cl format? https://codereview.chromium.org/835653005/diff/1/content/common/gpu/media/ren... content/common/gpu/media/rendering_helper.cc:258: new RenderingHelper::StubOzoneDelegate(window_loop.get())); Is it safe to pass a local variable pointer to the class member here? https://codereview.chromium.org/835653005/diff/1/content/common/gpu/media/ren... content/common/gpu/media/rendering_helper.cc:266: scoped_ptr<base::RunLoop> display_loop(new base::RunLoop); (new base::RunLoop());
On 2015/01/21 08:09:51, Owen Lin wrote: > On 2015/01/20 14:13:43, llandwerlin wrote: > > Owen, Pawel, this is making the RenderingHelper wait for the display to come > up > > with a size for its panel, and for the window to be actually resized. > > Without the window resized, the VSyncProvider can't find the > HardwareController > > associated to the bounds of the window and we end up waiting forever a vsync > > signal. > > Can you put more description in this issue ? > > In addition, it sounds like a bug in VSyncProvider. In the case you described, > why can't the vsync provider do the wait and tell us the vsync info when it is > ready ? That's a good question. As far as I can tell, other implementations of the VSyncProvider will also drop the callback if something goes wrong.
On 2015/01/21 08:21:36, llandwerlin wrote: > On 2015/01/21 08:09:51, Owen Lin wrote: > > On 2015/01/20 14:13:43, llandwerlin wrote: > > > Owen, Pawel, this is making the RenderingHelper wait for the display to come > > up > > > with a size for its panel, and for the window to be actually resized. > > > Without the window resized, the VSyncProvider can't find the > > HardwareController > > > associated to the bounds of the window and we end up waiting forever a vsync > > > signal. > > > > Can you put more description in this issue ? > > > > In addition, it sounds like a bug in VSyncProvider. In the case you described, > > why can't the vsync provider do the wait and tell us the vsync info when it is > > ready ? > > That's a good question. As far as I can tell, other implementations of the > VSyncProvider will also drop the callback if something goes wrong. I guess I could save the callback the last callback like this : https://code.google.com/p/chromium/codesearch#chromium/src/ui/gl/gl_surface_g... That would probably remove the need for waiting the window to be resized.
https://codereview.chromium.org/835653005/diff/1/content/common/gpu/media/ren... File content/common/gpu/media/rendering_helper.cc (right): https://codereview.chromium.org/835653005/diff/1/content/common/gpu/media/ren... content/common/gpu/media/rendering_helper.cc:87: ui::MultipleDisplayState failed_new_state) override {} On 2015/01/21 08:10:03, Owen Lin wrote: > Crash the program here ? > > LOG(FATAL) Acknowledged. https://codereview.chromium.org/835653005/diff/1/content/common/gpu/media/ren... content/common/gpu/media/rendering_helper.cc:104: void OnBoundsChanged(const gfx::Rect& new_bounds) override { loop_->Quit(); } On 2015/01/21 08:13:35, Pawel Osciak wrote: > Does this pass git cl format? Yes. https://codereview.chromium.org/835653005/diff/1/content/common/gpu/media/ren... content/common/gpu/media/rendering_helper.cc:255: scoped_ptr<base::RunLoop> window_loop(new base::RunLoop); On 2015/01/21 08:10:03, Owen Lin wrote: > Can we just use a local variable? > base::RunLoop window_loop; > > Same for others. > > How about renaming it to wait_resize_loop here and in the StubOzoneDelegate to > show the only purpose of the loop. Acknowledged. https://codereview.chromium.org/835653005/diff/1/content/common/gpu/media/ren... content/common/gpu/media/rendering_helper.cc:258: new RenderingHelper::StubOzoneDelegate(window_loop.get())); On 2015/01/21 08:13:35, Pawel Osciak wrote: > Is it safe to pass a local variable pointer to the class member here? Thanks, I should reset the class member after the loop is stop. https://codereview.chromium.org/835653005/diff/1/content/common/gpu/media/ren... content/common/gpu/media/rendering_helper.cc:273: display_configurator_->SetDisplayMode(ui::MULTIPLE_DISPLAY_STATE_SINGLE); On 2015/01/21 08:10:03, Owen Lin wrote: > Are we sure the "OnDisplayModeChanged" would be called? What if the current mode > is SINGLE already. As far as I can tell the ForceInitialConfigure will always notify the observers : https://code.google.com/p/chromium/codesearch#chromium/src/ui/display/chromeo... https://codereview.chromium.org/835653005/diff/1/content/common/gpu/media/ren... content/common/gpu/media/rendering_helper.cc:282: // Wait for the window to be resized. On 2015/01/21 08:10:03, Owen Lin wrote: > Here is even more vague. Why we are sure the window will be resized ? We initially create the window with a gfx::Rect() -> 0x0 : https://code.google.com/p/chromium/codesearch#chromium/src/content/common/gpu... After calling SetBounds(), will should be notified via PlatformWindowDelegate::OnBoundsChanged().
dnicoara@chromium.org changed reviewers: + dnicoara@chromium.org
I tested this on a Release and Debug build and everything worked fine. I'm still running into following DCHECKs in Debug builds in dri_cursor.cc. I'm wondering if that's unrelated to this. I'll followup with spang@ on the DCHECK. [0121/081122:FATAL:dri_cursor.cc(193)] Check failed: ui_task_runner_->BelongsToCurrentThread(). https://codereview.chromium.org/835653005/diff/20001/content/common/gpu/media... File content/common/gpu/media/rendering_helper.cc (right): https://codereview.chromium.org/835653005/diff/20001/content/common/gpu/media... content/common/gpu/media/rendering_helper.cc:279: display_configurator_->AddObserver(&display_observer); You can remove all the nested message loops and the observer in favor of the following: display_configurator_.reset(new ui::DisplayConfigurator()); display_configurator_->Init(true); // This needs to be called right after Init(). display_configurator_->ForceInitialConfigure(0); // Make sure all the display configuration is applied. base::RunLoop().RunUntilIdle(); display_configurator_->SetDisplayMode(ui::MULTIPLE_DISPLAY_STATE_SINGLE); // Make sure all the display configuration is applied. base::RunLoop().RunUntilIdle(); https://codereview.chromium.org/835653005/diff/20001/content/common/gpu/media... content/common/gpu/media/rendering_helper.cc:292: wait_resize_loop.Run(); This also needs to be replaced by a "base::RunLoop().RunUntilIdle()"
https://codereview.chromium.org/835653005/diff/20001/content/common/gpu/media... File content/common/gpu/media/rendering_helper.cc (right): https://codereview.chromium.org/835653005/diff/20001/content/common/gpu/media... content/common/gpu/media/rendering_helper.cc:279: display_configurator_->AddObserver(&display_observer); On 2015/01/21 16:23:57, dnicoara wrote: > You can remove all the nested message loops and the observer in favor of the > following: > > display_configurator_.reset(new ui::DisplayConfigurator()); > display_configurator_->Init(true); > > // This needs to be called right after Init(). > display_configurator_->ForceInitialConfigure(0); > // Make sure all the display configuration is applied. > base::RunLoop().RunUntilIdle(); > > display_configurator_->SetDisplayMode(ui::MULTIPLE_DISPLAY_STATE_SINGLE); > // Make sure all the display configuration is applied. > base::RunLoop().RunUntilIdle(); Done. https://codereview.chromium.org/835653005/diff/20001/content/common/gpu/media... content/common/gpu/media/rendering_helper.cc:292: wait_resize_loop.Run(); On 2015/01/21 16:23:57, dnicoara wrote: > This also needs to be replaced by a "base::RunLoop().RunUntilIdle()" Done.
https://codereview.chromium.org/835653005/diff/40001/content/common/gpu/media... File content/common/gpu/media/rendering_helper.cc (right): https://codereview.chromium.org/835653005/diff/40001/content/common/gpu/media... content/common/gpu/media/rendering_helper.cc:233: #elif defined(USE_OZONE) Spoke with spang@ and now I understand what's going on. - OzonePlatform is initialized on the main message loop declared in "int main()" in the unittests. - RenderingHelper::Initialize is posted on the rendering helper thread. This is an issue since Ozone expects all calls into the platform to be performed from the same thread. I'm guessing that the window creation (and in Ozone's case, display configuration) need to be performed separately on the main thread. Then pass |window_| as a parameter to RenderingHelper::Initialize(). https://codereview.chromium.org/835653005/diff/40001/content/common/gpu/media... content/common/gpu/media/rendering_helper.cc:240: base::MessageLoop::ScopedNestableTaskAllower nest_loop( You shouldn't need this anymore.
https://codereview.chromium.org/835653005/diff/40001/content/common/gpu/media... File content/common/gpu/media/rendering_helper.cc (right): https://codereview.chromium.org/835653005/diff/40001/content/common/gpu/media... content/common/gpu/media/rendering_helper.cc:233: #elif defined(USE_OZONE) On 2015/01/21 19:21:35, dnicoara wrote: > Spoke with spang@ and now I understand what's going on. > > - OzonePlatform is initialized on the main message loop declared in "int > main()" in the unittests. > - RenderingHelper::Initialize is posted on the rendering helper thread. > > This is an issue since Ozone expects all calls into the platform to be performed > from the same thread. I'm guessing that the window creation (and in Ozone's > case, display configuration) need to be performed separately on the main thread. > Then pass |window_| as a parameter to RenderingHelper::Initialize(). In addition, maybe we can move some code to InitilizeOneOff? That's in the main thread. And I think we need to do the display configuration only once. https://codereview.chromium.org/835653005/diff/40001/content/common/gpu/media... content/common/gpu/media/rendering_helper.cc:246: base::RunLoop().RunUntilIdle(); The correctness of using "RunUntilIdle()" here is highly depending on the implementation inside display_configurator. If there is another thread involved, it becomes flaky. I think we should avoid using it.
https://codereview.chromium.org/835653005/diff/40001/content/common/gpu/media... File content/common/gpu/media/rendering_helper.cc (right): https://codereview.chromium.org/835653005/diff/40001/content/common/gpu/media... content/common/gpu/media/rendering_helper.cc:233: #elif defined(USE_OZONE) On 2015/01/22 03:46:44, Owen Lin wrote: > On 2015/01/21 19:21:35, dnicoara wrote: > > Spoke with spang@ and now I understand what's going on. > > > > - OzonePlatform is initialized on the main message loop declared in "int > > main()" in the unittests. > > - RenderingHelper::Initialize is posted on the rendering helper thread. > > > > This is an issue since Ozone expects all calls into the platform to be > performed > > from the same thread. I'm guessing that the window creation (and in Ozone's > > case, display configuration) need to be performed separately on the main > thread. > > Then pass |window_| as a parameter to RenderingHelper::Initialize(). > > In addition, maybe we can move some code to InitilizeOneOff? That's in the main > thread. And I think we need to do the display configuration only once. Doing the window creation in InitializeOnOff means we're going to keep the window as a global variable. Maybe we can split the Initialize method in 2, with one part executed on the main thread (window creation), one part on the rendering thread (gl context setup). https://codereview.chromium.org/835653005/diff/40001/content/common/gpu/media... content/common/gpu/media/rendering_helper.cc:233: #elif defined(USE_OZONE) On 2015/01/21 19:21:35, dnicoara wrote: > Spoke with spang@ and now I understand what's going on. > > - OzonePlatform is initialized on the main message loop declared in "int > main()" in the unittests. > - RenderingHelper::Initialize is posted on the rendering helper thread. > > This is an issue since Ozone expects all calls into the platform to be performed > from the same thread. I'm guessing that the window creation (and in Ozone's > case, display configuration) need to be performed separately on the main thread. > Then pass |window_| as a parameter to RenderingHelper::Initialize(). Thanks for the explanation. https://codereview.chromium.org/835653005/diff/40001/content/common/gpu/media... content/common/gpu/media/rendering_helper.cc:240: base::MessageLoop::ScopedNestableTaskAllower nest_loop( On 2015/01/21 19:21:35, dnicoara wrote: > You shouldn't need this anymore. Acknowledged. https://codereview.chromium.org/835653005/diff/40001/content/common/gpu/media... content/common/gpu/media/rendering_helper.cc:246: base::RunLoop().RunUntilIdle(); On 2015/01/22 03:46:44, Owen Lin wrote: > The correctness of using "RunUntilIdle()" here is highly depending on the > implementation inside display_configurator. If there is another thread involved, > it becomes flaky. > I think we should avoid using it. I agree, posting a message to a different thread and waiting for the response might leave the message loop empty.
PTAL. This change affects the Windows platform too, if someone could trigger the trybots there, Thanks!
https://codereview.chromium.org/835653005/diff/60001/content/common/gpu/media... File content/common/gpu/media/rendering_helper.cc (right): https://codereview.chromium.org/835653005/diff/60001/content/common/gpu/media... content/common/gpu/media/rendering_helper.cc:271: wait_window_resize.Run(); I'm not fine with adding more complexity to platform implementations. This can be replaced with base::RunLoop().RunUntilIdle() to flush all the pending updates without adding extra complexity to the platform code. https://codereview.chromium.org/835653005/diff/60001/content/common/gpu/media... content/common/gpu/media/rendering_helper.cc:318: gl_surface_ = gfx::GLSurface::CreateViewGLSurface(window_); While reading more of this code I realized this is also broken. GLSurface::InitializeOneOff() is called from the main thread while this is called from the renderer_helper thread. This violates threading assumptions in Ozone (and X11). Though, given that this review is fixing a M41 beta blocker bug, perhaps just file a bug against this to have it fixed later.
ihf@chromium.org changed reviewers: + marcheu@chromium.org
https://codereview.chromium.org/835653005/diff/20001/content/common/gpu/media... File content/common/gpu/media/rendering_helper.cc (right): https://codereview.chromium.org/835653005/diff/20001/content/common/gpu/media... content/common/gpu/media/rendering_helper.cc:281: display_configurator_->SetDisplayMode(ui::MULTIPLE_DISPLAY_STATE_SINGLE); Why this line is removed ? https://codereview.chromium.org/835653005/diff/40001/content/common/gpu/media... File content/common/gpu/media/rendering_helper.cc (right): https://codereview.chromium.org/835653005/diff/40001/content/common/gpu/media... content/common/gpu/media/rendering_helper.cc:233: #elif defined(USE_OZONE) On 2015/01/22 11:03:18, llandwerlin wrote: > On 2015/01/22 03:46:44, Owen Lin wrote: > > On 2015/01/21 19:21:35, dnicoara wrote: > > > Spoke with spang@ and now I understand what's going on. > > > > > > - OzonePlatform is initialized on the main message loop declared in "int > > > main()" in the unittests. > > > - RenderingHelper::Initialize is posted on the rendering helper thread. > > > > > > This is an issue since Ozone expects all calls into the platform to be > > performed > > > from the same thread. I'm guessing that the window creation (and in Ozone's > > > case, display configuration) need to be performed separately on the main > > thread. > > > Then pass |window_| as a parameter to RenderingHelper::Initialize(). > > > > In addition, maybe we can move some code to InitilizeOneOff? That's in the > main > > thread. And I think we need to do the display configuration only once. > > Doing the window creation in InitializeOnOff means we're going to keep the > window as a global variable. > > Maybe we can split the Initialize method in 2, with one part executed on the > main thread (window creation), one part on the rendering thread (gl context > setup). Good idea. https://codereview.chromium.org/835653005/diff/60001/content/common/gpu/media... File content/common/gpu/media/rendering_helper.cc (right): https://codereview.chromium.org/835653005/diff/60001/content/common/gpu/media... content/common/gpu/media/rendering_helper.cc:271: wait_window_resize.Run(); On 2015/01/22 19:23:17, dnicoara wrote: > I'm not fine with adding more complexity to platform implementations. This can > be replaced with base::RunLoop().RunUntilIdle() to flush all the pending updates > without adding extra complexity to the platform code. I think you're talking the other CL (859943004). But correctness should be considered before simplicity. Anyway, to prevent the dilemma, how about just signaling done in the end of the function. So we don't need to wait for VSYNC info here. The current rendering_helper has been modified to live without vsync provider. We will try to get the VSync info when rendering a frame later. The drawback is we will never know whether we get the VSYNC or not. I hope we can get a performance alert in that case.
PTAL. I moved all the Gpu initialization code into the rendering thread. I now have to keep the rendering thread alive for the duration of the entire test suite, otherwise we run into problems at the initialization of vaapi (the thread that opened the drm fd exits and for some reason some code in libva assumes we're not identified anymore). dnicoara@ thanks for explaining the expectation for what thread should access what API, that's very helpful. Again, running the bots for the Windows platform would be useful.
https://codereview.chromium.org/835653005/diff/20001/content/common/gpu/media... File content/common/gpu/media/rendering_helper.cc (right): https://codereview.chromium.org/835653005/diff/20001/content/common/gpu/media... content/common/gpu/media/rendering_helper.cc:281: display_configurator_->SetDisplayMode(ui::MULTIPLE_DISPLAY_STATE_SINGLE); On 2015/01/23 08:57:38, Owen Lin wrote: > Why this line is removed ? You don't need to set the display mode since ForceInitialConfigure() will do it. The subtle detail is that if you have an external display attached, setting the mode like this will fail. You'd need to also set the power to turn the external off in order to be able to set the state to SINGLE. The easiest way would probably be to just use the state set in ForceInitialConfigure() and use DisplayConfigurator::cached_displays() to get the first display and use it's bounds when initializing the window. https://codereview.chromium.org/835653005/diff/60001/content/common/gpu/media... File content/common/gpu/media/rendering_helper.cc (right): https://codereview.chromium.org/835653005/diff/60001/content/common/gpu/media... content/common/gpu/media/rendering_helper.cc:271: wait_window_resize.Run(); On 2015/01/23 08:57:38, Owen Lin wrote: > On 2015/01/22 19:23:17, dnicoara wrote: > > I'm not fine with adding more complexity to platform implementations. This can > > be replaced with base::RunLoop().RunUntilIdle() to flush all the pending > updates > > without adding extra complexity to the platform code. > > I think you're talking the other CL (859943004). But correctness should be > considered before simplicity. > > Anyway, to prevent the dilemma, how about just signaling done in the end of the > function. So we don't need to wait for VSYNC info here. The current > rendering_helper has been modified to live without vsync provider. We will try > to get the VSync info when rendering a frame later. > > The drawback is we will never know whether we get the VSYNC or not. I hope we > can get a performance alert in that case. The current approach is correct. Platforms have 2 choices: 1) If they use their own message loop (or connection) they need to provide their own flushing primitives and those would need to be explicitly called from functions such as SetBounds(). 2) If they use the built-in message loop, then they have to rely on running the message loop to achieve the same effect. Think of base::RunLoop().RunUntilIdle() as a flush. Note, "wait_window_resize.Run()" essentially does the same thing. Let me know if I should expand, I'm happy to provide more details.
dnicoara@ : is it normal that the tests don't appear to be run on these trybots?
On 2015/01/23 15:55:06, llandwerlin wrote: > dnicoara@ : is it normal that the tests don't appear to be run on these trybots? I know they've been optimizing the try bots. The idea was to skip running tests on unaffected bots. I used the default list of bots, but maybe these tests aren't in that list. Do you know of a specific bot that should run these tests?
On 2015/01/23 15:58:25, dnicoara wrote: > On 2015/01/23 15:55:06, llandwerlin wrote: > > dnicoara@ : is it normal that the tests don't appear to be run on these > trybots? > > I know they've been optimizing the try bots. The idea was to skip running tests > on unaffected bots. I used the default list of bots, but maybe these tests > aren't in that list. Do you know of a specific bot that should run these tests? I would have expected win8_chromium_rel to at least run the video_decode_accelerator_unittest since it has recompiled it. But I'm not that knowledgeable about the infrastructure... linux_chromium_asan_rel seems to be running the tests.
On 2015/01/23 15:08:35, llandwerlin wrote: > PTAL. > > I moved all the Gpu initialization code into the rendering thread. > I now have to keep the rendering thread alive for the duration of the entire > test suite, otherwise we run into problems at the initialization of vaapi (the > thread that opened the drm fd exits and for some reason some code in libva > assumes we're not identified anymore). > > dnicoara@ thanks for explaining the expectation for what thread should access > what API, that's very helpful. > > Again, running the bots for the Windows platform would be useful. Thanks, I've taken a quick look and it looks fine modulo the waiting for the window resize. I've also manually started a few Win bots to try getting the tests running on those. Hopefully they'll run this time.
On 2015/01/23 16:25:35, dnicoara wrote: > On 2015/01/23 15:08:35, llandwerlin wrote: > > PTAL. > > > > I moved all the Gpu initialization code into the rendering thread. > > I now have to keep the rendering thread alive for the duration of the entire > > test suite, otherwise we run into problems at the initialization of vaapi (the > > thread that opened the drm fd exits and for some reason some code in libva > > assumes we're not identified anymore). > > > > dnicoara@ thanks for explaining the expectation for what thread should access > > what API, that's very helpful. > > > > Again, running the bots for the Windows platform would be useful. > > Thanks, I've taken a quick look and it looks fine modulo the waiting for the > window resize. Should replace only this one with RunUntilIdle() ? And leave the display ones with Run() ? > > I've also manually started a few Win bots to try getting the tests running on > those. Hopefully they'll run this time.
On 2015/01/23 16:42:02, llandwerlin wrote: > On 2015/01/23 16:25:35, dnicoara wrote: > > On 2015/01/23 15:08:35, llandwerlin wrote: > > > PTAL. > > > > > > I moved all the Gpu initialization code into the rendering thread. > > > I now have to keep the rendering thread alive for the duration of the entire > > > test suite, otherwise we run into problems at the initialization of vaapi > (the > > > thread that opened the drm fd exits and for some reason some code in libva > > > assumes we're not identified anymore). > > > > > > dnicoara@ thanks for explaining the expectation for what thread should > access > > > what API, that's very helpful. > > > > > > Again, running the bots for the Windows platform would be useful. > > > > Thanks, I've taken a quick look and it looks fine modulo the waiting for the > > window resize. > > Should replace only this one with RunUntilIdle() ? > And leave the display ones with Run() ? > You should replace "wait_window_resize.Run();" with RunUntilIdle(). For the display ones, you can leave it as is, but you'll need to handle the synchronous case too. For example, if DisplayConfigurator::ForceInitialConfigure() is executed synchronously then running the nested message loop will never return. > > > > I've also manually started a few Win bots to try getting the tests running on > > those. Hopefully they'll run this time. Looks like none of the bots actually run the tests, they only compile them.
On 2015/01/23 17:02:15, dnicoara wrote: > On 2015/01/23 16:42:02, llandwerlin wrote: > > On 2015/01/23 16:25:35, dnicoara wrote: > > > On 2015/01/23 15:08:35, llandwerlin wrote: > > > > PTAL. > > > > > > > > I moved all the Gpu initialization code into the rendering thread. > > > > I now have to keep the rendering thread alive for the duration of the > entire > > > > test suite, otherwise we run into problems at the initialization of vaapi > > (the > > > > thread that opened the drm fd exits and for some reason some code in libva > > > > assumes we're not identified anymore). > > > > > > > > dnicoara@ thanks for explaining the expectation for what thread should > > access > > > > what API, that's very helpful. > > > > > > > > Again, running the bots for the Windows platform would be useful. > > > > > > Thanks, I've taken a quick look and it looks fine modulo the waiting for the > > > window resize. > > > > Should replace only this one with RunUntilIdle() ? > > And leave the display ones with Run() ? > > > > You should replace "wait_window_resize.Run();" with RunUntilIdle(). > > For the display ones, you can leave it as is, but you'll need to handle the > synchronous case too. For example, if > DisplayConfigurator::ForceInitialConfigure() is executed synchronously then > running the nested message loop will never return. > Reading the RunLoop documentation and everything should be fine if Quit() is called before Run(). Ignore the statement related to the display run loops. > > > > > > I've also manually started a few Win bots to try getting the tests running > on > > > those. Hopefully they'll run this time. > > Looks like none of the bots actually run the tests, they only compile them.
PTAL.
On 2015/01/23 17:43:15, llandwerlin wrote: > PTAL. lgtm on the Ozone side nit: You can drop the DEPS change to simplify the review. (Up to you)
On 2015/01/23 17:49:11, dnicoara wrote: > On 2015/01/23 17:43:15, llandwerlin wrote: > > PTAL. > > lgtm on the Ozone side > > nit: You can drop the DEPS change to simplify the review. (Up to you) Thanks, Though I would need to remove the #include "ui/display/types/display_constants.h" display_configurator_->SetDisplayMode(ui::MULTIPLE_DISPLAY_STATE_SINGLE); otherwise git cl presubmit complains about it.
On 2015/01/23 17:51:06, llandwerlin wrote: > On 2015/01/23 17:49:11, dnicoara wrote: > > On 2015/01/23 17:43:15, llandwerlin wrote: > > > PTAL. > > > > lgtm on the Ozone side > > > > nit: You can drop the DEPS change to simplify the review. (Up to you) > > Thanks, > > Though I would need to remove the > > #include "ui/display/types/display_constants.h" > > display_configurator_->SetDisplayMode(ui::MULTIPLE_DISPLAY_STATE_SINGLE); > > otherwise git cl presubmit complains about it. Oh, see my previous explanation in comment #20. You may be able to simplify that part.
https://codereview.chromium.org/835653005/diff/60001/content/common/gpu/media... File content/common/gpu/media/rendering_helper.cc (right): https://codereview.chromium.org/835653005/diff/60001/content/common/gpu/media... content/common/gpu/media/rendering_helper.cc:271: wait_window_resize.Run(); On 2015/01/23 15:10:13, dnicoara wrote: > On 2015/01/23 08:57:38, Owen Lin wrote: > > On 2015/01/22 19:23:17, dnicoara wrote: > > > I'm not fine with adding more complexity to platform implementations. This > can > > > be replaced with base::RunLoop().RunUntilIdle() to flush all the pending > > updates > > > without adding extra complexity to the platform code. > > > > I think you're talking the other CL (859943004). But correctness should be > > considered before simplicity. > > > > Anyway, to prevent the dilemma, how about just signaling done in the end of > the > > function. So we don't need to wait for VSYNC info here. The current > > rendering_helper has been modified to live without vsync provider. We will try > > to get the VSync info when rendering a frame later. > > > > The drawback is we will never know whether we get the VSYNC or not. I hope we > > can get a performance alert in that case. > > The current approach is correct. Platforms have 2 choices: > 1) If they use their own message loop (or connection) they need to provide > their own flushing primitives and those would need to be explicitly called from > functions such as SetBounds(). > 2) If they use the built-in message loop, then they have to rely on running the > message loop to achieve the same effect. Think of base::RunLoop().RunUntilIdle() > as a flush. > > Note, "wait_window_resize.Run()" essentially does the same thing. > > Let me know if I should expand, I'm happy to provide more details. Thanks for the explanation. I am kind of understanding it. It makes sure all these setup commands have been flushed to the hardware (or something) before the vsync command. I think the basic issue is we don't understand what we need to make VSyncProvider works. https://codereview.chromium.org/835653005/diff/100001/content/common/gpu/medi... File content/common/gpu/media/rendering_helper.cc (right): https://codereview.chromium.org/835653005/diff/100001/content/common/gpu/medi... content/common/gpu/media/rendering_helper.cc:259: base::RunLoop wait_display_single; Are you going to removing this part ? https://codereview.chromium.org/835653005/diff/100001/content/common/gpu/medi... content/common/gpu/media/rendering_helper.cc:271: wait_window_resize.RunUntilIdle(); Please add comment to explain what this line is about. And the reason why we need it. https://codereview.chromium.org/835653005/diff/100001/content/common/gpu/medi... File content/common/gpu/media/video_decode_accelerator_unittest.cc (right): https://codereview.chromium.org/835653005/diff/100001/content/common/gpu/medi... content/common/gpu/media/video_decode_accelerator_unittest.cc:234: return rendering_loop_proxy_; considering the message_loop_proxy() has been deprecated. How about 1. renaming this function to rendering_task_runner 2. removing the variable "rendering_loop_proxy_" 3. directly returning rendering_thread_.task_runner()
https://codereview.chromium.org/835653005/diff/100001/content/common/gpu/medi... File content/common/gpu/media/rendering_helper.cc (right): https://codereview.chromium.org/835653005/diff/100001/content/common/gpu/medi... content/common/gpu/media/rendering_helper.cc:259: base::RunLoop wait_display_single; On 2015/01/26 07:25:29, Owen Lin wrote: > Are you going to removing this part ? Done. https://codereview.chromium.org/835653005/diff/100001/content/common/gpu/medi... content/common/gpu/media/rendering_helper.cc:271: wait_window_resize.RunUntilIdle(); On 2015/01/26 07:25:29, Owen Lin wrote: > Please add comment to explain what this line is about. And the reason why we > need it. Done. https://codereview.chromium.org/835653005/diff/100001/content/common/gpu/medi... File content/common/gpu/media/video_decode_accelerator_unittest.cc (right): https://codereview.chromium.org/835653005/diff/100001/content/common/gpu/medi... content/common/gpu/media/video_decode_accelerator_unittest.cc:234: return rendering_loop_proxy_; On 2015/01/26 07:25:29, Owen Lin wrote: > considering the message_loop_proxy() has been deprecated. > How about > 1. renaming this function to rendering_task_runner > 2. removing the variable "rendering_loop_proxy_" > 3. directly returning rendering_thread_.task_runner() Done, with slight change to 1. because coding style.
lgtm. Hi Pawel, please do a owner's review of this CL. Thanks.
lgtm % nit Thank you for great explanations in comments! https://codereview.chromium.org/835653005/diff/120001/content/common/gpu/medi... File content/common/gpu/media/video_decode_accelerator_unittest.cc (right): https://codereview.chromium.org/835653005/diff/120001/content/common/gpu/medi... content/common/gpu/media/video_decode_accelerator_unittest.cc:221: virtual void SetUp() { virtual -> override in this class please.
https://codereview.chromium.org/835653005/diff/120001/content/common/gpu/medi... File content/common/gpu/media/video_decode_accelerator_unittest.cc (right): https://codereview.chromium.org/835653005/diff/120001/content/common/gpu/medi... content/common/gpu/media/video_decode_accelerator_unittest.cc:221: virtual void SetUp() { On 2015/01/28 07:41:47, Pawel Osciak wrote: > virtual -> override in this class please. Done.
The CQ bit was checked by lionel.g.landwerlin@intel.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/835653005/140001
Message was sent while issue was closed.
Committed patchset #8 (id:140001)
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/7710e0b3a96c2d49cfe1146e3e4ba8c9e2405678 Cr-Commit-Position: refs/heads/master@{#313473} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
