STR: 1. Install Hex Race on an Aries device. [1] 2. Play it. Expected: Game is smooth. Actual: Game is jittery. This is a regression from the last 2 weeks, probably closer to the last week. [1] https://marketplace.firefox.com/app/hexgl-1

QA: Does this repro on Flame? If yes, could we get a regression window on it?

Hi Reporter, I try to repro this bug on latest Flame v2.0&2.1&2.2&3.0, but it shows white/black screen and user can't play this game after tapping the "START" button. Could you help to confirm whether the behavior is same as that you said: "Game is jittery"? Thank you very much. ------------------------------------------------------------------------------------------ Please see attachments: verify_v2.2.mp4, verify_v2.0&2.1&3.0.mp4 and logcat_v3.0_1650.txt. Reproduce rate: 6/6 STR: 1. Install Hex Race (v0.1) from Marketplace. 2. Open the game. 3. Tap "START" button and then tap the "TURN" or "ACCEL" icon for playing. **On Flame v2.0&2.1&3.0, it shows white/black screen and user can't play this game, after 1~2 mimutes, the game will exit automatically and return to the homescreen. **On Flame v2.2, it always shows white/black screen and user can't play this game.

Looks like we're not going to get much info from testing this on Flame. I know that this used to work on Aries. Could we get a regression window there?

Note from QA: We are depending on bug 1133074 to resolve before we can do regression windows on Aries.

NI Johan on getting regression windows on Aries.

We cut HexGL from the build, so this is no longer a blocker for Spark. However, we should really focus on getting this fixed, as the underlying issue may crop up in other places.

I saw the difference between a previous build and what we used to have. I'm trying to find an objective way to collect data (showing FPS on screen for instance), and then I'll go on the regression window.

The changes for Bug 1144906 seem to have caused this issue. Mozilla-inbound Regression Window Last Working Environmental Variables: Device: Aries 2.5 BuildID: 20150604221437 Gaia: 65369b217faac7d70c1a29100c4208c6d1db16e3 Gecko: c7720cbbe62e Gonk: 3af1ede0d0956cfbf9c549df7cd9a6807a9efdf2 Version: 41.0a1 (2.5) Firmware Version: D5803_23.1.A.1.28_NCB.ftf User Agent: Mozilla/5.0 (Mobile; rv:41.0) Gecko/41.0 Firefox/41.0 First Broken Environmental Variables: Device: Aries 2.5 BuildID: 20150604221642 Gaia: 65369b217faac7d70c1a29100c4208c6d1db16e3 Gecko: c4d1692d88ee Gonk: 3af1ede0d0956cfbf9c549df7cd9a6807a9efdf2 Version: 41.0a1 (2.5) Firmware Version: D5803_23.1.A.1.28_NCB.ftf User Agent: Mozilla/5.0 (Mobile; rv:41.0) Gecko/41.0 Firefox/41.0 Last Working gaia / First Broken gecko - Issue DOES occur Gaia: 65369b217faac7d70c1a29100c4208c6d1db16e3 Gecko: c4d1692d88ee First Broken gaia / Last Working gecko - Issue does NOT occur Gaia: 65369b217faac7d70c1a29100c4208c6d1db16e3 Gecko: c7720cbbe62e Gaia Pushlog: https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=c7720cbbe62e&tochange=c4d1692d88ee

Jeff, can you take a look at this please? This looks to have been caused by the work done for bug 1144906.

[Triage] Plus this due to it's break seriously.

Morris, please check if anything we can do for this bug?

I saw jittery on my foxfooding device, but didn't notice it in my custom build with latest m-c. Maybe this is resolved in recent patches?

Please check if this bug doesn't happen in latest build.

This issue is still happening on latest Aries. When playing the game moving the vehicle forward I can see the race track spasming back and forth. Device: Aries 2.5 BuildID: 20150818012543 Gaia: d9d99f32762975a370f1abd34a3512bd6fe29111 Gecko: 90d9b7c391d38ae118865bd87b5d011feee6dded Gonk: 2916e2368074b5383c80bf5a0fba3fc83ba310bd Version: 43.0a1 (2.5 Master) Firmware Version: D5803_23.1.A.1.28_NCB.ftf User Agent: Mozilla/5.0 (Mobile; rv:43.0) Gecko/43.0 Firefox/43.0

Comment on attachment 8655282 [details] [diff] [review] patch part2 - Change to gfx layers Review of attachment 8655282 [details] [diff] [review]: ----------------------------------------------------------------- The situation with SharedSurface and TextureClient (while not being as bad as it was a year ago) is still awful.

Comment on attachment 8655282 [details] [diff] [review] patch part2 - Change to gfx layers Review of attachment 8655282 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/layers/client/CanvasClient.cpp @@ +377,5 @@ > mFront->WaitForCompositorRecycle(); > } > } > +#else > + AutoRemoveTexture autoRemove(this); You *must* include comments here explaining why this is done, and why it's not not needed above.

Comment on attachment 8655280 [details] [diff] [review] patch part 1 - Change to SharedSurface Review of attachment 8655280 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/gl/SharedSurface.h @@ +199,5 @@ > } > > virtual bool ToSurfaceDescriptor(layers::SurfaceDescriptor* const out_descriptor) = 0; > + > + virtual layers::TextureClient* GetTextureClient() { This is confusing to add to the base class. I think it should only be added to the derived class. It must at least be obvious that it's only for getting the GrallocTextureClient. (Each ShSurf is owned by a TexClient. GetTextureClient sounds like it gives the backref for this relationship, when it doesn't)

(In reply to Jeff Gilbert [:jgilbert] from comment #22) > Comment on attachment 8655282 [details] [diff] [review] > patch part2 - Change to gfx layers > > Review of attachment 8655282 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: gfx/layers/client/CanvasClient.cpp > @@ +377,5 @@ > > mFront->WaitForCompositorRecycle(); > > } > > } > > +#else > > + AutoRemoveTexture autoRemove(this); > > You *must* include comments here explaining why this is done, and why it's > not not needed above. I'll update in a next patch.

(In reply to Jeff Gilbert [:jgilbert] from comment #23) > Comment on attachment 8655280 [details] [diff] [review] > patch part 1 - Change to SharedSurface > > Review of attachment 8655280 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: gfx/gl/SharedSurface.h > @@ +199,5 @@ > > } > > > > virtual bool ToSurfaceDescriptor(layers::SurfaceDescriptor* const out_descriptor) = 0; > > + > > + virtual layers::TextureClient* GetTextureClient() { > > This is confusing to add to the base class. I think it should only be added > to the derived class. > > It must at least be obvious that it's only for getting the > GrallocTextureClient. > > (Each ShSurf is owned by a TexClient. GetTextureClient sounds like it gives > the backref for this relationship, when it doesn't) I'll update in a next patch.

:jgilbert, can you review the patches?

Comment on attachment 8656356 [details] [diff] [review] patch part2 - Change to gfx layers (r=nical) Review of attachment 8656356 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/layers/client/CanvasClient.cpp @@ +380,5 @@ > +#else > + // Ensure to deliver FenceHandle from TextureHost to TextureClient, before > + // next TextureClient usage. It also works as TextureClient's recycling > + // timing controller. Hence, do not need to call WaitForCompositorRecycle(). > + AutoRemoveTexture autoRemove(this); Why/how does AutoRemoveTexture do what this comment says? This buffer juggling is very precarious. We need to understand every part of it.

(In reply to Jeff Gilbert [:jgilbert] from comment #29) > > Why/how does AutoRemoveTexture do what this comment says? > This buffer juggling is very precarious. We need to understand every part of > it. I looked into AutoRemoveTexture, its implementation has a problem. I filed Bug 1204895. I expected to AutoRemoveTexture the following 2 things. -[1] Call RemoveTexture() after newFront's "forwarder->UseTextures(this, textures)" call. It could improve performance on Host side. We could get better performance if GL texture's EGLImage binding is update by new EGLImage. Update GL texture binding to new TextureHost before old Texture's remove operation ensure it. -[2] Ensure to deliver fence from Host side to client side. Since android JB, fence handling became necessary. Therefore, before TextureClient reuse, TextureClient needs to receive fence from host side. attachment 8661254 [details] [diff] [review] ensures it.

Comment on attachment 8656356 [details] [diff] [review] patch part2 - Change to gfx layers (r=nical) Review of attachment 8656356 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/layers/client/CanvasClient.cpp @@ +380,5 @@ > +#else > + // Ensure to deliver FenceHandle from TextureHost to TextureClient, before > + // next TextureClient usage. It also works as TextureClient's recycling > + // timing controller. Hence, do not need to call WaitForCompositorRecycle(). > + AutoRemoveTexture autoRemove(this); Please expand on this comment.

Thanks, I will update the comment.