1374136 - Enable recycle when EGL sync extensions does not support

Status: RESOLVED WONTFIX

(Core :: Graphics: CanvasWebGL, defect)

Description

[Hiroshi Hatake [:cosmo0920]]

Attachment: 0001-Enable-sharing-SharedSurface_EGLImage.patch

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Firefox/45.0
Build ID: 20170419042421

Steps to reproduce:

In EGL sync extensions not supported environment, WebGL1 context lost, does not re-create, and occurred SEGV.
When KHR_fence_sync and OES_EGL_sync are not supported, I guess we can set mRecycle as true.
I confirmed that Renesas RZ/G1E board with SGX540 GPU.


Actual results:

EGL backend WebGL context didn't create and re-created when it lost.


Expected results:

EGL backend WebGL context should create and re-created when it lost.

Comment 1

[Hiroshi Hatake [:cosmo0920]]

Comment on attachment 8878970 [details] [diff] [review]
0001-Enable-sharing-SharedSurface_EGLImage.patch

SharedSurface_EGLImage constructor can set canRecycle as true when sync extensions is not supported in EGL.
How about this proof of concept patch, Jeff?

Comment 2

[Kelsey Gilbert [:jgilbert]]

Why?

Comment 3

[Takuro Ashie [:ashie]]

I explain what we mean.

At first, the first comment is totally wrong. Please forget it.
(I'll explain the detail of it later.)

The second comment is that we'd like to say.

> SharedSurface_EGLImage constructor can set canRecycle as true when sync extensions is not supported in EGL.
> How about this proof of concept patch, Jeff?

Most concrete classes of SharedSurface set "canRecycle" as true, only
SharedSurface_EGLImage seems to set it as false. So we think it would be better
if we can set it as true to reduce the cost of reallocation.

From this viewpoint, some codes say "mSync" is the root cause to prevent it:

>                    false) // Can't recycle, as mSync changes never update TextureHost.

> SharedSurface_EGLImage::ProducerReleaseImpl()
> {
> ...
>         if (mSync) {
>             MOZ_RELEASE_ASSERT(false, "GFX: Non-recycleable should not Fence twice.");

So we assumed that we can recycle it if mSync isn't used. Since our SoC
(Renesas RZ/G1E) doesn't have GL_OES_EGL_sync, mSync isn't used (in this case
glFinish is used instead of fence). When we change "canRecycle" to true
(the attached patch), it seems work fine, SuaredSurface_EGLImage instances are
recycled and it doesn't cause crash, stall or etc...

We want your advice for this patch:

* The above assumption is correct?
* If it's not correct, we want know the reason.
* If it's correct, it may also be worthful for other people, so we want submit the patch to mozilla-central.

Comment 4

[Takuro Ashie [:ashie]]

> At first, the first comment is totally wrong. Please forget it.
> (I'll explain the detail of it later.)

We are now trying to enable WebGL on our SoC (Renesas RZ/G1E).
When we force enable WebGL, eglDestroyImageKHR() in
~SharedSurface_EGLImage() causes crash.
We're now investigating the root cause but we don't resolve it yet.

Since we wanted to try WebGL anyway, we comment out the following line
in ~SharedSurface_EGLImage() temporarily:

>	 mEGL->fDestroyImage(Display(), mImage);

We could see the WebGL canvas but killed immediately by OOM killer.
To reduce memory usage, we enabled recycle feature of SharedSurface.
Now we can see WebGL contnets on this board.
But, of cause, it's not a correct solution.
We should resolve eglDestroyImageKHR() issue.

Comment 5

[Kelsey Gilbert [:jgilbert]]

My concern is that this adds complexity to the system (add bigger differences in behavior between systems) without providing useful performance improvements. Without fences, we must glFinish, which is overwhelming enough that it should allow us to ignore the perf penalty of not recycling these objects.

Comment 6

[Hiroshi Hatake [:cosmo0920]]

(In reply to Jeff Gilbert [:jgilbert] from comment #5)
> My concern is that this adds complexity to the system (add bigger
> differences in behavior between systems) without providing useful
> performance improvements. Without fences, we must glFinish, which is
> overwhelming enough that it should allow us to ignore the perf penalty of
> not recycling these objects.

I understand what you mean.

Our working to port RG/Z board EGL graphics driver's current behavior seems to be broken in eglDestoryImageKHR.
We use this workaround patch for prevent SEGV caused by this board EGL driver for now.

And my colleague said that this patch is just workaround to reduce memory usage in RG/Z board which has 1GB memory. 
We shall resolve eglDestroyImageKHR() issue.

Comment 7

[Kelsey Gilbert [:jgilbert]]

Memory usage shouldn't change much between the two approaches, as far as I can tell. eglDestroyImage is important for freeing memory anyways, so if fixing eglDestroyImage fixes the issue you're seeing here, I'll close this WONTFIX.

Comment 8

[Hiroshi Hatake [:cosmo0920]]

(In reply to Jeff Gilbert [:jgilbert] from comment #7)
> Memory usage shouldn't change much between the two approaches, as far as I
> can tell. eglDestroyImage is important for freeing memory anyways, so if
> fixing eglDestroyImage fixes the issue you're seeing here, I'll close this
> WONTFIX.

I see. It is what I want to know.
This issue is caused by RZ/G driver.
Thanks a lot.

Comment 9

[Kelsey Gilbert [:jgilbert]]

Please reopen this bug if the eglDestroyImage fix doesn't meet expectations.