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

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

[Sotaro Ikeda [:sotaro]]

+++ 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.

Comment 1

[Sotaro Ikeda [:sotaro]]

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

Comment 2

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Allocate CompositableBackendSpecificData for TiledLayerBufferComposite

Comment 3

[Sotaro Ikeda [:sotaro]]

After applying attachment 8445308 [details] [diff] [review], fEGLImageTargetTexture2D() appear only in a few samples.
http://people.mozilla.org/~bgirard/cleopatra/#report=f5e37d1fd7d293408e485a6e8dcec84c34b6a203

Comment 4

[Sotaro Ikeda [:sotaro]]

attachment 8445308 [details] [diff] [review] causes genlock failure on master hamachi :-(

Comment 5

[Sotaro Ikeda [:sotaro]]

Attachment: patch ver 2 - Allocate CompositableBackendSpecificData for TiledLayerBufferComposite

Fix genlock failure on hamachi.

Comment 6

[Sotaro Ikeda [:sotaro]]

https://tbpl.mozilla.org/?tree=Try&rev=c2c49ee57375

Comment 7

[Sotaro Ikeda [:sotaro]]

(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.

Comment 8

[Sotaro Ikeda [:sotaro]]

https://tbpl.mozilla.org/?tree=Try&rev=2e74a501146c

Comment 9

[Sotaro Ikeda [:sotaro]]

https://tbpl.mozilla.org/?tree=Try&rev=1a8b46c15307

Comment 10

[Nicolas Silva [:nical]]

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?

Comment 11

[Sotaro Ikeda [:sotaro]]

Current patch is too complex, it seems better to do like attachment 8467074 [details] [diff] [review] in Bug 1015984.

Comment 12

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Allocate CompositableBackendSpecificData for TiledLayerBufferComposite

Update the patch based on Bug 1017351.

Comment 13

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Allocate CompositableBackendSpecificData for TiledLayerBufferComposite

Add pointer check and textureOnWhite handling. This pach causes the genlock failure on hamachi.

Comment 14

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Allocate CompositableBackendSpecificData for TiledLayerBufferComposite

Fix genlock failure.

Comment 15

[Sotaro Ikeda [:sotaro]]

(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.

Comment 16

[Sotaro Ikeda [:sotaro]]

CompositableClient's lifetime is determined by a reference count. a reference to CompositableClient need to be kept until RemoveTextureFromCompositableAsync() is sent to parent side.

Comment 17

[Sotaro Ikeda [:sotaro]]

(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.

Comment 18

[Sotaro Ikeda [:sotaro]]

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.

Comment 19

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Allocate CompositableBackendSpecificData for TiledLayerBufferComposite

Fix stale PCompositableChild problem.

Comment 20

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Allocate CompositableBackendSpecificData for TiledLayerBufferComposite

Un-bitrot.

Comment 21

[Sotaro Ikeda [:sotaro]]

https://tbpl.mozilla.org/?tree=Try&rev=78f247313100

Comment 22

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Allocate CompositableBackendSpecificData for TiledLayerBufferComposite

Remove incorrect assert check.

Comment 23

[Sotaro Ikeda [:sotaro]]

https://tbpl.mozilla.org/?tree=Try&rev=60a98c31fdd1

Comment 24

[Sotaro Ikeda [:sotaro]]

(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 25

[Sotaro Ikeda [:sotaro]]

Comment 24 seems not cause a big problem. It is a back buffer only. Therefore there seems not urgent necessity to uplift its fix.

Comment 26

[Sotaro Ikeda [:sotaro]]

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.

Comment 27

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Allocate CompositableBackendSpecificData for TiledLayerBufferComposite

Remove RemoveTextureFromCompositableTracker usage for back buffer discard.

Comment 28

[Sotaro Ikeda [:sotaro]]

(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.

Comment 29

[Sotaro Ikeda [:sotaro]]

https://tbpl.mozilla.org/?tree=Try&rev=62f6c51c0f8c

Comment 30

[Sotaro Ikeda [:sotaro]]

(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.

Comment 31

[Sotaro Ikeda [:sotaro]]

(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().

Comment 32

[Sotaro Ikeda [:sotaro]]

(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.

Comment 33

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Allocate CompositableBackendSpecificData for TiledLayerBufferComposite

Fix transaction problem.

Comment 34

[Sotaro Ikeda [:sotaro]]

https://tbpl.mozilla.org/?tree=Try&rev=a5411909c5a2

Comment 35

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Allocate CompositableBackendSpecificData for TiledLayerBufferComposite

Fix tryserver test failures.

Comment 36

[Sotaro Ikeda [:sotaro]]

https://tbpl.mozilla.org/?tree=Try&rev=5730b887e89a

Comment 37

[Sotaro Ikeda [:sotaro]]

The patch might wait TextureHost and TextureSource refactoring. Nical is working for it. It is described at the following.
 https://etherpad.mozilla.org/texturesource

Comment 38

[Sotaro Ikeda [:sotaro]]

Attachment: patch - reduce fEGLImageTargetTexture2D() on TiledLayer

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.

Comment 39

[Sotaro Ikeda [:sotaro]]

Attachment: patch - reduce fEGLImageTargetTexture2D() on TiledLayer

Fix one gen lock problem. But still some genlock failure exist.

Comment 40

[Sotaro Ikeda [:sotaro]]

Attachment: patch - reduce fEGLImageTargetTexture2D() on TiledLayer

Fixed one genlock failure cause. But still other causes exist.

Comment 41

[Sotaro Ikeda [:sotaro]]

Attachment: patch - reduce fEGLImageTargetTexture2D() on TiledLayer

Fix genlock failure. The all genlock failures seems to be addressed.

Comment 42

[Sotaro Ikeda [:sotaro]]

https://tbpl.mozilla.org/?tree=Try&rev=4ea3677e5035
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=4ea3677e5035

Comment 43

[Sotaro Ikeda [:sotaro]]

(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.

Comment 44

[Sotaro Ikeda [:sotaro]]

Attachment: patch - reduce fEGLImageTargetTexture2D() on TiledLayer

Fix test failures.

Comment 45

[Sotaro Ikeda [:sotaro]]

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=70de7327fd1c
https://tbpl.mozilla.org/?tree=Try&rev=70de7327fd1c

Comment 46

[Nicolas Silva [:nical]]

Comment on attachment 8535825 [details] [diff] [review]
patch - reduce fEGLImageTargetTexture2D() on TiledLayer

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

Nice.

Comment 47

[Sotaro Ikeda [:sotaro]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8f22366d5435

Comment 48

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/8f22366d5435