Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
101163004:
Fix the context loss issue when switching tabs after enabled Canvas2D hardware acceleration (Closed)
|
Created:
6 years, 11 months ago by changjun.yang Modified:
4 years, 11 months ago CC:
blink-reviews, jamesr, krit, dsinclair, jbroman, danakj, Rik, Stephen Chennney, pdr., rwlbuis, Jin Yang Base URL:
http://src.chromium.org/blink/trunk/ Visibility:
Public. |
DescriptionFix the context loss issue when switching tabs after enabled Canvas2D hardware acceleration
Currently Fix it by force prepare Mailbox.
Patch Set 1 #Patch Set 2 : #
Total comments: 2
Patch Set 3 : #
Total comments: 1
Patch Set 4 : #Patch Set 5 : #
Total comments: 1
Messages
Total messages: 24 (0 generated)
I'm not too familiar with this code, but.. The more fundamental issue is webview can permanently drop back to software mode. In that case, my current thinking is webview needs to fallback to software canvas, which I suspect doesn't use this class.
I think the problem you are trying to resolve is the one I mentioned here: https://codereview.chromium.org/140693015/#msg1 After that CL, webview will release the mailbox when switching back to software-only mode (call to Canvas2DLayerBridge::mailboxReleased). In this case, prepareTexture should ignore the m_lastImageId check and return true. Do you want to modify this CL and land it?
On 2014/01/28 00:16:45, boliu wrote: > I think the problem you are trying to resolve is the one I mentioned here: > https://codereview.chromium.org/140693015/#msg1 > > After that CL, webview will release the mailbox when switching back to > software-only mode (call to Canvas2DLayerBridge::mailboxReleased). In this case, > prepareTexture should ignore the m_lastImageId check and return true. > > Do you want to modify this CL and land it? Yes, but I may do it after Chinese Spring Festival. :( Thanks!
On 2014/01/28 06:45:04, changjun.yang wrote: > On 2014/01/28 00:16:45, boliu wrote: > > I think the problem you are trying to resolve is the one I mentioned here: > > https://codereview.chromium.org/140693015/#msg1 > > > > After that CL, webview will release the mailbox when switching back to > > software-only mode (call to Canvas2DLayerBridge::mailboxReleased). In this > case, > > prepareTexture should ignore the m_lastImageId check and return true. > > > > Do you want to modify this CL and land it? > > Yes, but I may do it after Chinese Spring Festival. :( > Thanks! Any progress on this? If not, I can code it up tomorrow.
On 2014/02/11 02:56:15, boliu wrote: > On 2014/01/28 06:45:04, changjun.yang wrote: > > On 2014/01/28 00:16:45, boliu wrote: > > > I think the problem you are trying to resolve is the one I mentioned here: > > > https://codereview.chromium.org/140693015/#msg1 > > > > > > After that CL, webview will release the mailbox when switching back to > > > software-only mode (call to Canvas2DLayerBridge::mailboxReleased). In this > > case, > > > prepareTexture should ignore the m_lastImageId check and return true. > > > > > > Do you want to modify this CL and land it? > > > > Yes, but I may do it after Chinese Spring Festival. :( > > Thanks! > > Any progress on this? If not, I can code it up tomorrow. Updated based on FreeTextureMailbox(), this issue was initially identified via FishIETank: http://ie.microsoft.com/testdrive/Performance/FishIETank/. However, I couldn't verify its Correctness for trunk currently.
+senorblanco to advise since I'm not very familiar with this code myself Background: The m_lastImageId check in prepareMailbox is slightly too loose. It will incorrectly early exit even if m_lastImageId is already released by the compositor. This can happen when compositor loses its context, or when android webview switches modes. Stephen: Could you check my proposal in the comment is acceptable? Changjun: Please update the description to something local to Canvas2DLayerBridge.cpp https://codereview.chromium.org/101163004/diff/60001/Source/platform/graphics... File Source/platform/graphics/Canvas2DLayerBridge.cpp (right): https://codereview.chromium.org/101163004/diff/60001/Source/platform/graphics... Source/platform/graphics/Canvas2DLayerBridge.cpp:276: m_lastImageId = -1; We only want to do this if the released mailbox matches m_lastImageId. You can do this by comparing m_lastImageId to mailboxInfo->SkImage->uniqueID(), obviously with all the proper null checks in place. Also released here means calling Canvas2DLayerBridge::mailboxReleased (ie release back to us by compositor), not here, actually freeing the mailbox. So move this to Canvas2DLayerBridge::mailboxReleased. https://codereview.chromium.org/101163004/diff/60001/Source/platform/graphics... Source/platform/graphics/Canvas2DLayerBridge.cpp:370: MailboxInfo* mailboxInfo = createMailboxInfo(); Here, will need to check if there is already a mailbox with matching uniqueID. If there is, re-use the mailbox and not create another one.
On 2014/02/11 19:29:40, boliu wrote: > +senorblanco to advise since I'm not very familiar with this code myself +junov, who wrote the mailbox implementation.
On 2014/02/11 19:41:55, Stephen White wrote: > On 2014/02/11 19:29:40, boliu wrote: > > +senorblanco to advise since I'm not very familiar with this code myself > > +junov, who wrote the mailbox implementation. I disagree with this change, in some cases it will force perpetual mailbox updates on static canvases, which is highly undesirable.
Do you have a bug # for this change?
On 2014/02/11 21:19:17, junov wrote: > Do you have a bug # for this change? I think this can be folded into crbug.com/239864. On 2014/02/11 21:17:18, junov wrote: > On 2014/02/11 19:41:55, Stephen White wrote: > > On 2014/02/11 19:29:40, boliu wrote: > > > +senorblanco to advise since I'm not very familiar with this code myself > > > > +junov, who wrote the mailbox implementation. > > I disagree with this change, in some cases it will force perpetual mailbox > updates on static canvases, which is highly undesirable. You mean Patch Set 2, right? I agree it's not correct. How about the changes I suggested in the comments? Would that be acceptable?
On 2014/02/11 21:21:30, boliu wrote: > On 2014/02/11 21:19:17, junov wrote: > > Do you have a bug # for this change? > > I think this can be folded into crbug.com/239864. > > On 2014/02/11 21:17:18, junov wrote: > > On 2014/02/11 19:41:55, Stephen White wrote: > > > On 2014/02/11 19:29:40, boliu wrote: > > > > +senorblanco to advise since I'm not very familiar with this code myself > > > > > > +junov, who wrote the mailbox implementation. > > > > I disagree with this change, in some cases it will force perpetual mailbox > > updates on static canvases, which is highly undesirable. > > You mean Patch Set 2, right? I agree it's not correct. > > How about the changes I suggested in the comments? Would that be acceptable? When switching out of accelerated mode, if you want to resume normal operation with a software backed canvas, what you should do is call HTMLCanvasElement::discardImageBuffer() After doing that a new backing will be lazily created.
On 2014/02/11 21:28:12, junov wrote: > On 2014/02/11 21:21:30, boliu wrote: > > On 2014/02/11 21:19:17, junov wrote: > > > Do you have a bug # for this change? > > > > I think this can be folded into crbug.com/239864. > > > > On 2014/02/11 21:17:18, junov wrote: > > > On 2014/02/11 19:41:55, Stephen White wrote: > > > > On 2014/02/11 19:29:40, boliu wrote: > > > > > +senorblanco to advise since I'm not very familiar with this code myself > > > > > > > > +junov, who wrote the mailbox implementation. > > > > > > I disagree with this change, in some cases it will force perpetual mailbox > > > updates on static canvases, which is highly undesirable. > > > > You mean Patch Set 2, right? I agree it's not correct. > > > > How about the changes I suggested in the comments? Would that be acceptable? > > When switching out of accelerated mode, if you want to resume normal operation > with a software backed canvas, what you should do is call > HTMLCanvasElement::discardImageBuffer() > After doing that a new backing will be lazily created. But we don't want the canvas to switch to software mode in that case: https://code.google.com/p/chromium/issues/detail?id=332273#c11 The problem this is CL is trying to fix is this: Webview is in accelerated mode and a canvas is started in accelerated mode. The canvas is static. Then webview goes back to software-only mode, at this point, the mailbox is released by the compositor. Then webview goes into accelerated mode again, but prepareMailbox will never return true again due to the m_lastImageId check.
On 2014/02/11 21:37:41, boliu wrote: > The problem this is CL is trying to fix is this: Webview is in accelerated mode > and a canvas is started in accelerated mode. The canvas is static. Then webview > goes back to software-only mode, at this point, the mailbox is released by the > compositor. Then webview goes into accelerated mode again, but prepareMailbox > will never return true again due to the m_lastImageId check. Ah, that is easy to fix then. We should reset m_lastImageId whenever a switch from software back to hardware mode is detected. You can detect the switch in prepareMailbox.
> Ah, that is easy to fix then. We should reset m_lastImageId whenever a switch > from software back to hardware mode is detected. > You can detect the switch in prepareMailbox. Actually that is not a bulletproof solution. You can't detect the switch in cases where there were no calls to prepareMailbox while in software compositing mode. In mailboxReleased, we should detect whether the compositor just released the last remaining active mailbox. If so, then reset m_lastImageId.
On 2014/02/11 22:37:35, junov wrote: > > Ah, that is easy to fix then. We should reset m_lastImageId whenever a switch > > from software back to hardware mode is detected. > > You can detect the switch in prepareMailbox. > > Actually that is not a bulletproof solution. You can't detect the switch in > cases where there were no calls to prepareMailbox while in software compositing > mode. > > In mailboxReleased, we should detect whether the compositor just released the > last remaining active mailbox. If so, then reset m_lastImageId. That should work. Thanks! Changjun: Do this instead of what I said in the comments :)
On 2014/02/11 23:26:45, boliu wrote: > On 2014/02/11 22:37:35, junov wrote: > > > Ah, that is easy to fix then. We should reset m_lastImageId whenever a > switch > > > from software back to hardware mode is detected. > > > You can detect the switch in prepareMailbox. > > > > Actually that is not a bulletproof solution. You can't detect the switch in > > cases where there were no calls to prepareMailbox while in software > compositing > > mode. > > > > In mailboxReleased, we should detect whether the compositor just released the > > last remaining active mailbox. If so, then reset m_lastImageId. > > That should work. Thanks! > > Changjun: Do this instead of what I said in the comments :) Updated Patch Set 3, looks there is some display error in side-by-side diffs though... BTW, I am puzzled with this annotation: https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... How does the self-destruction be implemented here? Could you guys explain a bit more? Thanks a lot!
On 2014/02/12 15:31:00, changjun.yang wrote: > Updated Patch Set 3, looks there is some display error in side-by-side diffs > though... This is a codereview site problem. When this happens, just try git cl upload again, it should clear up after a few tries. > BTW, I am puzzled with this annotation: > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... > How does the self-destruction be implemented here? Could you guys explain a bit > more? Thanks a lot! I think it's referring to the layerTransientResourceAllocationChanged call. Maybe junov@ can chime in here. https://codereview.chromium.org/101163004/diff/190001/Source/platform/graphic... File Source/platform/graphics/Canvas2DLayerBridge.cpp (right): https://codereview.chromium.org/101163004/diff/190001/Source/platform/graphic... Source/platform/graphics/Canvas2DLayerBridge.cpp:448: This is not what junov@ said. We need to check that if *all* the mailboxes are released back by the compositor (ie m_status != MailboxInUse) Since there can be only a maximum of 4 mailboxes, just loop through all of them and check m_status. If all of them are not MailboxInUse, then reset m_lastImageId. Might want to refactor this into a private helper method, since we are already inside a loop here.
On 2014/02/12 19:12:22, boliu wrote: > On 2014/02/12 15:31:00, changjun.yang wrote: > > Updated Patch Set 3, looks there is some display error in side-by-side diffs > > though... > > This is a codereview site problem. When this happens, just try git cl upload > again, it should clear up after a few tries. > > > BTW, I am puzzled with this annotation: > > > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... > > How does the self-destruction be implemented here? Could you guys explain a > bit > > more? Thanks a lot! > > I think it's referring to the layerTransientResourceAllocationChanged call. > Maybe junov@ can chime in here. > > https://codereview.chromium.org/101163004/diff/190001/Source/platform/graphic... > File Source/platform/graphics/Canvas2DLayerBridge.cpp (right): > > https://codereview.chromium.org/101163004/diff/190001/Source/platform/graphic... > Source/platform/graphics/Canvas2DLayerBridge.cpp:448: > This is not what junov@ said. > > We need to check that if *all* the mailboxes are released back by the compositor > (ie m_status != MailboxInUse) > > Since there can be only a maximum of 4 mailboxes, just loop through all of them > and check m_status. If all of them are not MailboxInUse, then reset > m_lastImageId. Might want to refactor this into a private helper method, since > we are already inside a loop here. Hi Bo, I think this is the same implementation as junov@ mentioned, m_liveMailboxInfoIndex would not be equal with m_releasedMailboxInfoIndex unless the mailbox is the last one in use.However, this implementation is not as straightforward as the loop way, if you prefer the loop way, I may update then.
On 2014/02/13 02:27:30, changjun.yang wrote: > Hi Bo, I think this is the same implementation as junov@ mentioned, > m_liveMailboxInfoIndex would not be equal with m_releasedMailboxInfoIndex unless > the mailbox is the last one in use.However, this implementation is not as > straightforward as the loop way, if you prefer the loop way, I may update then. I think this assumes that 1) the compositor will have only one active mailbox, and 2) the compositor will release them in order, right? 1) is not true, and 2) is true for now, but I think it's a bad assumption to make.
On 2014/02/13 03:00:35, boliu wrote: > On 2014/02/13 02:27:30, changjun.yang wrote: > > Hi Bo, I think this is the same implementation as junov@ mentioned, > > m_liveMailboxInfoIndex would not be equal with m_releasedMailboxInfoIndex > unless > > the mailbox is the last one in use.However, this implementation is not as > > straightforward as the loop way, if you prefer the loop way, I may update > then. > > I think this assumes that 1) the compositor will have only one active mailbox, > and 2) the compositor will release them in order, right? 1) is not true, and 2) > is true for now, but I think it's a bad assumption to make. Updated with the loop way, please help to review. :)
Looks good. Please update the description. senorblanco/junov: PTAL
Please add a unit test for this in src/third_party/WebKit/Source/web/tests/Canvas2DLayerBridgeTest.cpp https://codereview.chromium.org/101163004/diff/420001/Source/platform/graphic... File Source/platform/graphics/Canvas2DLayerBridge.cpp (right): https://codereview.chromium.org/101163004/diff/420001/Source/platform/graphic... Source/platform/graphics/Canvas2DLayerBridge.cpp:464: for (mailboxInfo = m_mailboxes.begin(); mailboxInfo < m_mailboxes.end(); ++mailboxInfo) { You can do the check in O(1) time instead of O(N) if you add a new data member m_activeMailboxCount. You initialize it to zero, increment it in prepareMailbox, decrement in mailboxReleased.
I separately uploaded to https://codereview.chromium.org/166193004/ and see if I can get this through branch point... I looked into adding a unit test, but prepareMailbox assumes gpu backed RenderTarget, which is rather hard to mock out. Maybe another way is just skip all the GL calls if image->getTexture()? A condition that's only triggered in unit tests. Thoughts?
Actually, uploaded https://codereview.chromium.org/166093005/ instead of this. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
