Closed Bug 1010966 Opened 7 years ago Closed 7 years ago

Reduce gl()->fEGLImageTargetTexture2D() call from tiled layer on gonk

Categories

(Core :: Graphics: Layers, defect)

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla37

People

(Reporter: sotaro, Assigned: sotaro)

References

Details

(Keywords: perf)

Attachments

(1 file, 15 obsolete files)

11.71 KB, patch
nical
: review+
Details | Diff | Splinter Review
+++ This bug was initially created as a clone of Bug #1005908 +++

Bug 1005908 is going to reduce gl()->fEGLImageTargetTexture2D() call from layers except tiled layer. This bug is for reducing gl()->fEGLImageTargetTexture2D() call from tiled layer on gonk.
Assignee: nobody → sotaro.ikeda.g
Severity: blocker → normal
Priority: P1 → --
Depends on: 1005908
Status: NEW → ASSIGNED
On master nexus-5 with bug 1028532 fix, inSetting app scrolling use case, gl()->fEGLImageTargetTexture2D() seems to spend 13% of Compositor thread's time g.

http://people.mozilla.org/~bgirard/cleopatra/#report=45ef24dfa26732e1ef9250b57c4931130ffd4ed2
Depends on: 1027088
attachment 8445308 [details] [diff] [review] causes genlock failure on master hamachi :-(
Fix genlock failure on hamachi.
Attachment #8445308 - Attachment is obsolete: true
(In reply to Sotaro Ikeda [:sotaro] from comment #6)
> https://tbpl.mozilla.org/?tree=Try&rev=c2c49ee57375

tryserver has some failures. It is not clear that it is caused by the patch.
Attachment #8452555 - Flags: review?(nical.bugzilla)
Comment on attachment 8452555 [details] [diff] [review]
patch ver 2 - Allocate CompositableBackendSpecificData for TiledLayerBufferComposite

Review of attachment 8452555 [details] [diff] [review]:
-----------------------------------------------------------------

GrallocTextureHostOGL is becoming quite a mess, it's hard to follow what happens in which case given the multiplication of states and the way we do thing either in the Host, the Source or the BackendSpecificData depending on some mebers being set or not, and how the 3 interact (it's not a rant about your patch, but it makes reviewing tricky. We should look into simplifying/documenting these things at some point).
A lot of tuning and experimentation went into using the compositor's temporary textures pool for tiles on b2g. Are you certain this strategy is better? If so, can you explain why, is it because of how fences work (we were testing with genlock back then)? Does this mean this will make things better with fences and worse with genlock?
Attachment #8452555 - Flags: review?(nical.bugzilla)
Current patch is too complex, it seems better to do like attachment 8467074 [details] [diff] [review] in Bug 1015984.
Depends on: 1017351
Update the patch based on Bug 1017351.
Attachment #8452555 - Attachment is obsolete: true
Add pointer check and textureOnWhite handling. This pach causes the genlock failure on hamachi.
Attachment #8496040 - Attachment is obsolete: true
Fix genlock failure.
Attachment #8496070 - Attachment is obsolete: true
(In reply to Sotaro Ikeda [:sotaro] from comment #14)
> Created attachment 8496158 [details] [diff] [review]
> patch - Allocate CompositableBackendSpecificData for
> TiledLayerBufferComposite
> 
> Fix genlock failure.

It cause occasional application crash because of stale PCompositable.
CompositableClient's lifetime is determined by a reference count. a reference to CompositableClient need to be kept until RemoveTextureFromCompositableAsync() is sent to parent side.
(In reply to Sotaro Ikeda [:sotaro] from comment #16)
> CompositableClient's lifetime is determined by a reference count. a
> reference to CompositableClient need to be kept until
> RemoveTextureFromCompositableAsync() is sent to parent side.

In the TiledContentClient's destructor, RemoveTextureFromCompositableAsync() is called.
On tiled layer, the buffers that is hold during TiledContentClient destruction also recycled to TextureClientPool. This is a different point compared to other layers. Even during the destruction, need to ensure RemoveTextureFromCompositableAsync() can be done.
Fix stale PCompositableChild problem.
Attachment #8496158 - Attachment is obsolete: true
Attachment #8496251 - Flags: review?(nical.bugzilla)
Un-bitrot.
Attachment #8496251 - Attachment is obsolete: true
Attachment #8496251 - Flags: review?(nical.bugzilla)
Attachment #8496353 - Flags: review?(nical.bugzilla)
Remove incorrect assert check.
Attachment #8496353 - Attachment is obsolete: true
Attachment #8496353 - Flags: review?(nical.bugzilla)
Attachment #8496418 - Flags: review?(nical.bugzilla)
(In reply to Sotaro Ikeda [:sotaro] from comment #23)
> https://tbpl.mozilla.org/?tree=Try&rev=60a98c31fdd1

b2g debugs are failed by the following. This is a regression caused by Bug 1044241. It call TileClient::DiscardBackBuffer() out side of layer transaction but it requests LayerTransaction.

> [Child 776] ###!!! ABORT: forgot BeginTransaction?: '!Finished()', file ../../../gecko/gfx/layers/ipc/ShadowLayers.cpp, line 87

This regression is not recognized because this code path is not enabled on ICS. And tryserver runt b2g gonk test only on ICS.
Comment 24 seems not cause a big problem. It is a back buffer only. Therefore there seems not urgent necessity to uplift its fix.
I misunderstood how to discard back buffer. tiled layer only forward front buffer to Parent side. Therefore there is no necessity to use RemoveTextureFromCompositableTracker to discard back buffer.
Remove RemoveTextureFromCompositableTracker usage for back buffer discard.
Attachment #8496418 - Attachment is obsolete: true
Attachment #8496418 - Flags: review?(nical.bugzilla)
(In reply to Sotaro Ikeda [:sotaro] from comment #25)
> Comment 24 seems not cause a big problem. It is a back buffer only.
> Therefore there seems not urgent necessity to uplift its fix.

There seems a possibility that could cause deadlock. It might better to handle this as a different bug.
Depends on: 1073862
(In reply to Sotaro Ikeda [:sotaro] from comment #29)
> https://tbpl.mozilla.org/?tree=Try&rev=62f6c51c0f8c

b2g debug tests were still failed. There is another path that request ShadowLayerForwarder::RemoveTextureFromCompositableAsync() out side of LayerTransaction.
(In reply to Sotaro Ikeda [:sotaro] from comment #30)
> (In reply to Sotaro Ikeda [:sotaro] from comment #29)
> > https://tbpl.mozilla.org/?tree=Try&rev=62f6c51c0f8c
> 
> b2g debug tests were still failed. There is another path that request
> ShadowLayerForwarder::RemoveTextureFromCompositableAsync() out side of
> LayerTransaction.

ClientTiledPaintedLayer is destructed after LayerTransaction in nsDisplayList::PaintForFrame().
(In reply to Sotaro Ikeda [:sotaro] from comment #31)
> ClientTiledPaintedLayer is destructed after LayerTransaction in
> nsDisplayList::PaintForFrame().

The above is not the only problem. ClearCachedResources() is always called from outside of a layer transaction. In both cases, CompositableClient is also destruted. Then it makes RemoveTextureFromCompositableTracker's state to complete. Therefore they are not possible cause of deadlock like Bug 1073862.
Fix transaction problem.
Attachment #8496458 - Attachment is obsolete: true
Fix tryserver test failures.
Attachment #8497167 - Attachment is obsolete: true
Attachment #8497419 - Flags: review?(nical.bugzilla)
Blocks: 1067455
Depends on: 1075136
No longer blocks: 1067455
Depends on: 1076868
Attachment #8497419 - Flags: review?(nical.bugzilla)
The patch might wait TextureHost and TextureSource refactoring. Nical is working for it. It is described at the following.
 https://etherpad.mozilla.org/texturesource
Since Bug 1077301 fix, architecture was changed. The patch implement based on the new architecture. It seems works on flame-kk. But on hamachi, it causes genlock failure.
Attachment #8497419 - Attachment is obsolete: true
Fix one gen lock problem. But still some genlock failure exist.
Attachment #8535139 - Attachment is obsolete: true
Fixed one genlock failure cause. But still other causes exist.
Attachment #8535212 - Attachment is obsolete: true
Fix genlock failure. The all genlock failures seems to be addressed.
Attachment #8535341 - Attachment is obsolete: true
(In reply to Sotaro Ikeda [:sotaro] from comment #42)
> https://tbpl.mozilla.org/?tree=Try&rev=4ea3677e5035

B2G ICS Emulator Debug has a lot of test failure.
Fix test failures.
Attachment #8535667 - Attachment is obsolete: true
Attachment #8535825 - Flags: review?(nical.bugzilla)
Comment on attachment 8535825 [details] [diff] [review]
patch - reduce fEGLImageTargetTexture2D() on TiledLayer

Review of attachment 8535825 [details] [diff] [review]:
-----------------------------------------------------------------

Nice.
Attachment #8535825 - Flags: review?(nical.bugzilla) → review+
https://hg.mozilla.org/mozilla-central/rev/8f22366d5435
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
You need to log in before you can comment on or make changes to this bug.