Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1287043002:
cc: Setup API to release OutputSurface from LTHClient. (Closed)
|
Created:
5 years, 4 months ago by sohanjg Modified:
5 years, 3 months ago CC:
chromium-reviews, mlamouri+watch-content_chromium.org, jam, darin-cc_chromium.org, mkwst+moarreviews-renderer_chromium.org, piman+watch_chromium.org, cc-bugs_chromium.org, sohanjg_ Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionThis sets up API to release OutputSurface from LTHClient.
It will be helpful in Android where we can release OutputSurface without
destroying LTHClient, thereby making OutputSurface switches have less glitches.
BUG=374906
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
Committed: https://crrev.com/3a33d87b15b764a6db1cd863ce54efec2bcd2b23
Cr-Commit-Position: refs/heads/master@{#349787}
Patch Set 1 #
Total comments: 1
Patch Set 2 : unit test #
Total comments: 30
Patch Set 3 : review comments addressed. #
Total comments: 11
Patch Set 4 : review comments addressed. #
Total comments: 4
Patch Set 5 : tests updated. #
Total comments: 4
Patch Set 6 : update test. #
Total comments: 7
Dependent Patchsets: Messages
Total messages: 42 (12 generated)
sohan.jyoti@samsung.com changed reviewers: + sievers@chromium.org
On 2015/08/12 10:47:51, sohanjg wrote: > mailto:sohan.jyoti@samsung.com changed reviewers: > + mailto:sievers@chromium.org This is pretty rough, can you please take a look, if this is what we want ? thanks.
https://codereview.chromium.org/1287043002/diff/1/content/renderer/gpu/render... File content/renderer/gpu/render_widget_compositor.cc (right): https://codereview.chromium.org/1287043002/diff/1/content/renderer/gpu/render... content/renderer/gpu/render_widget_compositor.cc:960: did_give_back_output_surface_ = true; i have added this member to reflect the compositor's state. But couldnt find any uasge.
sievers@chromium.org changed reviewers: + brianderson@chromium.org, enne@chromium.org, sunnyps@chromium.org
On 2015/08/12 10:50:29, sohanjg wrote: > https://codereview.chromium.org/1287043002/diff/1/content/renderer/gpu/render... > File content/renderer/gpu/render_widget_compositor.cc (right): > > https://codereview.chromium.org/1287043002/diff/1/content/renderer/gpu/render... > content/renderer/gpu/render_widget_compositor.cc:960: > did_give_back_output_surface_ = true; > i have added this member to reflect the compositor's state. But couldnt find any > uasge. ping! :)
sievers@chromium.org changed reviewers: + danakj@chromium.org
On 2015/08/18 04:53:42, sohanjg wrote: > On 2015/08/12 10:50:29, sohanjg wrote: > > > https://codereview.chromium.org/1287043002/diff/1/content/renderer/gpu/render... > > File content/renderer/gpu/render_widget_compositor.cc (right): > > > > > https://codereview.chromium.org/1287043002/diff/1/content/renderer/gpu/render... > > content/renderer/gpu/render_widget_compositor.cc:960: > > did_give_back_output_surface_ = true; > > i have added this member to reflect the compositor's state. But couldnt find > any > > uasge. > > ping! :) Can you add some sort of unittest? You might want to look at some of the LayerTreeHost related unittests, such as LayerTreeHostClientNotReadyDoesNotCreateOutputSurface for inspiration. We'd then have to argue about what state it's in now. There are two cases: - the LTH is visible: Does taking the OS away mean it will immediately call RequestNewOutputSurface()? Or does it expect the client to just call SetOutputSurface() whenever it's ready? - the LTH is hidden: It should probably do nothing wrt OutputSurface But when it becomes visible again, does that trigger RequestNewOutputSurface()? Those cases should probably all be in a unit test.
On 2015/08/19 22:29:09, sievers wrote: > On 2015/08/18 04:53:42, sohanjg wrote: > > On 2015/08/12 10:50:29, sohanjg wrote: > > > > > > https://codereview.chromium.org/1287043002/diff/1/content/renderer/gpu/render... > > > File content/renderer/gpu/render_widget_compositor.cc (right): > > > > > > > > > https://codereview.chromium.org/1287043002/diff/1/content/renderer/gpu/render... > > > content/renderer/gpu/render_widget_compositor.cc:960: > > > did_give_back_output_surface_ = true; > > > i have added this member to reflect the compositor's state. But couldnt find > > any > > > uasge. > > > > ping! :) > > Can you add some sort of unittest? You might want to look at some of the > LayerTreeHost related unittests, such as > LayerTreeHostClientNotReadyDoesNotCreateOutputSurface for inspiration. > > We'd then have to argue about what state it's in now. There are two cases: > - the LTH is visible: > Does taking the OS away mean it will immediately call RequestNewOutputSurface()? > Or does it expect the client to just call SetOutputSurface() whenever it's > ready? > > - the LTH is hidden: > It should probably do nothing wrt OutputSurface > But when it becomes visible again, does that trigger RequestNewOutputSurface()? > > Those cases should probably all be in a unit test. OK. I just made this as a draft, to check if this is what we want. I will update some test soon. I was thinking maybe we can add our use cases in RenderWidgetCompositorOutputSurface in render_widget_compositor_unittest ? I will take care of the LTH visibility cases too.
On 2015/08/20 14:29:05, sohanjg wrote: > On 2015/08/19 22:29:09, sievers wrote: > > On 2015/08/18 04:53:42, sohanjg wrote: > > > On 2015/08/12 10:50:29, sohanjg wrote: > > > > > > > > > > https://codereview.chromium.org/1287043002/diff/1/content/renderer/gpu/render... > > > > File content/renderer/gpu/render_widget_compositor.cc (right): > > > > > > > > > > > > > > https://codereview.chromium.org/1287043002/diff/1/content/renderer/gpu/render... > > > > content/renderer/gpu/render_widget_compositor.cc:960: > > > > did_give_back_output_surface_ = true; > > > > i have added this member to reflect the compositor's state. But couldnt > find > > > any > > > > uasge. > > > > > > ping! :) > > > > Can you add some sort of unittest? You might want to look at some of the > > LayerTreeHost related unittests, such as > > LayerTreeHostClientNotReadyDoesNotCreateOutputSurface for inspiration. > > > > We'd then have to argue about what state it's in now. There are two cases: > > - the LTH is visible: > > Does taking the OS away mean it will immediately call > RequestNewOutputSurface()? > > Or does it expect the client to just call SetOutputSurface() whenever it's > > ready? > > > > - the LTH is hidden: > > It should probably do nothing wrt OutputSurface > > But when it becomes visible again, does that trigger > RequestNewOutputSurface()? > > > > Those cases should probably all be in a unit test. > > OK. I just made this as a draft, to check if this is what we want. > > I will update some test soon. > I was thinking maybe we can add our use cases in > RenderWidgetCompositorOutputSurface in render_widget_compositor_unittest ? We should have cc tests for the behavior. I don't think we do want to touch anything in RenderWidgetCompositor for this for now. If you insist on having another call site for the code, you can change the code in compositor_impl_android.cc to use this API in SetVisible() and remove the need to tear down the LTH. Regarding visibility and requesting a new output surface, let's make it simple: If you took the output surface away, you are expected to call SetOutputSurface() regardless of visibility. I.e LTH will not call RequestNewOutputSurface(), it will be in a state as if it did though.
Can you please advise on the test. I havent put any expectations yet. Because i am not sure on which thread should we try and get the output surface ? Also, even if i force it to give back output surface, it does not invoke SetOutputSuurface again, as we expected. Please give your opinion, thanks. https://codereview.chromium.org/1287043002/diff/20001/cc/trees/layer_tree_hos... File cc/trees/layer_tree_host_unittest_context.cc (right): https://codereview.chromium.org/1287043002/diff/20001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_unittest_context.cc:414: void DidInitializeOutputSurface() override { ReturnOpSurface(); } It expects this to be called in ImplThread, if we do it thru posttask in ImplRunner, then it fails in the layertreetest check for isMainThreadBlocked etc. Not sure how to proceed :/
Sorry for the super long delay, this fell off my radar! https://codereview.chromium.org/1287043002/diff/20001/cc/test/fake_proxy.h File cc/test/fake_proxy.h (right): https://codereview.chromium.org/1287043002/diff/20001/cc/test/fake_proxy.h#ne... cc/test/fake_proxy.h:28: scoped_ptr<OutputSurface> GetOutputSurface(); override https://codereview.chromium.org/1287043002/diff/20001/cc/trees/layer_tree_hos... File cc/trees/layer_tree_host.cc (right): https://codereview.chromium.org/1287043002/diff/20001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host.cc:400: scoped_ptr<OutputSurface> LayerTreeHost::GetOutputSurface() { DCHECK(!visible_); Otherwise it seems like you could end up displaying corruption. Also making the LTH invisible before releasing the OutputSurface causes resources to be deleted (incl. UI resource eviction), which is probably what you want before you do this. For this you will also have to make a small change here: bool SchedulerStateMachine::ShouldBeginOutputSurfaceCreation() const { + if (!visible_) + return false; https://codereview.chromium.org/1287043002/diff/20001/cc/trees/layer_tree_host.h File cc/trees/layer_tree_host.h (right): https://codereview.chromium.org/1287043002/diff/20001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host.h:115: scoped_ptr<OutputSurface> GetOutputSurface(); nit: Can we call it ReleaseOutputSurface()? https://codereview.chromium.org/1287043002/diff/20001/cc/trees/layer_tree_hos... File cc/trees/layer_tree_host_impl.cc (right): https://codereview.chromium.org/1287043002/diff/20001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_impl.cc:2198: return output_surface_.Pass(); |renderer_| and |resource_provider_| still have pointers to the underlying context. You could move the tear-down code from InitializeRenderer() into here, and then call ReleaseOutputSurface() from InitializeRenderer(). https://codereview.chromium.org/1287043002/diff/20001/cc/trees/layer_tree_hos... File cc/trees/layer_tree_host_unittest_context.cc (right): https://codereview.chromium.org/1287043002/diff/20001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_unittest_context.cc:403: void SetOutputSurface(scoped_ptr<OutputSurface> surface) { Can this just be part of the function above? https://codereview.chromium.org/1287043002/diff/20001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_unittest_context.cc:407: EndTest(); Don't end the test yet. https://codereview.chromium.org/1287043002/diff/20001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_unittest_context.cc:414: void DidInitializeOutputSurface() override { ReturnOpSurface(); } On 2015/08/26 14:38:56, sohanjg wrote: > It expects this to be called in ImplThread, if we do it thru posttask in > ImplRunner, then it fails in the layertreetest check for isMainThreadBlocked > etc. > Not sure how to proceed :/ What you can do here is: EXPECT_TRUE(layer_tree_host()->visible()); if (setos_counter == 0) { MainThreadTaskRunner()->PostTask( FROM_HERE, base::Bind(&LayerTreeHostClientTakeAwayOutputSurface:: HideAndReleaseOutputSurface, base::Unretained(this))); } else { EndTest(); } void HideAndReleaseOutputSurface() { EXPECT_TRUE(layer_tree_host()->proxy()->IsMainThread()); layer_tree_host()->SetVisible(false); layer_tree_host()->GetOutputSurface(); MainThreadTaskRunner()->PostTask( FROM_HERE, base::Bind(&LayerTreeHostClientTakeAwayOutputSurface:: MakeVisible, base::Unretained(this))); } void MakeVisible() { EXPECT_TRUE(layer_tree_host()->proxy()->IsMainThread()); layer_tree_host()->SetVisible(true); } https://codereview.chromium.org/1287043002/diff/20001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_unittest_context.cc:421: MULTI_THREAD_TEST_F(LayerTreeHostClientTakeAwayOutputSurface); SINGLE_AND_MULTI_THREAD_TEST_F https://codereview.chromium.org/1287043002/diff/20001/cc/trees/single_thread_... File cc/trees/single_thread_proxy.cc (right): https://codereview.chromium.org/1287043002/diff/20001/cc/trees/single_thread_... cc/trees/single_thread_proxy.cc:152: scoped_ptr<OutputSurface> SingleThreadProxy::GetOutputSurface() { if (scheduler_on_impl_thread_) scheduler_on_impl_thread_->DidLoseOutputSurface(); You can argue over whether the scheduler should trigger a RequestNewOutputSurface() or not after you become visible again, but I think you still need to clear some other state in the scheduler regardless which this should take care of. https://codereview.chromium.org/1287043002/diff/20001/cc/trees/thread_proxy.cc File cc/trees/thread_proxy.cc (right): https://codereview.chromium.org/1287043002/diff/20001/cc/trees/thread_proxy.c... cc/trees/thread_proxy.cc:222: scoped_ptr<OutputSurface> ThreadProxy::GetOutputSurface() { impl().scheduler->DidLoseOutputSurface(); https://codereview.chromium.org/1287043002/diff/20001/cc/trees/thread_proxy.h File cc/trees/thread_proxy.h (right): https://codereview.chromium.org/1287043002/diff/20001/cc/trees/thread_proxy.h... cc/trees/thread_proxy.h:246: scoped_ptr<OutputSurface> GetOutputSurface(); override and move up to under 'Proxy implementation' https://codereview.chromium.org/1287043002/diff/20001/content/renderer/gpu/re... File content/renderer/gpu/render_widget_compositor.h (right): https://codereview.chromium.org/1287043002/diff/20001/content/renderer/gpu/re... content/renderer/gpu/render_widget_compositor.h:81: scoped_ptr<cc::OutputSurface> GiveBackOutputSurface(); Can you remove the changes to RenderWidgetCompositor? Since the API is unused here and we have no immediate plan to use it in the renderer. https://codereview.chromium.org/1287043002/diff/20001/content/renderer/render... File content/renderer/render_widget.h (right): https://codereview.chromium.org/1287043002/diff/20001/content/renderer/render... content/renderer/render_widget.h:818: scoped_ptr<cc::OutputSurface> output_surface_; remove
https://codereview.chromium.org/1287043002/diff/20001/cc/trees/thread_proxy.cc File cc/trees/thread_proxy.cc (right): https://codereview.chromium.org/1287043002/diff/20001/cc/trees/thread_proxy.c... cc/trees/thread_proxy.cc:222: scoped_ptr<OutputSurface> ThreadProxy::GetOutputSurface() { You'll have to post this to the impl thread and then block on it, see for example FinishAllRendering().
thank you for the direction. please take a look. https://codereview.chromium.org/1287043002/diff/20001/cc/test/fake_proxy.h File cc/test/fake_proxy.h (right): https://codereview.chromium.org/1287043002/diff/20001/cc/test/fake_proxy.h#ne... cc/test/fake_proxy.h:28: scoped_ptr<OutputSurface> GetOutputSurface(); On 2015/09/10 00:00:22, sievers wrote: > override Done. https://codereview.chromium.org/1287043002/diff/20001/cc/trees/layer_tree_hos... File cc/trees/layer_tree_host.cc (right): https://codereview.chromium.org/1287043002/diff/20001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host.cc:400: scoped_ptr<OutputSurface> LayerTreeHost::GetOutputSurface() { On 2015/09/10 00:00:22, sievers wrote: > DCHECK(!visible_); > > Otherwise it seems like you could end up displaying corruption. > Also making the LTH invisible before releasing the OutputSurface causes > resources to be deleted (incl. UI resource eviction), which is probably what you > want before you do this. > > For this you will also have to make a small change here: > > bool SchedulerStateMachine::ShouldBeginOutputSurfaceCreation() const { > + if (!visible_) > + return false; sorry, i didnt get this part, you mean before asking for OS from proxy, we need to set LTH visibility as false, right ? should we also inform scheduler for an explicit OS creation after we release ? https://codereview.chromium.org/1287043002/diff/20001/cc/trees/layer_tree_host.h File cc/trees/layer_tree_host.h (right): https://codereview.chromium.org/1287043002/diff/20001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host.h:115: scoped_ptr<OutputSurface> GetOutputSurface(); On 2015/09/10 00:00:22, sievers wrote: > nit: Can we call it ReleaseOutputSurface()? Done. https://codereview.chromium.org/1287043002/diff/20001/cc/trees/layer_tree_hos... File cc/trees/layer_tree_host_impl.cc (right): https://codereview.chromium.org/1287043002/diff/20001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_impl.cc:2198: return output_surface_.Pass(); On 2015/09/10 00:00:22, sievers wrote: > |renderer_| and |resource_provider_| still have pointers to the underlying > context. You could move the tear-down code from InitializeRenderer() into here, > and then call ReleaseOutputSurface() from InitializeRenderer(). Done. https://codereview.chromium.org/1287043002/diff/20001/cc/trees/layer_tree_hos... File cc/trees/layer_tree_host_unittest_context.cc (right): https://codereview.chromium.org/1287043002/diff/20001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_unittest_context.cc:403: void SetOutputSurface(scoped_ptr<OutputSurface> surface) { On 2015/09/10 00:00:22, sievers wrote: > Can this just be part of the function above? Done. https://codereview.chromium.org/1287043002/diff/20001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_unittest_context.cc:407: EndTest(); On 2015/09/10 00:00:22, sievers wrote: > Don't end the test yet. Done. https://codereview.chromium.org/1287043002/diff/20001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_unittest_context.cc:414: void DidInitializeOutputSurface() override { ReturnOpSurface(); } On 2015/09/10 00:00:22, sievers wrote: > On 2015/08/26 14:38:56, sohanjg wrote: > > It expects this to be called in ImplThread, if we do it thru posttask in > > ImplRunner, then it fails in the layertreetest check for isMainThreadBlocked > > etc. > > Not sure how to proceed :/ > > What you can do here is: > > > EXPECT_TRUE(layer_tree_host()->visible()); > if (setos_counter == 0) { > MainThreadTaskRunner()->PostTask( > FROM_HERE, base::Bind(&LayerTreeHostClientTakeAwayOutputSurface:: > HideAndReleaseOutputSurface, > base::Unretained(this))); > } else { > EndTest(); > } > > void HideAndReleaseOutputSurface() { > EXPECT_TRUE(layer_tree_host()->proxy()->IsMainThread()); > layer_tree_host()->SetVisible(false); > layer_tree_host()->GetOutputSurface(); > MainThreadTaskRunner()->PostTask( > FROM_HERE, base::Bind(&LayerTreeHostClientTakeAwayOutputSurface:: > MakeVisible, > base::Unretained(this))); > } > > void MakeVisible() { > EXPECT_TRUE(layer_tree_host()->proxy()->IsMainThread()); > layer_tree_host()->SetVisible(true); > } For Single Threaded test, the call to MakeVisible may come after 2nd OS is requested. So, moved the visibility check inside set_os = 0. https://codereview.chromium.org/1287043002/diff/20001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_unittest_context.cc:421: MULTI_THREAD_TEST_F(LayerTreeHostClientTakeAwayOutputSurface); On 2015/09/10 00:00:22, sievers wrote: > SINGLE_AND_MULTI_THREAD_TEST_F Done. https://codereview.chromium.org/1287043002/diff/20001/cc/trees/single_thread_... File cc/trees/single_thread_proxy.cc (right): https://codereview.chromium.org/1287043002/diff/20001/cc/trees/single_thread_... cc/trees/single_thread_proxy.cc:152: scoped_ptr<OutputSurface> SingleThreadProxy::GetOutputSurface() { On 2015/09/10 00:00:22, sievers wrote: > if (scheduler_on_impl_thread_) > scheduler_on_impl_thread_->DidLoseOutputSurface(); > > You can argue over whether the scheduler should trigger a > RequestNewOutputSurface() or not after you become visible again, but I think you > still need to clear some other state in the scheduler regardless which this > should take care of. Done. https://codereview.chromium.org/1287043002/diff/20001/cc/trees/thread_proxy.cc File cc/trees/thread_proxy.cc (right): https://codereview.chromium.org/1287043002/diff/20001/cc/trees/thread_proxy.c... cc/trees/thread_proxy.cc:222: scoped_ptr<OutputSurface> ThreadProxy::GetOutputSurface() { On 2015/09/10 01:26:24, sievers wrote: > You'll have to post this to the impl thread and then block on it, see for > example FinishAllRendering(). You mean both DidLoseOutputSurface, and Get/ReleaseOS shud be called in impl thread, right ? But, we are expecting to return the OS from this call. I have maintained OS as a member in ThreadProxy, so that both the threads have access to modify it. We are protected as main will not read till impl has completed writing it. https://codereview.chromium.org/1287043002/diff/20001/cc/trees/thread_proxy.h File cc/trees/thread_proxy.h (right): https://codereview.chromium.org/1287043002/diff/20001/cc/trees/thread_proxy.h... cc/trees/thread_proxy.h:246: scoped_ptr<OutputSurface> GetOutputSurface(); On 2015/09/10 00:00:22, sievers wrote: > override and move up to under 'Proxy implementation' Done. https://codereview.chromium.org/1287043002/diff/20001/content/renderer/gpu/re... File content/renderer/gpu/render_widget_compositor.h (right): https://codereview.chromium.org/1287043002/diff/20001/content/renderer/gpu/re... content/renderer/gpu/render_widget_compositor.h:81: scoped_ptr<cc::OutputSurface> GiveBackOutputSurface(); On 2015/09/10 00:00:22, sievers wrote: > Can you remove the changes to RenderWidgetCompositor? Since the API is unused > here and we have no immediate plan to use it in the renderer. Done. https://codereview.chromium.org/1287043002/diff/20001/content/renderer/render... File content/renderer/render_widget.h (right): https://codereview.chromium.org/1287043002/diff/20001/content/renderer/render... content/renderer/render_widget.h:818: scoped_ptr<cc::OutputSurface> output_surface_; On 2015/09/10 00:00:22, sievers wrote: > remove Done.
https://codereview.chromium.org/1287043002/diff/20001/cc/trees/layer_tree_hos... File cc/trees/layer_tree_host.cc (right): https://codereview.chromium.org/1287043002/diff/20001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host.cc:400: scoped_ptr<OutputSurface> LayerTreeHost::GetOutputSurface() { On 2015/09/10 15:07:23, sohanjg wrote: > On 2015/09/10 00:00:22, sievers wrote: > > DCHECK(!visible_); > > > > Otherwise it seems like you could end up displaying corruption. > > Also making the LTH invisible before releasing the OutputSurface causes > > resources to be deleted (incl. UI resource eviction), which is probably what > you > > want before you do this. > > > > For this you will also have to make a small change here: > > > > bool SchedulerStateMachine::ShouldBeginOutputSurfaceCreation() const { > > + if (!visible_) > > + return false; > > sorry, i didnt get this part, you mean before asking for OS from proxy, we need > to set LTH visibility as false, right ? > Yes, I think it makes sense to properly clean up resources instead of going through the more forceful 'lost context' codepath entirely here. I'm afraid that otherwise you could end up with glitches on the screen or GL errors in the log. > should we also inform scheduler for an explicit OS creation after we release ? We can leave the scheduler alone. I can change that behavior in a separate patch. It's a bit orthogonal whether LTH asks the client for a new OS while it's hidden or not. https://codereview.chromium.org/1287043002/diff/40001/cc/test/fake_proxy.cc File cc/test/fake_proxy.cc (right): https://codereview.chromium.org/1287043002/diff/40001/cc/test/fake_proxy.cc#n... cc/test/fake_proxy.cc:28: return NULL; nit: nullptr https://codereview.chromium.org/1287043002/diff/40001/cc/trees/layer_tree_hos... File cc/trees/layer_tree_host.cc (right): https://codereview.chromium.org/1287043002/diff/40001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host.cc:400: DCHECK(!visible_); DCHECK(!output_surface_lost_) also https://codereview.chromium.org/1287043002/diff/40001/cc/trees/layer_tree_hos... File cc/trees/layer_tree_host_unittest_context.cc (right): https://codereview.chromium.org/1287043002/diff/40001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_unittest_context.cc:418: EXPECT_TRUE(layer_tree_host()->visible()); I think the reason that you were not able to move this check outside of the if-statement is that you didn't make the change to the scheduler state machine and it now immediately asks you for a new OutputSurface after you release it. We can pull the behavioral change into a separate patch though (that's probably better since otherwise the patch doesn't change existing behavior in use), but then you should change the test to not call CreateAndSetOutputSurface() the second time until after we made the LTH visible again. https://codereview.chromium.org/1287043002/diff/40001/cc/trees/thread_proxy.cc File cc/trees/thread_proxy.cc (right): https://codereview.chromium.org/1287043002/diff/40001/cc/trees/thread_proxy.c... cc/trees/thread_proxy.cc:229: return output_surface_.Pass(); You don't need this variable, but you can have a scoped_ptr<> on the stack here and then pass a ptr to it to ReleaseOutputSurfaceOnImplThread(). https://codereview.chromium.org/1287043002/diff/40001/cc/trees/thread_proxy.c... cc/trees/thread_proxy.cc:1099: TRACE_EVENT0("cc", "ThreadProxy::FinishAllRenderingOnImplThread"); nit: "ThreadProxy::ReleaseOutputSurfaceOnImplThread" https://codereview.chromium.org/1287043002/diff/40001/content/renderer/gpu/re... File content/renderer/gpu/render_widget_compositor.cc (right): https://codereview.chromium.org/1287043002/diff/40001/content/renderer/gpu/re... content/renderer/gpu/render_widget_compositor.cc:218: weak_factory_(this) {} nit: change not needed, remove this file from patch set
please take a look, thanks. https://codereview.chromium.org/1287043002/diff/20001/cc/trees/layer_tree_hos... File cc/trees/layer_tree_host.cc (right): https://codereview.chromium.org/1287043002/diff/20001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host.cc:400: scoped_ptr<OutputSurface> LayerTreeHost::GetOutputSurface() { On 2015/09/10 17:27:45, sievers wrote: > On 2015/09/10 15:07:23, sohanjg wrote: > > On 2015/09/10 00:00:22, sievers wrote: > > > DCHECK(!visible_); > > > > > > Otherwise it seems like you could end up displaying corruption. > > > Also making the LTH invisible before releasing the OutputSurface causes > > > resources to be deleted (incl. UI resource eviction), which is probably what > > you > > > want before you do this. > > > > > > For this you will also have to make a small change here: > > > > > > bool SchedulerStateMachine::ShouldBeginOutputSurfaceCreation() const { > > > + if (!visible_) > > > + return false; > > > > sorry, i didnt get this part, you mean before asking for OS from proxy, we > need > > to set LTH visibility as false, right ? > > > > Yes, I think it makes sense to properly clean up resources instead of going > through the more forceful 'lost context' codepath entirely here. > I'm afraid that otherwise you could end up with glitches on the screen or GL > errors in the log. > > > should we also inform scheduler for an explicit OS creation after we release ? > > > We can leave the scheduler alone. I can change that behavior in a separate > patch. It's a bit orthogonal whether LTH asks the client for a new OS while it's > hidden or not. Acknowledged. https://codereview.chromium.org/1287043002/diff/40001/cc/test/fake_proxy.cc File cc/test/fake_proxy.cc (right): https://codereview.chromium.org/1287043002/diff/40001/cc/test/fake_proxy.cc#n... cc/test/fake_proxy.cc:28: return NULL; On 2015/09/10 17:27:45, sievers wrote: > nit: nullptr Done. https://codereview.chromium.org/1287043002/diff/40001/cc/trees/layer_tree_hos... File cc/trees/layer_tree_host.cc (right): https://codereview.chromium.org/1287043002/diff/40001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host.cc:400: DCHECK(!visible_); On 2015/09/10 17:27:45, sievers wrote: > DCHECK(!output_surface_lost_) also Done. https://codereview.chromium.org/1287043002/diff/40001/cc/trees/layer_tree_hos... File cc/trees/layer_tree_host_unittest_context.cc (right): https://codereview.chromium.org/1287043002/diff/40001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_unittest_context.cc:418: EXPECT_TRUE(layer_tree_host()->visible()); On 2015/09/10 17:27:45, sievers wrote: > I think the reason that you were not able to move this check outside of the > if-statement is that you didn't make the change to the scheduler state machine > and it now immediately asks you for a new OutputSurface after you release it. We > can pull the behavioral change into a separate patch though (that's probably > better since otherwise the patch doesn't change existing behavior in use), but > then you should change the test to not call CreateAndSetOutputSurface() the > second time until after we made the LTH visible again. Done. https://codereview.chromium.org/1287043002/diff/40001/cc/trees/thread_proxy.cc File cc/trees/thread_proxy.cc (right): https://codereview.chromium.org/1287043002/diff/40001/cc/trees/thread_proxy.c... cc/trees/thread_proxy.cc:229: return output_surface_.Pass(); On 2015/09/10 17:27:45, sievers wrote: > You don't need this variable, but you can have a scoped_ptr<> on the stack here > and then pass a ptr to it to ReleaseOutputSurfaceOnImplThread(). Done. https://codereview.chromium.org/1287043002/diff/40001/cc/trees/thread_proxy.c... cc/trees/thread_proxy.cc:1099: TRACE_EVENT0("cc", "ThreadProxy::FinishAllRenderingOnImplThread"); On 2015/09/10 17:27:45, sievers wrote: > nit: "ThreadProxy::ReleaseOutputSurfaceOnImplThread" Done.
That looks good to me. @Dana: wdyt? https://codereview.chromium.org/1287043002/diff/60001/cc/trees/layer_tree_hos... File cc/trees/layer_tree_host_unittest_context.cc (right): https://codereview.chromium.org/1287043002/diff/60001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_unittest_context.cc:394: if (!made_visible_) You mean 'if (made_visible_)'? Or you could simply do 'if (layer_tree_host()->visible()'. It's supposed to test the following use case (and it would be nice to put that in a comment above the class here: // SetUp LTH and create and init OutputSurface // LTH::SetVisible(false); // LTH::ReleaseOutputSurface(); // ... // LTH::SetVisible(true); // Create and init new OutputSurface https://codereview.chromium.org/1287043002/diff/60001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_unittest_context.cc:420: if (setos_counter_ == 0) { == 1? Otherwise this is unreachable, and it probably explains how your test passed with the inverted condition in line 394.
Patchset #5 (id:80001) has been deleted
i had to update the test a bit, seems we were skipping a lot of code as you pointed. also we were not actually updating the OS pointer in impl-thread it seems. please take a look, thanks. https://codereview.chromium.org/1287043002/diff/60001/cc/trees/layer_tree_hos... File cc/trees/layer_tree_host_unittest_context.cc (right): https://codereview.chromium.org/1287043002/diff/60001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_unittest_context.cc:394: if (!made_visible_) On 2015/09/11 22:44:50, sievers wrote: > You mean 'if (made_visible_)'? Or you could simply do 'if > (layer_tree_host()->visible()'. > > It's supposed to test the following use case (and it would be nice to put that > in a comment above the class here: > > // SetUp LTH and create and init OutputSurface > // LTH::SetVisible(false); > // LTH::ReleaseOutputSurface(); > // ... > // LTH::SetVisible(true); > // Create and init new OutputSurface Done. https://codereview.chromium.org/1287043002/diff/60001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_unittest_context.cc:420: if (setos_counter_ == 0) { On 2015/09/11 22:44:50, sievers wrote: > == 1? Otherwise this is unreachable, and it probably explains how your test > passed with the inverted condition in line 394. Done.
https://codereview.chromium.org/1287043002/diff/100001/cc/trees/layer_tree_ho... File cc/trees/layer_tree_host_unittest_context.cc (right): https://codereview.chromium.org/1287043002/diff/100001/cc/trees/layer_tree_ho... cc/trees/layer_tree_host_unittest_context.cc:404: EndTest(); Can we still go through and create another OutputSurface after making the LTH visible again? I think it's worth testing also that there are no broken assumptions and that succeeds without blowing up. Don't you just have to move the EndTest() to DidInitializeOutputSurface() for when |setos_counter == 2|? You'd also have to handle races between RequestNewOutputSurface() and MakeVisible() to trigger the new OutputSurface creation (you'd probably just need to track a |requested_| bool). But maybe it's easier if you just rebase this on top of https://codereview.chromium.org/1336813002/? Then RequestNewOS() will not happen until after MakeVisible(). https://codereview.chromium.org/1287043002/diff/100001/cc/trees/layer_tree_ho... cc/trees/layer_tree_host_unittest_context.cc:430: EXPECT_TRUE(layer_tree_host()->visible()); I think you can pull this out of the if-block regardless now.
Patchset #6 (id:120001) has been deleted
please take a look. thanks. https://codereview.chromium.org/1287043002/diff/100001/cc/trees/layer_tree_ho... File cc/trees/layer_tree_host_unittest_context.cc (right): https://codereview.chromium.org/1287043002/diff/100001/cc/trees/layer_tree_ho... cc/trees/layer_tree_host_unittest_context.cc:404: EndTest(); On 2015/09/14 21:28:13, sievers wrote: > Can we still go through and create another OutputSurface after making the LTH > visible again? > > I think it's worth testing also that there are no broken assumptions and that > succeeds without blowing up. > > Don't you just have to move the EndTest() to DidInitializeOutputSurface() for > when |setos_counter == 2|? > > You'd also have to handle races between RequestNewOutputSurface() and > MakeVisible() to trigger the new OutputSurface creation (you'd probably just > need to track a |requested_| bool). But maybe it's easier if you just rebase > this on top of https://codereview.chromium.org/1336813002/ Then RequestNewOS() > will not happen until after MakeVisible(). Yes, in the current form we were getting RequestNewOutputSurface, before MakeVisible, hence the EndTest here. But, you are right, there is scope of race between the 2 calls. If i rebase on top of your scheduler changes, the order is maintained well i.e new OS request comes only after makevisible is executed. I will move this to didinitializeos.. . So, what should be the plan ? we wait for the scheduler patch to land? and then push this? https://codereview.chromium.org/1287043002/diff/100001/cc/trees/layer_tree_ho... cc/trees/layer_tree_host_unittest_context.cc:430: EXPECT_TRUE(layer_tree_host()->visible()); On 2015/09/14 21:28:13, sievers wrote: > I think you can pull this out of the if-block regardless now. Done.
lgtm, thanks. but needs https://codereview.chromium.org/1336813002/ to land first.
On 2015/09/15 17:41:06, sievers wrote: > lgtm, thanks. > > but needs https://codereview.chromium.org/1336813002/ to land first. Dana@ can you pls have a look, before we cq it.
The CQ bit was checked by sohan.jyoti@samsung.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1287043002/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1287043002/140001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: The author sohan.jyoti@samsung.com has not signed Google Contributor License Agreement. Please visit https://cla.developers.google.com to sign and manage CLA.
The CQ bit was checked by sohan.jyoti@samsung.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1287043002/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1287043002/140001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
On 2015/09/17 10:13:29, commit-bot: I haz the power wrote: > Try jobs failed on following builders: > chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, > http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) Looks like the real CQ got triggered, i wanted to try the dry run to check my CLA status.
Cool looks good, few comments. https://codereview.chromium.org/1287043002/diff/140001/cc/trees/layer_tree_ho... File cc/trees/layer_tree_host.cc (right): https://codereview.chromium.org/1287043002/diff/140001/cc/trees/layer_tree_ho... cc/trees/layer_tree_host.cc:403: output_surface_lost_ = true; rather than doing this directly here, could this function call DidLoseOutputSurface? https://codereview.chromium.org/1287043002/diff/140001/cc/trees/layer_tree_ho... File cc/trees/layer_tree_host_impl.cc (right): https://codereview.chromium.org/1287043002/diff/140001/cc/trees/layer_tree_ho... cc/trees/layer_tree_host_impl.cc:2159: // Since we will create a new resource provider, we cannot continue to use A trace event in here might be helpful https://codereview.chromium.org/1287043002/diff/140001/cc/trees/single_thread... File cc/trees/single_thread_proxy.cc (right): https://codereview.chromium.org/1287043002/diff/140001/cc/trees/single_thread... cc/trees/single_thread_proxy.cc:154: scheduler_on_impl_thread_->DidLoseOutputSurface(); Can you DCHECK that LTH knows the OS is lost, and comment on that saying that we don't need to call LTH::DidLoseOutputSurface since it already knows? https://codereview.chromium.org/1287043002/diff/140001/cc/trees/thread_proxy.cc File cc/trees/thread_proxy.cc (right): https://codereview.chromium.org/1287043002/diff/140001/cc/trees/thread_proxy.... cc/trees/thread_proxy.cc:220: scoped_ptr<OutputSurface> ThreadProxy::ReleaseOutputSurface() { Can you DCHECK that LTH knows the OS is lost? https://codereview.chromium.org/1287043002/diff/140001/cc/trees/thread_proxy.... cc/trees/thread_proxy.cc:230: return output_surface.Pass(); don't need Pass() on return https://codereview.chromium.org/1287043002/diff/140001/cc/trees/thread_proxy.... cc/trees/thread_proxy.cc:1101: TRACE_EVENT0("cc", "ThreadProxy::ReleaseOutputSurfaceOnImplThread"); we didn't trace in the STP, i think i'd rather the trace be in LTHI. https://codereview.chromium.org/1287043002/diff/140001/cc/trees/thread_proxy.... cc/trees/thread_proxy.cc:1104: impl().scheduler->DidLoseOutputSurface(); Can you comment saying that, unlike in DidLoseOutputSurfaceOnImplThread, we don't need to call LayerTreeHost::DidLoseOutputSurface since it already knows?
The CQ bit was checked by sievers@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1287043002/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1287043002/140001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM with https://codereview.chromium.org/1352763003/ as followup
The CQ bit was checked by sievers@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1287043002/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1287043002/140001
Message was sent while issue was closed.
Committed patchset #6 (id:140001)
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/3a33d87b15b764a6db1cd863ce54efec2bcd2b23 Cr-Commit-Position: refs/heads/master@{#349787} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
