From 67dcd6c2392caf3ff98f35a3d1ec550d229c167b Mon Sep 17 00:00:00 2001 From: Jack Palevich Date: Tue, 10 Nov 2009 18:26:42 +0800 Subject: Fix multi-lock ordering issues in GLSurfaceView There were potential deadlocks between the per-GLThread monitors and the GLThreadManager monitor. To avoid these deadlocks we now use a single monitor for both the GLThreadManager state and the per-GLThread state. Converted GLThreadManager's semaphore into the equivalent synchronized-wait-notifyAll code. This enables us to wait for either mDone, or user events, or the EGL surface with a single "wait()". Simplified the logic used to acquire and release the EGL surface. The EGL surface is now only requested while the surfaceFlinger surface is acquired. Removed the "egl surface stealing" policy we had recently inserted. It's not needed now that we reliably quit when requested. Pulled user event processing outside of the GLThreadManager monitor so that we don't call any potentially-long-running code while inside the monitor. This should help with bug 2228262. --- opengl/java/android/opengl/GLSurfaceView.java | 265 ++++++++++++++------------ 1 file changed, 144 insertions(+), 121 deletions(-) (limited to 'opengl/java') diff --git a/opengl/java/android/opengl/GLSurfaceView.java b/opengl/java/android/opengl/GLSurfaceView.java index 03301639cd29..9ca57bae7a1f 100644 --- a/opengl/java/android/opengl/GLSurfaceView.java +++ b/opengl/java/android/opengl/GLSurfaceView.java @@ -18,7 +18,6 @@ package android.opengl; import java.io.Writer; import java.util.ArrayList; -import java.util.concurrent.Semaphore; import javax.microedition.khronos.egl.EGL10; import javax.microedition.khronos.egl.EGL11; @@ -943,6 +942,9 @@ public class GLSurfaceView extends SurfaceView implements SurfaceHolder.Callback * to a Renderer instance to do the actual drawing. Can be configured to * render continuously or on request. * + * All potentially blocking synchronization is done through the + * sGLThreadManager object. This avoids multiple-lock ordering issues. + * */ class GLThread extends Thread { GLThread(Renderer renderer) { @@ -962,51 +964,31 @@ public class GLSurfaceView extends SurfaceView implements SurfaceHolder.Callback Log.i("GLThread", "starting tid=" + getId()); } - /* - * When the android framework launches a second instance of - * an activity, the new instance's onCreate() method may be - * called before the first instance returns from onDestroy(). - * - * This semaphore ensures that only one instance at a time - * accesses EGL. - */ try { guardedRun(); } catch (InterruptedException e) { // fall thru and exit normally } finally { - synchronized(this) { - if (LOG_THREADS) { - Log.i("GLThread", "exiting tid=" + getId()); - } - mDone = true; - notifyAll(); - } + sGLThreadManager.threadExiting(this); } } - private void startEgl() throws InterruptedException { - if (! mHaveEgl) { - mHaveEgl = true; - sGLThreadManager.start(this); - mEglHelper.start(); - } - } - - private void stopEgl() { + /* + * This private method should only be called inside a + * synchronized(sGLThreadManager) block. + */ + private void stopEglLocked() { if (mHaveEgl) { mHaveEgl = false; mEglHelper.destroySurface(); mEglHelper.finish(); - sGLThreadManager.end(this); + sGLThreadManager.releaseEglSurface(this); } } private void guardedRun() throws InterruptedException { mEglHelper = new EglHelper(); try { - startEgl(); - GL10 gl = null; boolean tellRendererSurfaceCreated = true; boolean tellRendererSurfaceChanged = true; @@ -1015,63 +997,97 @@ public class GLSurfaceView extends SurfaceView implements SurfaceHolder.Callback * This is our main activity thread's loop, we go until * asked to quit. */ - while (!mDone) { - + while (!isDone()) { /* * Update the asynchronous state (window size) */ - int w, h; - boolean changed; + int w = 0; + int h = 0; + boolean changed = false; boolean needStart = false; - synchronized (this) { - Runnable r; - while ((r = getEvent()) != null) { - r.run(); - } - if (mPaused) { - stopEgl(); - needStart = true; - } - while(true) { + boolean eventsWaiting = false; + + synchronized (sGLThreadManager) { + while (true) { + // Manage acquiring and releasing the SurfaceView + // surface and the EGL surface. + if (mPaused) { + stopEglLocked(); + } if (!mHasSurface) { if (!mWaitingForSurface) { - stopEgl(); + stopEglLocked(); mWaitingForSurface = true; - notifyAll(); + sGLThreadManager.notifyAll(); } } else { - boolean shouldHaveEgl = sGLThreadManager.shouldHaveEgl(this); - if (mHaveEgl && (!shouldHaveEgl)) { - stopEgl(); - } else if ((!mHaveEgl) && shouldHaveEgl) { - startEgl(); - needStart = true; + if (!mHaveEgl) { + if (sGLThreadManager.tryAcquireEglSurface(this)) { + mHaveEgl = true; + mEglHelper.start(); + mRequestRender = true; + needStart = true; + } } } - if (!needToWait()) { + + // Check if we need to wait. If not, update any state + // that needs to be updated, copy any state that + // needs to be copied, and use "break" to exit the + // wait loop. + + if (mDone) { + return; + } + + if (mEventsWaiting) { + eventsWaiting = true; + mEventsWaiting = false; + break; + } + + if ( (! mPaused) && mHasSurface && mHaveEgl + && (mWidth > 0) && (mHeight > 0) + && (mRequestRender || (mRenderMode == RENDERMODE_CONTINUOUSLY)) + ) { + changed = mSizeChanged; + w = mWidth; + h = mHeight; + mSizeChanged = false; + mRequestRender = false; + if (mHasSurface && mWaitingForSurface) { + changed = true; + mWaitingForSurface = false; + sGLThreadManager.notifyAll(); + } break; } + + // By design, this is the only place where we wait(). + if (LOG_THREADS) { - Log.i("GLThread", "needToWait tid=" + getId()); + Log.i("GLThread", "waiting tid=" + getId()); } - wait(); - } - if (mDone) { - break; + sGLThreadManager.wait(); } - changed = mSizeChanged; - w = mWidth; - h = mHeight; - mSizeChanged = false; - mRequestRender = false; - if (mHasSurface && mWaitingForSurface) { - changed = true; - mWaitingForSurface = false; - notifyAll(); + } // end of synchronized(sGLThreadManager) + + /* + * Handle queued events + */ + if (eventsWaiting) { + Runnable r; + while ((r = getEvent()) != null) { + r.run(); + if (isDone()) { + return; + } } + // Go back and see if we need to wait to render. + continue; } + if (needStart) { - startEgl(); tellRendererSurfaceCreated = true; changed = true; } @@ -1102,71 +1118,63 @@ public class GLSurfaceView extends SurfaceView implements SurfaceHolder.Callback /* * clean-up everything... */ - stopEgl(); + synchronized (sGLThreadManager) { + stopEglLocked(); + } } } - private boolean needToWait() { - if (mDone) { - return false; + private boolean isDone() { + synchronized (sGLThreadManager) { + return mDone; } - - if (mPaused || (! mHasSurface) || (! mHaveEgl)) { - return true; - } - - if ((mWidth > 0) && (mHeight > 0) && (mRequestRender || (mRenderMode == RENDERMODE_CONTINUOUSLY))) { - return false; - } - - return true; } public void setRenderMode(int renderMode) { if ( !((RENDERMODE_WHEN_DIRTY <= renderMode) && (renderMode <= RENDERMODE_CONTINUOUSLY)) ) { throw new IllegalArgumentException("renderMode"); } - synchronized(this) { + synchronized(sGLThreadManager) { mRenderMode = renderMode; if (renderMode == RENDERMODE_CONTINUOUSLY) { - notifyAll(); + sGLThreadManager.notifyAll(); } } } public int getRenderMode() { - synchronized(this) { + synchronized(sGLThreadManager) { return mRenderMode; } } public void requestRender() { - synchronized(this) { + synchronized(sGLThreadManager) { mRequestRender = true; - notifyAll(); + sGLThreadManager.notifyAll(); } } public void surfaceCreated() { - synchronized(this) { + synchronized(sGLThreadManager) { if (LOG_THREADS) { Log.i("GLThread", "surfaceCreated tid=" + getId()); } mHasSurface = true; - notifyAll(); + sGLThreadManager.notifyAll(); } } public void surfaceDestroyed() { - synchronized(this) { + synchronized(sGLThreadManager) { if (LOG_THREADS) { Log.i("GLThread", "surfaceDestroyed tid=" + getId()); } mHasSurface = false; - notifyAll(); + sGLThreadManager.notifyAll(); while(!mWaitingForSurface && isAlive() && ! mDone) { try { - wait(); + sGLThreadManager.wait(); } catch (InterruptedException e) { Thread.currentThread().interrupt(); } @@ -1175,35 +1183,35 @@ public class GLSurfaceView extends SurfaceView implements SurfaceHolder.Callback } public void onPause() { - synchronized (this) { + synchronized (sGLThreadManager) { mPaused = true; - notifyAll(); + sGLThreadManager.notifyAll(); } } public void onResume() { - synchronized (this) { + synchronized (sGLThreadManager) { mPaused = false; mRequestRender = true; - notifyAll(); + sGLThreadManager.notifyAll(); } } public void onWindowResize(int w, int h) { - synchronized (this) { + synchronized (sGLThreadManager) { mWidth = w; mHeight = h; mSizeChanged = true; - notifyAll(); + sGLThreadManager.notifyAll(); } } public void requestExitAndWait() { // don't call this from GLThread thread or it is a guaranteed // deadlock! - synchronized(this) { + synchronized(sGLThreadManager) { mDone = true; - notifyAll(); + sGLThreadManager.notifyAll(); } try { join(); @@ -1219,6 +1227,10 @@ public class GLSurfaceView extends SurfaceView implements SurfaceHolder.Callback public void queueEvent(Runnable r) { synchronized(this) { mEventQueue.add(r); + synchronized(sGLThreadManager) { + mEventsWaiting = true; + sGLThreadManager.notifyAll(); + } } } @@ -1232,6 +1244,8 @@ public class GLSurfaceView extends SurfaceView implements SurfaceHolder.Callback return null; } + // Once the thread is started, all accesses to the following member + // variables are protected by the sGLThreadManager monitor private boolean mDone; private boolean mPaused; private boolean mHasSurface; @@ -1241,6 +1255,9 @@ public class GLSurfaceView extends SurfaceView implements SurfaceHolder.Callback private int mHeight; private int mRenderMode; private boolean mRequestRender; + private boolean mEventsWaiting; + // End of member variables protected by the sGLThreadManager monitor. + private Renderer mRenderer; private ArrayList mEventQueue = new ArrayList(); private EglHelper mEglHelper; @@ -1286,37 +1303,43 @@ public class GLSurfaceView extends SurfaceView implements SurfaceHolder.Callback } } - static class GLThreadManager { - public boolean shouldHaveEgl(GLThread thread) { - synchronized(this) { - return thread == mMostRecentGLThread || mMostRecentGLThread == null; + private static class GLThreadManager { + + public synchronized void threadExiting(GLThread thread) { + if (LOG_THREADS) { + Log.i("GLThread", "exiting tid=" + thread.getId()); } - } - public void start(GLThread thread) throws InterruptedException { - GLThread oldThread = null; - synchronized(this) { - oldThread = mMostRecentGLThread; - mMostRecentGLThread = thread; + thread.mDone = true; + if (mEglOwner == thread) { + mEglOwner = null; } - if (oldThread != null) { - synchronized(oldThread) { - oldThread.notifyAll(); - } + notifyAll(); + } + + /* + * Tries once to acquire the right to use an EGL + * surface. Does not block. + * @return true if the right to use an EGL surface was acquired. + */ + public synchronized boolean tryAcquireEglSurface(GLThread thread) { + if (mEglOwner == thread || mEglOwner == null) { + mEglOwner = thread; + notifyAll(); + return true; } - sEglSemaphore.acquire(); + return false; } - public void end(GLThread thread) { - sEglSemaphore.release(); - synchronized(this) { - if (mMostRecentGLThread == thread) { - mMostRecentGLThread = null; - } + + public synchronized void releaseEglSurface(GLThread thread) { + if (mEglOwner == thread) { + mEglOwner = null; } + notifyAll(); } - private GLThread mMostRecentGLThread; + + private GLThread mEglOwner; } - private static final Semaphore sEglSemaphore = new Semaphore(1); private static final GLThreadManager sGLThreadManager = new GLThreadManager(); private boolean mSizeChanged = true; -- cgit v1.2.3-59-g8ed1b