Closed Bug 1172719 Opened 4 years ago Closed 4 years ago

[Aries] Hex Race is very jittery

Categories

(Core :: Canvas: WebGL, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla44
blocking-b2g 2.5+
Tracking Status
firefox44 --- fixed
b2g-master --- affected

People

(Reporter: drs, Assigned: sotaro)

References

()

Details

(Keywords: dogfood, regression, Whiteboard: gfx-noted)

Attachments

(5 files, 4 obsolete files)

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?
Keywords: qawanted, regression
Whiteboard: gfx-noted
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.
Flags: needinfo?(drs)
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?
Flags: needinfo?(drs)
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.
Flags: needinfo?(jlorenzo)
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.
blocking-b2g: spark+ → 3.0?
Keywords: dogfood
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.
Flags: needinfo?(jlorenzo)
QA Contact: jlorenzo
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
QA Whiteboard: [QAnalyst-Triage?]
Flags: needinfo?(ktucker)
Jeff, can you take a look at this please? This looks to have been caused by the work done for bug 1144906.
Blocks: 1144906
QA Whiteboard: [QAnalyst-Triage?] → [QAnalyst-Triage+]
Flags: needinfo?(ktucker) → needinfo?(jgilbert)
[Triage] Plus this due to it's break seriously.
blocking-b2g: 2.5? → 2.5+
Morris, please check if anything we can do for this bug?
Flags: needinfo?(mtseng)
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?
Flags: needinfo?(mtseng)
Please check if this bug doesn't happen in latest build.
Keywords: qawanted
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
QA Whiteboard: [QAnalyst-Triage+] → [QAnalyst-Triage?]
Flags: needinfo?(ktucker)
Keywords: qawanted
QA Whiteboard: [QAnalyst-Triage?] → [QAnalyst-Triage+]
Flags: needinfo?(ktucker)
Assignee: nobody → sotaro.ikeda.g
Attachment #8655281 - Attachment is obsolete: true
Attachment #8655280 - Flags: review?(jgilbert)
Attachment #8655282 - Flags: review?(nical.bugzilla)
Attachment #8655282 - Flags: review?(jgilbert)
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.
Attachment #8655282 - Flags: review?(nical.bugzilla) → review+
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.
Attachment #8655282 - Flags: review?(jgilbert) → review-
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)
Attachment #8655280 - Flags: review?(jgilbert) → review-
(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.
Attachment #8655280 - Attachment is obsolete: true
Attachment #8655282 - Attachment is obsolete: true
Attachment #8656349 - Flags: review?(jgilbert)
Attachment #8656356 - Flags: review?(jgilbert)
See Also: → 988160
:jgilbert, can you review the patches?
Flags: needinfo?(jgilbert)
Attachment #8656349 - Flags: review?(jgilbert) → review+
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.
Flags: needinfo?(sotaro.ikeda.g)
Flags: needinfo?(jgilbert)
Depends on: 1204895
(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.
Flags: needinfo?(sotaro.ikeda.g)
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.
Attachment #8656356 - Flags: review?(jgilbert) → review+
Thanks, I will update the comment.
Attachment #8656356 - Attachment is obsolete: true
Attachment #8663672 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/f00e666185eb
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
Blocks: 1217740
You need to log in before you can comment on or make changes to this bug.