Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please) Reporter
	Description • 10 years ago

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.

	Kevin Grandon :kgrandon
	Comment 1 • 10 years ago

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

	Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)
	Comment 2 • 10 years ago

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.

	Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)
	Comment 3 • 10 years ago

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.

	Andreas Gal :gal
	Comment 4 • 10 years ago

Can someone attach a profile?

	Milan Sreckovic [:milan] (needinfo for best results)
	Comment 5 • 10 years ago

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.

	Benoit Girard (:BenWa)
	Comment 6 • 10 years ago

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.

	Andreas Gal :gal
	Comment 7 • 10 years ago

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

	Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please) Reporter
	Comment 8 • 10 years ago

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

	Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please) Reporter
	Comment 9 • 10 years ago

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

	Andreas Gal :gal
	Comment 10 • 10 years ago

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

	Andreas Gal :gal
	Comment 11 • 10 years ago

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

	Benoit Girard (:BenWa)
	Comment 12 • 10 years ago

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.

	Kevin Grandon :kgrandon
	Comment 13 • 10 years ago

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.

	Benoit Girard (:BenWa)
	Comment 14 • 10 years ago

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

	Kevin Grandon :kgrandon
	Comment 15 • 10 years ago

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?

	Benoit Girard (:BenWa)
	Comment 16 • 10 years ago

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.

	Milan Sreckovic [:milan] (needinfo for best results)
	Comment 17 • 10 years ago

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

	Ivan Tsay (:ITsay)
	Comment 18 • 10 years ago

Link to graphic FFOS 2.1 eature meta for tracking.

	Anders Fridlund
	Comment 19 • 10 years ago

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)

	Chris Lord [:cwiiis]
	Comment 20 • 10 years ago

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

	Nicolas Silva [:nical]
	Comment 21 • 10 years ago

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.

	Sotaro Ikeda [:sotaro]
	Comment 22 • 10 years ago

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

	Sotaro Ikeda [:sotaro]
	Comment 23 • 10 years ago

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

	Sotaro Ikeda [:sotaro]
	Comment 24 • 10 years ago

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

	Sotaro Ikeda [:sotaro]
	Comment 25 • 10 years ago

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

	Anders Fridlund
	Comment 26 • 10 years ago

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

	Sotaro Ikeda [:sotaro]
	Comment 27 • 10 years ago

I am going to for Bug 1017351 from tomorrow.

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

	Bobby Chien
	Comment 28 • 10 years ago

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