Release Surface when calling makeCurrent with null.
Refactorings to egl::Surface to enable ref-counting were causing
a situation where we could have two Window surfaces alive at the
same time. This would confuse the window procedure logic in
SurfaceD3D. Releasing the surface fixes this issue and conforms
closely to the wording on the spec on when Surfaces should be
deleted. Also add a test for message loops and surfaces.
BUG=475085
BUG=angleproject:963
Change-Id: Icdee3a7db97c9b54d779dabf1e1f82a89fefc546
Reviewed-on: https://chromium-review.googlesource.com/265064
Reviewed-by: Kenneth Russell <kbr@chromium.org>
Tested-by: Jamie Madill <jmadill@chromium.org>
diff --git a/src/libANGLE/Context.cpp b/src/libANGLE/Context.cpp
index 16a7d65..6915706 100644
--- a/src/libANGLE/Context.cpp
+++ b/src/libANGLE/Context.cpp
@@ -174,6 +174,8 @@
void Context::makeCurrent(egl::Surface *surface)
{
+ ASSERT(surface != nullptr);
+
if (!mHasBeenCurrent)
{
initRendererString();
@@ -187,12 +189,16 @@
// TODO(jmadill): do not allocate new pointers here
Framebuffer *framebufferZero = new DefaultFramebuffer(mCaps, mRenderer, surface);
-
setFramebufferZero(framebufferZero);
-
mRenderBuffer = surface->getRenderBuffer();
}
+void Context::releaseSurface()
+{
+ setFramebufferZero(nullptr);
+ mRenderBuffer = EGL_NONE;
+}
+
// NOTE: this function should not assume that this context is current!
void Context::markContextLost()
{
@@ -666,17 +672,19 @@
// First, check to see if the old default framebuffer
// was set for draw or read framebuffer, and change
// the bindings to point to the new one before deleting it.
- if (mState.getDrawFramebuffer()->id() == 0)
+ if (mState.getDrawFramebuffer() == nullptr ||
+ mState.getDrawFramebuffer()->id() == 0)
{
mState.setDrawFramebufferBinding(buffer);
}
- if (mState.getReadFramebuffer()->id() == 0)
+ if (mState.getReadFramebuffer() == nullptr ||
+ mState.getReadFramebuffer()->id() == 0)
{
mState.setReadFramebufferBinding(buffer);
}
- delete mFramebufferMap[0];
+ SafeDelete(mFramebufferMap[0]);
mFramebufferMap[0] = buffer;
}
@@ -884,7 +892,7 @@
{
// Queries about context capabilities and maximums are answered by Context.
// Queries about current GL state values are answered by State.
- // Indexed integer queries all refer to current state, so this function is a
+ // Indexed integer queries all refer to current state, so this function is a
// mere passthrough.
return mState.getIndexedIntegerv(target, index, data);
}
@@ -893,7 +901,7 @@
{
// Queries about context capabilities and maximums are answered by Context.
// Queries about current GL state values are answered by State.
- // Indexed integer queries all refer to current state, so this function is a
+ // Indexed integer queries all refer to current state, so this function is a
// mere passthrough.
return mState.getIndexedInteger64v(target, index, data);
}
@@ -1321,7 +1329,7 @@
void Context::detachBuffer(GLuint buffer)
{
- // Buffer detachment is handled by Context, because the buffer must also be
+ // Buffer detachment is handled by Context, because the buffer must also be
// attached from any VAOs in existence, and Context holds the VAO map.
// [OpenGL ES 2.0.24] section 2.9 page 22:
@@ -1365,8 +1373,8 @@
void Context::detachVertexArray(GLuint vertexArray)
{
- // Vertex array detachment is handled by Context, because 0 is a valid
- // VAO, and a pointer to it must be passed from Context to State at
+ // Vertex array detachment is handled by Context, because 0 is a valid
+ // VAO, and a pointer to it must be passed from Context to State at
// binding time.
// [OpenGL ES 3.0.2] section 2.10 page 43:
diff --git a/src/libANGLE/Context.h b/src/libANGLE/Context.h
index eeada43..32bfa08 100644
--- a/src/libANGLE/Context.h
+++ b/src/libANGLE/Context.h
@@ -63,11 +63,12 @@
virtual ~Context();
void makeCurrent(egl::Surface *surface);
+ void releaseSurface();
virtual void markContextLost();
bool isContextLost();
- // These create and destroy methods are merely pass-throughs to
+ // These create and destroy methods are merely pass-throughs to
// ResourceManager, which owns these object types
GLuint createBuffer();
GLuint createShader(GLenum type);
@@ -94,7 +95,7 @@
// NV Fences are owned by the Context.
GLuint createFenceNV();
void deleteFenceNV(GLuint fence);
-
+
// Queries are owned by the Context;
GLuint createQuery();
void deleteQuery(GLuint query);
diff --git a/src/libANGLE/Display.cpp b/src/libANGLE/Display.cpp
index a217b73..4af5a44 100644
--- a/src/libANGLE/Display.cpp
+++ b/src/libANGLE/Display.cpp
@@ -496,8 +496,9 @@
return error;
}
- if (context && drawSurface)
+ if (context != nullptr && drawSurface != nullptr)
{
+ ASSERT(readSurface == drawSurface);
context->makeCurrent(drawSurface);
}
diff --git a/src/libANGLE/State.cpp b/src/libANGLE/State.cpp
index f75c230..1e34c63 100644
--- a/src/libANGLE/State.cpp
+++ b/src/libANGLE/State.cpp
@@ -765,7 +765,8 @@
bool State::removeReadFramebufferBinding(GLuint framebuffer)
{
- if (mReadFramebuffer->id() == framebuffer)
+ if (mReadFramebuffer != nullptr &&
+ mReadFramebuffer->id() == framebuffer)
{
mReadFramebuffer = NULL;
return true;
@@ -776,7 +777,8 @@
bool State::removeDrawFramebufferBinding(GLuint framebuffer)
{
- if (mDrawFramebuffer->id() == framebuffer)
+ if (mReadFramebuffer != nullptr &&
+ mDrawFramebuffer->id() == framebuffer)
{
mDrawFramebuffer = NULL;
return true;
diff --git a/src/libGLESv2/entry_points_egl.cpp b/src/libGLESv2/entry_points_egl.cpp
index 801240a..b6c0894 100644
--- a/src/libGLESv2/entry_points_egl.cpp
+++ b/src/libGLESv2/entry_points_egl.cpp
@@ -512,6 +512,20 @@
return EGL_FALSE;
}
+ if (dpy == EGL_NO_DISPLAY)
+ {
+ SetGlobalError(Error(EGL_BAD_DISPLAY, "'dpy' not a valid EGLDisplay handle"));
+ return EGL_FALSE;
+ }
+
+ // EGL 1.5 spec: dpy can be uninitialized if all other parameters are null
+ if (dpy != EGL_NO_DISPLAY && !display->isInitialized() &&
+ (ctx != EGL_NO_CONTEXT || draw != EGL_NO_SURFACE || read != EGL_NO_SURFACE))
+ {
+ SetGlobalError(Error(EGL_NOT_INITIALIZED, "'dpy' not initialized"));
+ return EGL_FALSE;
+ }
+
if (ctx != EGL_NO_CONTEXT)
{
Error error = ValidateContext(display, context);
@@ -564,14 +578,20 @@
UNIMPLEMENTED(); // FIXME
}
+ gl::Context *previousContext = GetGlobalContext();
+
SetGlobalDisplay(display);
SetGlobalDrawSurface(drawSurface);
SetGlobalReadSurface(readSurface);
SetGlobalContext(context);
- if (context != nullptr && display != nullptr && drawSurface != nullptr)
+ display->makeCurrent(drawSurface, readSurface, context);
+
+ // Release the surface from the previously-current context, to allow
+ // destroyed surfaces to delete themselves.
+ if (previousContext != nullptr && context != previousContext)
{
- display->makeCurrent(drawSurface, readSurface, context);
+ previousContext->releaseSurface();
}
SetGlobalError(Error(EGL_SUCCESS));
diff --git a/src/tests/angle_end2end_tests.gypi b/src/tests/angle_end2end_tests.gypi
index 64dbd89..22fa5e4 100644
--- a/src/tests/angle_end2end_tests.gypi
+++ b/src/tests/angle_end2end_tests.gypi
@@ -54,8 +54,9 @@
'<(angle_path)/src/tests/end2end_tests/UnpackRowLength.cpp',
'<(angle_path)/src/tests/end2end_tests/VertexAttributeTest.cpp',
'<(angle_path)/src/tests/end2end_tests/ViewportTest.cpp',
- '<(angle_path)/src/tests/standalone_tests/EGLThreadTest.cpp',
'<(angle_path)/src/tests/standalone_tests/EGLQueryContextTest.cpp',
+ '<(angle_path)/src/tests/standalone_tests/EGLSurfaceTest.cpp',
+ '<(angle_path)/src/tests/standalone_tests/EGLThreadTest.cpp',
],
},
'dependencies':
diff --git a/src/tests/end2end_tests/ANGLETest.cpp b/src/tests/end2end_tests/ANGLETest.cpp
index 375724f..c7cbcbf 100644
--- a/src/tests/end2end_tests/ANGLETest.cpp
+++ b/src/tests/end2end_tests/ANGLETest.cpp
@@ -3,15 +3,14 @@
#include "OSWindow.h"
ANGLETest::ANGLETest(EGLint glesMajorVersion, const EGLPlatformParameters &platform)
- : mEGLWindow(NULL)
+ : mEGLWindow(nullptr)
{
mEGLWindow = new EGLWindow(1280, 720, glesMajorVersion, platform);
}
ANGLETest::~ANGLETest()
{
- delete mEGLWindow;
- mEGLWindow = NULL;
+ SafeDelete(mEGLWindow);
}
void ANGLETest::SetUp()
@@ -50,7 +49,10 @@
void ANGLETest::swapBuffers()
{
- mEGLWindow->swap();
+ if (mEGLWindow->isGLInitialized())
+ {
+ mEGLWindow->swap();
+ }
}
void ANGLETest::drawQuad(GLuint program, const std::string& positionAttribName, GLfloat quadDepth, GLfloat quadScale)
diff --git a/src/tests/standalone_tests/EGLSurfaceTest.cpp b/src/tests/standalone_tests/EGLSurfaceTest.cpp
new file mode 100644
index 0000000..f53e05f
--- /dev/null
+++ b/src/tests/standalone_tests/EGLSurfaceTest.cpp
@@ -0,0 +1,226 @@
+//
+// Copyright 2015 The ANGLE Project Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+//
+// EGLSurfaceTest:
+// Tests pertaining to egl::Surface.
+//
+
+#include <gtest/gtest.h>
+#include <EGL/egl.h>
+#include <EGL/eglext.h>
+#include <GLES2/gl2.h>
+
+#include "common/angleutils.h"
+#include "OSWindow.h"
+
+namespace
+{
+
+class EGLSurfaceTest : public testing::Test
+{
+ protected:
+ EGLSurfaceTest()
+ : mDisplay(EGL_NO_DISPLAY),
+ mWindowSurface(EGL_NO_SURFACE),
+ mPbufferSurface(EGL_NO_SURFACE),
+ mContext(EGL_NO_CONTEXT),
+ mSecondContext(EGL_NO_CONTEXT),
+ mOSWindow(nullptr)
+ {
+ }
+
+ void SetUp() override
+ {
+ mOSWindow = CreateOSWindow();
+ mOSWindow->initialize("EGLSurfaceTest", 64, 64);
+ }
+
+ // Release any resources created in the test body
+ void TearDown() override
+ {
+ mOSWindow->destroy();
+ SafeDelete(mOSWindow);
+
+ if (mDisplay != EGL_NO_DISPLAY)
+ {
+ eglMakeCurrent(mDisplay, EGL_NO_SURFACE, EGL_NO_SURFACE, EGL_NO_CONTEXT);
+
+ if (mWindowSurface != EGL_NO_SURFACE)
+ {
+ eglDestroySurface(mDisplay, mWindowSurface);
+ mWindowSurface = EGL_NO_SURFACE;
+ }
+
+ if (mPbufferSurface != EGL_NO_SURFACE)
+ {
+ eglDestroySurface(mDisplay, mPbufferSurface);
+ mPbufferSurface = EGL_NO_SURFACE;
+ }
+
+ if (mContext != EGL_NO_CONTEXT)
+ {
+ eglDestroyContext(mDisplay, mContext);
+ mContext = EGL_NO_CONTEXT;
+ }
+
+ if (mSecondContext != EGL_NO_CONTEXT)
+ {
+ eglDestroyContext(mDisplay, mSecondContext);
+ mSecondContext = EGL_NO_CONTEXT;
+ }
+
+ eglTerminate(mDisplay);
+ mDisplay = EGL_NO_DISPLAY;
+ }
+
+ ASSERT_TRUE(mWindowSurface == EGL_NO_SURFACE && mContext == EGL_NO_CONTEXT);
+ }
+
+ void initializeSurface(EGLenum platformType)
+ {
+ PFNEGLGETPLATFORMDISPLAYEXTPROC eglGetPlatformDisplayEXT = reinterpret_cast<PFNEGLGETPLATFORMDISPLAYEXTPROC>(eglGetProcAddress("eglGetPlatformDisplayEXT"));
+ ASSERT_TRUE(eglGetPlatformDisplayEXT != nullptr);
+
+ const EGLint displayAttributes[] =
+ {
+ EGL_PLATFORM_ANGLE_TYPE_ANGLE, platformType,
+ EGL_PLATFORM_ANGLE_MAX_VERSION_MAJOR_ANGLE, EGL_DONT_CARE,
+ EGL_PLATFORM_ANGLE_MAX_VERSION_MINOR_ANGLE, EGL_DONT_CARE,
+ EGL_PLATFORM_ANGLE_DEVICE_TYPE_ANGLE, EGL_PLATFORM_ANGLE_DEVICE_TYPE_HARDWARE_ANGLE,
+ EGL_NONE,
+ };
+
+ mDisplay = eglGetPlatformDisplayEXT(EGL_PLATFORM_ANGLE_ANGLE, mOSWindow->getNativeDisplay(), displayAttributes);
+ ASSERT_TRUE(mDisplay != EGL_NO_DISPLAY);
+
+ EGLint majorVersion, minorVersion;
+ ASSERT_TRUE(eglInitialize(mDisplay, &majorVersion, &minorVersion) == EGL_TRUE);
+
+ eglBindAPI(EGL_OPENGL_ES_API);
+ ASSERT_TRUE(eglGetError() == EGL_SUCCESS);
+
+ const EGLint configAttributes[] =
+ {
+ EGL_RED_SIZE, EGL_DONT_CARE,
+ EGL_GREEN_SIZE, EGL_DONT_CARE,
+ EGL_BLUE_SIZE, EGL_DONT_CARE,
+ EGL_ALPHA_SIZE, EGL_DONT_CARE,
+ EGL_DEPTH_SIZE, EGL_DONT_CARE,
+ EGL_STENCIL_SIZE, EGL_DONT_CARE,
+ EGL_SAMPLE_BUFFERS, 0,
+ EGL_NONE
+ };
+
+ EGLint configCount;
+ ASSERT_TRUE(eglChooseConfig(mDisplay, configAttributes, &mConfig, 1, &configCount) || (configCount != 1) == EGL_TRUE);
+
+ std::vector<EGLint> surfaceAttributes;
+ surfaceAttributes.push_back(EGL_NONE);
+ surfaceAttributes.push_back(EGL_NONE);
+
+ // Create first window surface
+ mWindowSurface = eglCreateWindowSurface(mDisplay, mConfig, mOSWindow->getNativeWindow(), &surfaceAttributes[0]);
+ ASSERT_TRUE(eglGetError() == EGL_SUCCESS);
+
+ mPbufferSurface = eglCreatePbufferSurface(mDisplay, mConfig, &surfaceAttributes[0]);
+
+ EGLint contextAttibutes[] =
+ {
+ EGL_CONTEXT_CLIENT_VERSION, 2,
+ EGL_NONE
+ };
+
+ mContext = eglCreateContext(mDisplay, mConfig, nullptr, contextAttibutes);
+ ASSERT_TRUE(eglGetError() == EGL_SUCCESS);
+
+ mSecondContext = eglCreateContext(mDisplay, mConfig, nullptr, contextAttibutes);
+ ASSERT_TRUE(eglGetError() == EGL_SUCCESS);
+ }
+
+ void runMessageLoopTest(EGLSurface secondSurface, EGLContext secondContext)
+ {
+ eglMakeCurrent(mDisplay, mWindowSurface, mWindowSurface, mContext);
+ ASSERT_TRUE(eglGetError() == EGL_SUCCESS);
+
+ // Make a second context current
+ eglMakeCurrent(mDisplay, secondSurface, secondSurface, secondContext);
+ eglDestroySurface(mDisplay, mWindowSurface);
+
+ // Create second window surface
+ std::vector<EGLint> surfaceAttributes;
+ surfaceAttributes.push_back(EGL_NONE);
+ surfaceAttributes.push_back(EGL_NONE);
+
+ mWindowSurface = eglCreateWindowSurface(mDisplay, mConfig, mOSWindow->getNativeWindow(), &surfaceAttributes[0]);
+ ASSERT_TRUE(eglGetError() == EGL_SUCCESS);
+
+ eglMakeCurrent(mDisplay, mWindowSurface, mWindowSurface, mContext);
+ ASSERT_TRUE(eglGetError() == EGL_SUCCESS);
+
+ mOSWindow->signalTestEvent();
+ mOSWindow->messageLoop();
+ ASSERT_TRUE(mOSWindow->didTestEventFire());
+
+ // Simple operation to test the FBO is set appropriately
+ glClear(GL_COLOR_BUFFER_BIT);
+ }
+
+ EGLDisplay mDisplay;
+ EGLSurface mWindowSurface;
+ EGLSurface mPbufferSurface;
+ EGLContext mContext;
+ EGLContext mSecondContext;
+ EGLConfig mConfig;
+ OSWindow *mOSWindow;
+};
+
+// Test a surface bug where we could have two Window surfaces active
+// at one time, blocking message loops. See http://crbug.com/475085
+TEST_F(EGLSurfaceTest, MessageLoopBug)
+{
+ const char *extensionsString = eglQueryString(EGL_NO_DISPLAY, EGL_EXTENSIONS);
+ if (strstr(extensionsString, "EGL_ANGLE_platform_angle_d3d") == nullptr)
+ {
+ std::cout << "D3D Platform not supported in ANGLE";
+ return;
+ }
+
+ initializeSurface(EGL_PLATFORM_ANGLE_TYPE_D3D11_ANGLE);
+
+ runMessageLoopTest(EGL_NO_SURFACE, EGL_NO_CONTEXT);
+}
+
+// Tests the message loop bug, but with setting a second context
+// instead of null.
+TEST_F(EGLSurfaceTest, MessageLoopBugContext)
+{
+ const char *extensionsString = eglQueryString(EGL_NO_DISPLAY, EGL_EXTENSIONS);
+ if (strstr(extensionsString, "EGL_ANGLE_platform_angle_d3d") == nullptr)
+ {
+ std::cout << "D3D Platform not supported in ANGLE";
+ return;
+ }
+
+ initializeSurface(EGL_PLATFORM_ANGLE_TYPE_D3D11_ANGLE);
+
+ runMessageLoopTest(mPbufferSurface, mSecondContext);
+}
+
+// Test a bug where calling makeCurrent twice would release the surface
+TEST_F(EGLSurfaceTest, MakeCurrentTwice)
+{
+ initializeSurface(EGL_PLATFORM_ANGLE_TYPE_DEFAULT_ANGLE);
+
+ eglMakeCurrent(mDisplay, mWindowSurface, mWindowSurface, mContext);
+ ASSERT_TRUE(eglGetError() == EGL_SUCCESS);
+
+ eglMakeCurrent(mDisplay, mWindowSurface, mWindowSurface, mContext);
+ ASSERT_TRUE(eglGetError() == EGL_SUCCESS);
+
+ // Simple operation to test the FBO is set appropriately
+ glClear(GL_COLOR_BUFFER_BIT);
+}
+
+}