Implemented semantics for eglDestroySurface when surface is current on thread.

Implemented semantics for eglDestroySurface when surface is current on thread.

The spec has this to say.

"
All resources associated with surface which were allocated by EGL are marked for deletion as soon as possible. If surface is current to any thread (see section 3.7.3), resources are not actually released while the surface remains current. Future references to surface remain valid only so long as it is current; it will be destroyed, and all future references to it will become invalid, as soon as any otherwise valid eglMakeCurrent call is made from the thread it is bound to."
Committed: http://code.google.com/p/angleproject/source/detail?r=632

[apatrick1, 2011-04-26 23:29:59]

Daniel.

Please review.

Thanks,

Al

[dgkoch, 2011-04-28 03:36:24]

Hi Al,

You need to use the EGL 1.4 spec, not the EGL 1.3 spec.  These semantics have
changed slightly. 

EGL 1.4 says the following about eglDestroySurface:
"All resources associated with surface which were allocated by EGL are marked
for deletion as soon as possible. Following eglDestroySurface, the surface and
the handle referring to it are treated in the same fashion as a surface
destroyed by eglTerminate (see section 3.2)."

Looking at eglTerminate it says: 
"Termination marks all EGL-specific resources associated with the specified
display for deletion. If contexts or surfaces created with respect to dpy are
current (see section 3.7.3) to any thread, then they are not actually destroyed
while they remain current. Such contexts and surfaces will be destroyed as soon
as eglReleaseThread is called from the thread they are bound to, or
eglMakeCurrent is called from that thread with the current rendering API (see
section 3.7) set such that the current context is affected. However, handles
referring to such surfaces and contexts be- come invalid (fn 1) as soon as
eglTerminate returns. Passing such handles to any other EGL function will
generate EGL_BAD_SURFACE or EGL_BAD_CONTEXT errors.

fn 1: Immediately releasing the handle is a subtle behavior change. Prior to the
January 13, 2009 release of the EGL 1.4 Specification, handles remained valid so
long as the underlying surface was current."

So the main difference from what you've implemented here is that the handle
needs to become invalid right away.  I believe you can accomplish this by always
executing mSurfaceSet.erase(surface) in the Display::destroySurface() function. 
This should then make the display->isValidSurface(surface) check from
validate(egl::Display *display, egl::Surface *surface) fail and report the
necessary errors due to invalid handles.

[apatrick1, 2011-04-28 19:53:36]

I think if I do as you suggest then the current surface will not be reset on
context lost. It will also not be destroyed on eglTerminate. So I added an
additional check to isValidSurface instead.

Also, am I being crazy or is there some handle validation missing in libEGL.cpp?

I patched this into Chrome and everything seemed to work okay.

[dgkoch, 2011-04-29 03:56:01]

http://codereview.appspot.com/4449064/diff/5001/src/libEGL/libEGL.cpp
File src/libEGL/libEGL.cpp (right):

http://codereview.appspot.com/4449064/diff/5001/src/libEGL/libEGL.cpp#newcode552
src/libEGL/libEGL.cpp:552: if (surface == EGL_NO_SURFACE)
the EGL_NO_SURFACE check should be before the validateSurface (either that or
it's redundant and it can be removed -- but I do prefer the explicit checks).

http://codereview.appspot.com/4449064/diff/5001/src/libEGL/libEGL.cpp#newcode584
src/libEGL/libEGL.cpp:584: if (surface == EGL_NO_SURFACE)
same as above

http://codereview.appspot.com/4449064/diff/5001/src/libEGL/libEGL.cpp#newcode663
src/libEGL/libEGL.cpp:663: if (!validateSurface(display, eglSurface))
ditto

http://codereview.appspot.com/4449064/diff/5001/src/libEGL/libEGL.cpp#newcode848
src/libEGL/libEGL.cpp:848: if (surface == EGL_NO_SURFACE ||
eglSurface->getWindowHandle())
ditto

http://codereview.appspot.com/4449064/diff/5001/src/libEGL/libEGL.cpp#newcode894
src/libEGL/libEGL.cpp:894: if (surface == EGL_NO_SURFACE ||
eglSurface->getWindowHandle())
ditto

http://codereview.appspot.com/4449064/diff/5001/src/libEGL/libEGL.cpp#newcode...
src/libEGL/libEGL.cpp:1015: if (ctx == EGL_NO_CONTEXT)
EGL_NO_CONTEXT check should happen before the validateContext call.

[dgkoch, 2011-04-29 03:57:40]

On 2011/04/28 19:53:36, apatrick1 wrote:
> I think if I do as you suggest then the current surface will not be reset on
> context lost. It will also not be destroyed on eglTerminate. So I added an
> additional check to isValidSurface instead.

That seems reasonable.

> Also, am I being crazy or is there some handle validation missing in
libEGL.cpp?

Are you referring to all the additional surface validations you added?  Seems
quite likely yes. EGL is definitely less um.. thoroughly implemented than
GLESv2.

[apatrick1, 2011-04-29 20:44:07]

> > Also, am I being crazy or is there some handle validation missing in
> libEGL.cpp?
> 
> Are you referring to all the additional surface validations you added?  Seems
> quite likely yes. EGL is definitely less um.. thoroughly implemented than
> GLESv2.

Yeah I was referring to the validation code I added.

[nicolas, 2011-05-18 15:07:14]

It looks like this caused a regression. The PowerVR sample applications call
eglTerminate() when the device is lost, and then it hangs (losing the device can
happen either when running on Windows XP or when ENABLE_D3D9EX is set to 0, and
you lock/unlock the screen).

What happens is that Display::terminate() calls destroySurface() on every
surface in the mSurfaceSet, but destroySurface() does not actually remove the
surface from the set if it is the current draw or read surface, but only flags
it for a pending destroy. And so it keeps looping.

Since eglTerminate() should destroy everything anyhow, we could either add a
'terminate' parameter to destroySurface() to really delete/erase it, or
alternatively call setCurrentDrawSurface(EGL_NO_SURFACE) and
setCurrentReadSurface(EGL_NO_SURFACE) at the top of Display::terminate().