848481 - [SkiaGL] Flickering with canvas

Status: RESOLVED FIXED

(Core :: Graphics, defect, P1)

Description

[George Wright (:gw280) (needinfo me!)]

When SkiaGL is enabled for canvas, there's substantial flickering. This is most visible on Android/GUIMark2 Bitmap, but I've observed it on Linux as well.

Comment 1

[James Willcox (:snorp) ([e-mail]) (he/him)]

George, what device did you see this on? I know I saw this a while back but I can't remember where now. The GUIMark2 HTML5 gaming test doesn't flicker on my Nexus 4.

Comment 2

[George Wright (:gw280) (needinfo me!)]

Try the desktop version of GUIMark2 bitmap (http://www.craftymind.com/factory/guimark2/HTML5GamingTest.html). I've seen it on both Linux/desktop and on my ASUS Eee Pad Infinity (much worse on the infinity).

Comment 3

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: ♫ Don't say ClientWaitSync unless FOREVER♫

After much debugging I arrived to the conclusion that on the affected NVIDIA drivers, ClientWaitSync ignored its timeout parameter unless it is the EGL_FOREVER symbolic constant.

Then Vlad forwarded me a conversation had with NVIDIA confirming exactly that as a known issue that they had resolved in their newest drivers.

I think that waiting FOREVER makes sense here anyway. It's not great, but flickering isn't either. Happy to see a more perfect solution worked out in a future bug.

Comment 4

[Kelsey Gilbert [:jgilbert]]

(In reply to Benoit Jacob [:bjacob] from comment #3)
> Created attachment 745228 [details] [diff] [review]
> ♫ Don't say ClientWaitSync unless FOREVER♫
> 
> After much debugging I arrived to the conclusion that on the affected NVIDIA
> drivers, ClientWaitSync ignored its timeout parameter unless it is the
> EGL_FOREVER symbolic constant.
> 
> Then Vlad forwarded me a conversation had with NVIDIA confirming exactly
> that as a known issue that they had resolved in their newest drivers.
> 
> I think that waiting FOREVER makes sense here anyway. It's not great, but
> flickering isn't either. Happy to see a more perfect solution worked out in
> a future bug.

The API is supposed to work fine even with a timeout of 0. We should be detecting that it returned something other than EGL_CONDITION_SATISFIED, and not swap in the new frame. This probably doesn't play well with the rest of the layers code, though.

Functionally, waiting up to FOREVER isn't different than Finish, I suppose. Still, having a timeout would be nice from an interactivity point of view.

Really, we should be testing for (and using, where available) the EGL_KHR_wait_sync extension.

Comment 5

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 745228 [details] [diff] [review]
♫ Don't say ClientWaitSync unless FOREVER♫

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

The explanatory comment needs work, but functionally fine.

::: gfx/gl/SharedSurfaceEGL.cpp
@@ +189,5 @@
>  
> +    // Wait FOREVER, because
> +    //  - 1. This is pretty much what we actually want; and
> +    //  - 2. Some NVIDIA (both Tegra and Geforce) drivers have ClientWaitSync returning
> +    //       immediately if the timeout delay is anything else than FOREVER.

I don't believe this is really what we want. We'd prefer just delay the composite of the new frame until the webgl frame is complete. We should be able to do this by leaving the old Consumer buffer in place, and leaving the dirty bit set.

We should explore this in another bug, but I think this pair of comments needs to be corrected to match what the actual problem is.

Also, we should really just do this for NV, if that's the only place the problem is. I'll accept this patch for now, but we should do better later.

Comment 6

[George Wright (:gw280) (needinfo me!)]

This fixes it for Android, but not for Linux I think.

Comment 7

[Benoit Jacob [:bjacob] (mostly away)]

(In reply to Jeff Gilbert [:jgilbert] from comment #4)
> The API is supposed to work fine even with a timeout of 0. We should be
> detecting that it returned something other than EGL_CONDITION_SATISFIED, and
> not swap in the new frame. This probably doesn't play well with the rest of
> the layers code, though.

I suggested that to a few people offline and there was disagreement. If you care, you could file a bug.

> 
> Functionally, waiting up to FOREVER isn't different than Finish, I suppose.

It is different in that it allows the producer thread to continue while its rendering finishes.

> Really, we should be testing for (and using, where available) the
> EGL_KHR_wait_sync extension.

Unfortunately, the Tegra 3 doesn't seem to support that. But yes, by all means, pushing the wait to the GPU side is much better where it's possible. It's not going to give as big wins on Android as it did on Mac though, because of OMTC.

Comment 8

[Benoit Jacob [:bjacob] (mostly away)]

(In reply to Jeff Gilbert [:jgilbert] from comment #5)
> Comment on attachment 745228 [details] [diff] [review]
> ♫ Don't say ClientWaitSync unless FOREVER♫
> 
> Review of attachment 745228 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> The explanatory comment needs work, but functionally fine.
> 
> ::: gfx/gl/SharedSurfaceEGL.cpp
> @@ +189,5 @@
> >  
> > +    // Wait FOREVER, because
> > +    //  - 1. This is pretty much what we actually want; and
> > +    //  - 2. Some NVIDIA (both Tegra and Geforce) drivers have ClientWaitSync returning
> > +    //       immediately if the timeout delay is anything else than FOREVER.
> 
> I don't believe this is really what we want. We'd prefer just delay the
> composite of the new frame until the webgl frame is complete. We should be
> able to do this by leaving the old Consumer buffer in place, and leaving the
> dirty bit set.

I tried having that conversation in the office last week and there wasn't much consensus. That's definitely worth filing a bug and discussing in detail.

> 
> We should explore this in another bug, but I think this pair of comments
> needs to be corrected to match what the actual problem is.

I'll trim my comment.

> 
> Also, we should really just do this for NV, if that's the only place the
> problem is. I'll accept this patch for now, but we should do better later.

I understand that it's bad to have the compositor stuck FOREVER. But I don't know the granularity of multitasking on mobile GPUs, and so I don't know if we actually gain any interactivity by not using FOREVER.

It's worth filing a bug to discuss that alone. (For ClientWaitSync alone, orthogonal to trying to use server-side WaitSync in EGL).

Comment 9

[Benoit Jacob [:bjacob] (mostly away)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6bf36fe43896

Comment 10

[Benoit Jacob [:bjacob] (mostly away)]

(In reply to George Wright (:gw280) from comment #6)
> This fixes it for Android, but not for Linux I think.

This being a fix in SharedSurfaceEGL.cpp, it only affects EGL platforms. Good point... looking at other contextproviders now.

Comment 11

[Benoit Jacob [:bjacob] (mostly away)]

Hm, so Linux and Mac will use SharedSurfaceGL.cpp, which only uses server-side glWaitSync, so there is no timeout argument there (well, there is a dummy one, but it must be GL_TIMEOUT_IGNORED).

So this can't be exactly the same bug.

Comment 12

[George Wright (:gw280) (needinfo me!)]

(In reply to Benoit Jacob [:bjacob] from comment #11)
> Hm, so Linux and Mac will use SharedSurfaceGL.cpp, which only uses
> server-side glWaitSync, so there is no timeout argument there (well, there
> is a dummy one, but it must be GL_TIMEOUT_IGNORED).
> 
> So this can't be exactly the same bug.

Makes sense. Let's deprioritise Linux/OSX for now and create a new bug to track it.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/6bf36fe43896