1015984 - [OGL] Sharing the same image across multiple image layers causes the EGLImage to be recreated constantly

Status: RESOLVED INVALID

(Core :: Graphics: Layers, defect, P1)

Description

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: transPerfMod2.zip

With multiples layers on the screen, it seems like the compositor has an hard time keeping up and reaching 60fps. On the flame device it looks more 10fps.

Step to reproduce:
 - On a flame install the attached app
 - launch the app, and click on the screen to start the animation

I have heard a few times that the number of draw calls could be an issue when there is multiple layers on screen animation at the same time. CC'ing Roc as that's from him that I heard the word Draw Call for the first time, and CC'ing Nikal as he told me some similar words today as soon as I showned him the demo.

Comment 1

[Kevin Grandon :kgrandon]

Feels also like what we are looking for in bug 1010584. Feel free to dupe that against this one if you'd like.

Comment 2

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Jukka had some good comments based on looking at GL traces in bug 1011017 comment 8 as well that are likely relevant here.

We should have no problem compositing this at 60fps.  I'm going to add this testcase to the gfx perf test app as well.

Comment 3

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

From a quick glance at the code, some observations:

- the images have 1280x720 in the name, but they're really only 32x18 (this is good)
- the images are animated using CSS animation, triggered by a change in the transform property.  Subsequent animations are triggered off 'transitionend'.

The images are just normal <img> elements.  I'm assuming that layers are being created for each of them since they have transform, will-change: transform, etc., but that would be good to confirm.  If they are, there is no reason why this shouldn't hit 60fps.

Comment 4

[Andreas Gal :gal]

Can someone attach a profile?

Comment 5

[Milan Sreckovic [:milan] (needinfo for best results)]

We're tracking this in a few different bugs.  Haven't finished the investigation, but BenWa has seen us be limited to ~100 draw calls on the Flame if we want to keep 60fps.

Comment 6

[Benoit Girard (:BenWa)]

Can we get a profile attached with performance bugs? We're already talking about draw calls (and it likely is) but really we don't know yet. This eliminates all the initial guess work.

Use the following option:
./profile.sh start -p b2g -t Compositor -f stackwalk,threads && ./profile.sh -p APP -f stackwalk,js

(In reply to Milan Sreckovic [:milan] from comment #5)
> We're tracking this in a few different bugs.  Haven't finished the
> investigation, but BenWa has seen us be limited to ~100 draw calls on the
> Flame if we want to keep 60fps.

It's in that range but I'm measuring going through our Compositor abstraction. It's possible that lowering our OGL overhead will improve this number.

Comment 7

[Andreas Gal :gal]

I saw a lot of restyling in the profile. Please attach profile.

Comment 8

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Here is a profile: http://people.mozilla.org/~bgirard/cleopatra/#report=221a61f50bb0ffaa1c107e1a1c5b5f2046eb80bb

Comment 9

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to Vivien Nicolas (:vingtetun) (:21) (NOT reading bugmails, needinfo? please) from comment #8)
> Here is a profile:
> http://people.mozilla.org/~bgirard/cleopatra/
> #report=221a61f50bb0ffaa1c107e1a1c5b5f2046eb80bb

In the middle of the profile the animation was stopped for a small time. I restarted it quickly.

Comment 10

[Andreas Gal :gal]

I see a ton of layer dumping in the profile. Do you have some DEBUG flag enabled?

Comment 11

[Andreas Gal :gal]

I am almost 100% certain you have layers.dump set. Please set that to false and attach another profile. Thanks!

Comment 12

[Benoit Girard (:BenWa)]

The restyle problem is certainly bug 931668.

I did some profiling today and found bug 1017351 which is causing us to have a 20 FPS, fixing that (or disable image->thebes promotions) bring this up to 35 FPS.

Fixing 1017353 will reduce jank with incoming updates and stabilize frame rate a bit.

Once all of this is fixed well be sitting at 35-40 FPS, we will still need to investigate why we can't keep up with 120 layers when we should get about a tad over 200 on the flame.

Comment 13

[Kevin Grandon :kgrandon]

Milan - is this impossible for 2.0?  Just asking because this is blocking bug 1015984, and I think that's what we need for the vertical homescreen to have a really nice editing experience.

Comment 14

[Benoit Girard (:BenWa)]

(In reply to Kevin Grandon :kgrandon from comment #13)
> Milan - is this impossible for 2.0?  Just asking because this is blocking
> bug 1015984, and I think that's what we need for the vertical homescreen to
> have a really nice editing experience.

We don't know that these share the same root problem. This problem is very specific to reusing the same TextureHost for image layers. The vertical homescreen may not be using Image Layer or reusing the same TextureHost and may not be effected by this issue of this bug in this very specific test case.

If we want to optimize homescreen editing experience I suggest that we do a separate investigation.

Comment 15

[Kevin Grandon :kgrandon]

Thanks for the info. The reason ask is because I filed bug 1010584 for the vertical homescreen and was told that the actionable items for the platform were contained within this bug. If this is not the case then perhaps we should unblock bug 1010584 from this?

Comment 16

[Benoit Girard (:BenWa)]

It may or may not be accurate, I don't know.

IMO the bar for making this block bug 1010584 is fairly low because the issues are similar, we want to fix both and correlating the two is useful. However the bar for 'let's rush this for 2.0' is much higher. In the latter I would want to be sure it would fix the problem we consider rushing the bug for.

Comment 17

[Milan Sreckovic [:milan] (needinfo for best results)]

Conversation offline on the scheduling topic.  Will post the conclusions.

Comment 18

[Ivan Tsay (:ITsay)]

Link to graphic FFOS 2.1 eature meta for tracking.

Comment 19

[Anders Fridlund]

Attachment: reuse_images.diff

When profiling this problem I noticed that a lot of time was spent deleting and creating images in GrallocTextureSourceOGL::SetCompositableBackendSpecificData.
Reusing the images have increased the composition rate significantly for me. Are there any specific reasons why we shouldn't resuse mEGLImage instead of deleting and create a new one?
(Attaching some code to clarify my reasoning)

Comment 20

[Chris Lord [:cwiiis]]

(In reply to Anders Fridlund from comment #19)
> Created attachment 8467074 [details] [diff] [review]
> reuse_images.diff
> 
> When profiling this problem I noticed that a lot of time was spent deleting
> and creating images in
> GrallocTextureSourceOGL::SetCompositableBackendSpecificData.
> Reusing the images have increased the composition rate significantly for me.
> Are there any specific reasons why we shouldn't resuse mEGLImage instead of
> deleting and create a new one?
> (Attaching some code to clarify my reasoning)

I think we should, and we do for tiled layers, as best we can at least. nical had a patch to get other layers to use the same mechanism as tiled layers, but due to them not using them in quite the same way as tiled layers, it would cause deadlock. This work lived on bug 980647.

Comment 21

[Nicolas Silva [:nical]]

Deleting an EGLImage ensures that the galloc buffer it is attached to gets unlocked. As Chris pointed out we tried in bug 980647 and there are still a few issues to figure out. Note that you will probably not have the same behaviour depending on the version of the android kernel. IIRC the issues we had with unlocking gralloc buffer in bug 980647 happen when the device uses libgenlock. Now we also have phones that use fences for synchronization instead of genlock so the problem might not happen there or might happen differently. So we need to keep this in mind when experimenting with those things.

Comment 22

[Sotaro Ikeda [:sotaro]]

attachment 8467074 [details] [diff] [review] seems to have a gen lock failure risk on ics devices. I tried to test the patch on latest master hamachi. But latest master hamachi does not start on my build :-(

Comment 23

[Sotaro Ikeda [:sotaro]]

(In reply to Sotaro Ikeda [:sotaro] from comment #22)
> attachment 8467074 [details] [diff] [review] seems to have a gen lock
> failure risk on ics devices. I tried to test the patch on latest master
> hamachi. But latest master hamachi does not start on my build :-(

Sorry, this was my misunderstanding. On ImageLayer, when TextureHost has CompositableBackendSpecificData means, the TextureHost is used by CompositableHost. So, there is no risk of gen lock failure. I also tried on hamachi device. genlock failure does not happened.

I also created similar patch in Bug 1017351 in the past. The patch was unnecessary complex. attachment 8467074 [details] [diff] [review] is better!

Comment 24

[Sotaro Ikeda [:sotaro]]

(In reply to Sotaro Ikeda [:sotaro] from comment #23)
> (In reply to Sotaro Ikeda [:sotaro] from comment #22)
> > attachment 8467074 [details] [diff] [review] seems to have a gen lock
> > failure risk on ics devices. I tried to test the patch on latest master
> > hamachi. But latest master hamachi does not start on my build :-(
> 
> Sorry, this was my misunderstanding. On ImageLayer, when TextureHost has
> CompositableBackendSpecificData means, the TextureHost is used by
> CompositableHost.

The above becomes always valid since AsyncTransactionTracker is used in gfx laysers on gonk.

Comment 25

[Sotaro Ikeda [:sotaro]]

Sorry, comment 23 and comment 24 was wrong. Today was a first works day since summer vacation. I had forgotten what I thought before :-(

On gonk, each CompositableHost except tiled layers has one OpneGL texture. The openGL texture is wrapped by CompositableBackendSpecificData. Change CompositableBackendSpecificData means change CompositableHost and OpenGL texture.

Current gfx layers code does not expect a code like attachment 8467074 [details] [diff] [review]. CompositableDataGonkOGL::BindEGLImage() seems not call correctly  call  gl()->fEGLImageTargetTexture2D() in OpenGL texture changed case. In theory, it could cause the the previous gralloc buffer's unbinding failure. And could cause the genlock failure (on ICS) or drawing curruption (since on JB). And fixing the CompositableDataGonkOGL::BindEGLImage() cause more gl()->fEGLImageTargetTexture2D() calls.

I created attachment 8432575 [details] [diff] [review] in Bug 1017351 to fix the above problems. But attachment 8432575 [details] [diff] [review] is messy, need to simplify more.



gl()->fEGLImageTargetTexture2D()

Comment 26

[Anders Fridlund]

(In reply to Sotaro Ikeda [:sotaro] from comment #25)
> Sorry, comment 23 and comment 24 was wrong. Today was a first works day
> since summer vacation. I had forgotten what I thought before :-(
> 
> On gonk, each CompositableHost except tiled layers has one OpneGL texture.
> The openGL texture is wrapped by CompositableBackendSpecificData. Change
> CompositableBackendSpecificData means change CompositableHost and OpenGL
> texture.
> 
> Current gfx layers code does not expect a code like attachment 8467074 [details] [diff] [review]
> [details] [diff] [review]. CompositableDataGonkOGL::BindEGLImage() seems not
> call correctly  call  gl()->fEGLImageTargetTexture2D() in OpenGL texture
> changed case. In theory, it could cause the the previous gralloc buffer's
> unbinding failure. And could cause the genlock failure (on ICS) or drawing
> curruption (since on JB). And fixing the
> CompositableDataGonkOGL::BindEGLImage() cause more
> gl()->fEGLImageTargetTexture2D() calls.
> 
> I created attachment 8432575 [details] [diff] [review] in Bug 1017351 to fix
> the above problems. But attachment 8432575 [details] [diff] [review] is
> messy, need to simplify more.
> 
> 
> 
> gl()->fEGLImageTargetTexture2D()

Great! I had a quick try with attachment 8432575 [details] [diff] [review] and it seems to improve performance even more than attachment 8467074 [details] [diff] [review] (for this specific case anyway). Is this fix something that you expect to commit soon?

Comment 27

[Sotaro Ikeda [:sotaro]]

I am going to for Bug 1017351 from tomorrow.

By the way bug 1010966 is a similar bug about tiled layers.

Comment 28

[Bobby Chien]

[Tracking Requested - why for this release]: tracking backlog for future release.