Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
17176027:
Add gpu command buffer support for loseContextCHROMIUM (Closed)
|
Created:
7 years, 6 months ago by Justin Novosad Modified:
7 years, 6 months ago Reviewers:
piman CC:
chromium-reviews, joi+watch-content_chromium.org, darin-cc_chromium.org, cc-bugs_chromium.org, jam, apatrick_chromium Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionAdd gpu command buffer support for loseContextCHROMIUM
Calls to loseContextCHROMIUM were only being propagated by the in-process command buffer. This patch adds it to the inter-process version of the command buffer. This is important for layout tests that use loseContextCHROMIUM to work as intended in content_shell.
BUG=251901
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=208810
Patch Set 1 #Patch Set 2 : patch 2 #
Total comments: 5
Patch Set 3 : response to piman feedback #
Messages
Total messages: 11 (0 generated)
PTAL piman ->src/content/common enne -> src/cc/layers
https://codereview.chromium.org/17176027/diff/2001/cc/layers/texture_layer.cc File cc/layers/texture_layer.cc (right): https://codereview.chromium.org/17176027/diff/2001/cc/layers/texture_layer.cc... cc/layers/texture_layer.cc:167: if (rate_limit_context_ && client_ && client_->Context3d() && I was getting test crashes right here because of lack of NULL ptr check after context loss. The other similar checks I added in this file are speculative. Do you think they are necessary? https://codereview.chromium.org/17176027/diff/2001/content/common/message_rou... File content/common/message_router.cc (right): https://codereview.chromium.org/17176027/diff/2001/content/common/message_rou... content/common/message_router.cc:49: return listener->OnMessageReceived(msg); This is to prevent an IPC deadlock between the renderer and GPU processes. In the case where a gpu context is lost during SyncFlush listener->OnMessageReceived(msg) will return false, and we need to propagate the failure to the caller ( GpuChannel::HandleMessage() in this case) to ensure that a sync response is sent back.
https://codereview.chromium.org/17176027/diff/2001/cc/layers/texture_layer.cc File cc/layers/texture_layer.cc (right): https://codereview.chromium.org/17176027/diff/2001/cc/layers/texture_layer.cc... cc/layers/texture_layer.cc:167: if (rate_limit_context_ && client_ && client_->Context3d() && On 2013/06/21 18:24:11, junov wrote: > I was getting test crashes right here because of lack of NULL ptr check after > context loss. The other similar checks I added in this file are speculative. > Do you think they are necessary? Why is the context NULL after a context loss? Losing the context doesn't mean the context has to become NULL. It's the responsibility of the client to keep the context around.... https://codereview.chromium.org/17176027/diff/2001/content/common/gpu/client/... File content/common/gpu/client/webgraphicscontext3d_command_buffer_impl.cc (right): https://codereview.chromium.org/17176027/diff/2001/content/common/gpu/client/... content/common/gpu/client/webgraphicscontext3d_command_buffer_impl.cc:1605: gl_->Finish(); Actually, doing a Finish() here makes it less interesting as far as testing goes, I would replace it by a Flush(). In production, context loss will always be asynchronous, and 95% of the issues are to deal with that asynchronicity. If some given tests really need a synchronous behavior, they can call finish() directly. https://codereview.chromium.org/17176027/diff/2001/content/common/message_rou... File content/common/message_router.cc (right): https://codereview.chromium.org/17176027/diff/2001/content/common/message_rou... content/common/message_router.cc:49: return listener->OnMessageReceived(msg); On 2013/06/21 18:24:11, junov wrote: > This is to prevent an IPC deadlock between the renderer and GPU processes. In > the case where a gpu context is lost during SyncFlush > listener->OnMessageReceived(msg) will return false, and we need to propagate the > failure to the caller ( GpuChannel::HandleMessage() in this case) to ensure that > a sync response is sent back. This is used in many places, for many different channels. The contract is that RouteMessage only returns false if there's no receiver. If there is, the receiver (in this case, GpuCommandBufferStub) is responsible for sending the error reply. I think changing this will have a lot more impact than just the GPU process. I'm not opposed to the change of semantics - it makes sense - but this needs to be done in a separate change, while auditing/fixing all users so that we don't send 2 replies or something. The alternative is to fix GpuCommandBufferStub::OnMessageReceived to send an error reply if it early returns false. Also, is the problem actually happening, still? In r206264 I changed GetStateFast to not call MakeCurrent, which also means it won't return false, so FlushSync shouldn't block. That said, there's a couple of other calls that might.
On 2013/06/21 19:05:05, piman wrote: > Why is the context NULL after a context loss? It has to do with a WIP Blink CL I am working on : https://codereview.chromium.org/16032003/ With that change, Canvas2DLayerBridge drops its ref on the graphics context when the context is lost. The code in question here is obtaining the context3d from the layer bridge. > Losing the context doesn't mean the context has to become NULL. It's the > responsibility of the client to keep the context around.... > > https://codereview.chromium.org/17176027/diff/2001/content/common/gpu/client/... > File content/common/gpu/client/webgraphicscontext3d_command_buffer_impl.cc > (right): > > https://codereview.chromium.org/17176027/diff/2001/content/common/gpu/client/... > content/common/gpu/client/webgraphicscontext3d_command_buffer_impl.cc:1605: > gl_->Finish(); > Actually, doing a Finish() here makes it less interesting as far as testing > goes, I would replace it by a Flush(). > > In production, context loss will always be asynchronous, and 95% of the issues > are to deal with that asynchronicity. > > If some given tests really need a synchronous behavior, they can call finish() > directly. Right, I will do just that. > > This is used in many places, for many different channels. The contract is that > RouteMessage only returns false if there's no receiver. If there is, the > receiver (in this case, GpuCommandBufferStub) is responsible for sending the > error reply. > > I think changing this will have a lot more impact than just the GPU process. I'm > not opposed to the change of semantics - it makes sense - but this needs to be > done in a separate change, while auditing/fixing all users so that we don't send > 2 replies or something. > > The alternative is to fix GpuCommandBufferStub::OnMessageReceived to send an > error reply if it early returns false. That sound like a much safer alternative > > Also, is the problem actually happening, still? In r206264 I changed > GetStateFast to not call MakeCurrent, which also means it won't return false, so > FlushSync shouldn't block. That said, there's a couple of other calls that > might. I am updating right to to see if this is still an issue. Thanks.
On Fri, Jun 21, 2013 at 12:19 PM, <junov@chromium.org> wrote: > On 2013/06/21 19:05:05, piman wrote: > >> Why is the context NULL after a context loss? >> > > It has to do with a WIP Blink CL I am working on : > https://codereview.chromium.**org/16032003/<https://codereview.chromium.org/1... > With that change, Canvas2DLayerBridge drops its ref on the graphics > context when > the context is lost. > Right, it needs to keep it around, until it gets a new one. > > The code in question here is obtaining the context3d from the layer bridge. > > > Losing the context doesn't mean the context has to become NULL. It's the >> responsibility of the client to keep the context around.... >> > > > https://codereview.chromium.**org/17176027/diff/2001/** > content/common/gpu/client/**webgraphicscontext3d_command_**buffer_impl.cc<https://codereview.chromium.org/17176027/diff/2001/content/common/gpu/client/webgraphicscontext3d_command_buffer_impl.cc> > >> File content/common/gpu/client/**webgraphicscontext3d_command_** >> buffer_impl.cc >> (right): >> > > > https://codereview.chromium.**org/17176027/diff/2001/** > content/common/gpu/client/**webgraphicscontext3d_command_** > buffer_impl.cc#newcode1605<https://codereview.chromium.org/17176027/diff/2001/content/common/gpu/client/webgraphicscontext3d_command_buffer_impl.cc#newcode1605> > >> content/common/gpu/client/**webgraphicscontext3d_command_** >> buffer_impl.cc:1605: >> gl_->Finish(); >> Actually, doing a Finish() here makes it less interesting as far as >> testing >> goes, I would replace it by a Flush(). >> > > In production, context loss will always be asynchronous, and 95% of the >> issues >> are to deal with that asynchronicity. >> > > If some given tests really need a synchronous behavior, they can call >> finish() >> directly. >> > > Right, I will do just that. > > > > > This is used in many places, for many different channels. The contract is >> that >> RouteMessage only returns false if there's no receiver. If there is, the >> receiver (in this case, GpuCommandBufferStub) is responsible for sending >> the >> error reply. >> > > I think changing this will have a lot more impact than just the GPU >> process. >> > I'm > >> not opposed to the change of semantics - it makes sense - but this needs >> to be >> done in a separate change, while auditing/fixing all users so that we >> don't >> > send > >> 2 replies or something. >> > > The alternative is to fix GpuCommandBufferStub::**OnMessageReceived to >> send an >> error reply if it early returns false. >> > > That sound like a much safer alternative > > > Also, is the problem actually happening, still? In r206264 I changed >> GetStateFast to not call MakeCurrent, which also means it won't return >> false, >> > so > >> FlushSync shouldn't block. That said, there's a couple of other calls that >> might. >> > > I am updating right to to see if this is still an issue. Thanks. > > > https://codereview.chromium.**org/17176027/<https://codereview.chromium.org/1... >
On 2013/06/21 19:46:13, piman wrote: > > Right, it needs to keep it around, until it gets a new one. > > No problem, that's easy to workaround in Canvas2DLayerBridge.
New Patch. piman: Like you said, the removal of MakeCurrent from GetStateFast does solve the deadlock issue. I also removed the changes to texture_layer.cc which are not necessary (made up for it on the Blink side)
Also, I changed the implementation of loseContextCHROMIUM to call Flush() rather than Finish(), as you suggested. However, I am not 100% sure this is the right thing to do. AFAIK, loseContextCHROMIUM is only used for testing robustness, I think that by making the context loss synchronous (with Finish()) we will have less flaky tests. Of course the caller can always decide whether or not to call Finish after calling loseContextCHROMIUM, but I am concerned that this will often be overlooked, and we'll get flaky tests as a result.
LGTM. I think keeping it asynchronous leaves us with the option to write useful tests with the interaction with asynchronous things, such as sync points or queries.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/junov@chromium.org/17176027/12001
Message was sent while issue was closed.
Change committed as 208810 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
