Resize surface on WM_SIZE

Resize surface on receipt of WM_SIZE to avoid corruption during resize. We hook WM_SIZE using window subclassing.

This is a continuation of http://codereview.appspot.com/3038042/



Committed: http://code.google.com/p/angleproject/source/detail?r=486

[apatrick, 2010-11-16 18:15:42]

A few thoughts...

http://codereview.appspot.com/3122041/diff/1/src/libEGL/Surface.cpp
File src/libEGL/Surface.cpp (left):

http://codereview.appspot.com/3122041/diff/1/src/libEGL/Surface.cpp#oldcode393
src/libEGL/Surface.cpp:393: checkForOutOfDateSwapChain();
Removing this will prevent the present interval from being updated until the
window is next resized.

http://codereview.appspot.com/3122041/diff/1/src/libEGL/Surface.cpp
File src/libEGL/Surface.cpp (right):

http://codereview.appspot.com/3122041/diff/1/src/libEGL/Surface.cpp#newcode354
src/libEGL/Surface.cpp:354: return CallWindowProc(prevWndFunc, hwnd, message,
wparam, lparam);
This could be fragile if the client has also subclassed the window. Is there any
way we could at least assert if the symmetry between eglCreateWindowSurface,
eglDestroySurface and setting and restoring the WNDPROC is broken?

http://codereview.appspot.com/3122041/diff/1/src/libEGL/Surface.cpp#newcode359
src/libEGL/Surface.cpp:359: LONG oldWndProc = SetWindowLong(mWindow,
GWL_WNDPROC, reinterpret_cast<LONG>(SurfaceWindowProc));
I expect this will fail if the window was created by another thread. Maybe give
up on intercepting WM_SIZE in that case and fall back to the old resize on swap.

http://codereview.appspot.com/3122041/diff/1/src/libEGL/Surface.cpp#newcode368
src/libEGL/Surface.cpp:368: SetWindowLong(mWindow, GWL_WNDPROC, prevWndFunc);
This could be fragile if the client has also subclassed the window.

http://codereview.appspot.com/3122041/diff/1/src/libEGL/Surface.h
File src/libEGL/Surface.h (right):

http://codereview.appspot.com/3122041/diff/1/src/libEGL/Surface.h#newcode45
src/libEGL/Surface.h:45: virtual bool checkForOutOfDateSwapChain(); // returns
true if swapchain changed due to resize or interval update
This need not be public or virtual now.

[dgkoch, 2010-11-16 19:17:56]

I'm not that familiar with how the window side of things works -- can the
WM_SIZE message come in at any point (ie mid-frame?), or will that only happen
when GL stuff is not being drawn?

http://codereview.appspot.com/3122041/diff/1/src/libEGL/Surface.cpp
File src/libEGL/Surface.cpp (left):

http://codereview.appspot.com/3122041/diff/1/src/libEGL/Surface.cpp#oldcode393
src/libEGL/Surface.cpp:393: checkForOutOfDateSwapChain();
Agree with Al here.
If you are concerned about loosing part of a frame, just move it until after the
Present call (line 468-ish new code).

http://codereview.appspot.com/3122041/diff/1/src/libEGL/Surface.cpp
File src/libEGL/Surface.cpp (right):

http://codereview.appspot.com/3122041/diff/1/src/libEGL/Surface.cpp#newcode351
src/libEGL/Surface.cpp:351: surf->checkForOutOfDateSwapChain();
might want to include a NULL check on surf here.

[nduca, 2010-11-16 19:20:38]

http://codereview.appspot.com/3122041/diff/1/src/libEGL/Surface.cpp
File src/libEGL/Surface.cpp (left):

http://codereview.appspot.com/3122041/diff/1/src/libEGL/Surface.cpp#oldcode393
src/libEGL/Surface.cpp:393: checkForOutOfDateSwapChain();
On 2010/11/16 18:15:42, apatrick wrote:
> Removing this will prevent the present interval from being updated until the
> window is next resized.

Done.

http://codereview.appspot.com/3122041/diff/1/src/libEGL/Surface.cpp#oldcode393
src/libEGL/Surface.cpp:393: checkForOutOfDateSwapChain();
On 2010/11/16 19:17:56, dgkoch wrote:
> Agree with Al here.
> If you are concerned about loosing part of a frame, just move it until after
the
> Present call (line 468-ish new code).

Done.

http://codereview.appspot.com/3122041/diff/1/src/libEGL/Surface.cpp
File src/libEGL/Surface.cpp (right):

http://codereview.appspot.com/3122041/diff/1/src/libEGL/Surface.cpp#newcode351
src/libEGL/Surface.cpp:351: surf->checkForOutOfDateSwapChain();
On 2010/11/16 19:17:56, dgkoch wrote:
> might want to include a NULL check on surf here.

Done.

http://codereview.appspot.com/3122041/diff/1/src/libEGL/Surface.cpp#newcode354
src/libEGL/Surface.cpp:354: return CallWindowProc(prevWndFunc, hwnd, message,
wparam, lparam);
On 2010/11/16 18:15:42, apatrick wrote:
> This could be fragile if the client has also subclassed the window. Is there
any
> way we could at least assert if the symmetry between eglCreateWindowSurface,
> eglDestroySurface and setting and restoring the WNDPROC is broken?

Done.

http://codereview.appspot.com/3122041/diff/1/src/libEGL/Surface.cpp#newcode359
src/libEGL/Surface.cpp:359: LONG oldWndProc = SetWindowLong(mWindow,
GWL_WNDPROC, reinterpret_cast<LONG>(SurfaceWindowProc));
On 2010/11/16 18:15:42, apatrick wrote:
> I expect this will fail if the window was created by another thread. Maybe
give
> up on intercepting WM_SIZE in that case and fall back to the old resize on
swap.

Done.

http://codereview.appspot.com/3122041/diff/1/src/libEGL/Surface.cpp#newcode368
src/libEGL/Surface.cpp:368: SetWindowLong(mWindow, GWL_WNDPROC, prevWndFunc);
On 2010/11/16 18:15:42, apatrick wrote:
> This could be fragile if the client has also subclassed the window.

Done.

http://codereview.appspot.com/3122041/diff/1/src/libEGL/Surface.h
File src/libEGL/Surface.h (right):

http://codereview.appspot.com/3122041/diff/1/src/libEGL/Surface.h#newcode45
src/libEGL/Surface.h:45: virtual bool checkForOutOfDateSwapChain(); // returns
true if swapchain changed due to resize or interval update
On 2010/11/16 18:15:42, apatrick wrote:
> This need not be public or virtual now.

Still needs to be public so the SurfaceWindowProc can get to it. But definitely
not virtual.

[apatrick1, 2010-11-16 20:24:26]

http://codereview.appspot.com/3122041/diff/8001/src/libEGL/Surface.cpp
File src/libEGL/Surface.cpp (right):

http://codereview.appspot.com/3122041/diff/8001/src/libEGL/Surface.cpp#newcod...
src/libEGL/Surface.cpp:352: surf->checkForOutOfDateSwapChain(true);
The WM_SIZE message will arrive synchronously when SetWindowPos or similar are
called or, if the window is user resizable, potentially any time the thread that
created the window pumps its message loop. In either case, there is the
potential for a partial frame to have been rendered.

resetSwapChain does not preserve the contents of the back buffer or the depth /
stencil buffer. I think this is okay though. It does not make a lot of sense to
me to preserve it, especially for depth / stencil where there is no obvious
default value for exposed pixels when the window grows.

I think the advantage of this change is that the thread that creates the window
can at least rely on assuming that the back buffer is always exactly the same
size as the window, which is an appealing invariant.

Although losing the contents of the back buffer on window resize seems
undesirable, I think this is a gray area in OpenGL and in practice
implementations vary. The thread that created the window does still have the
ability to prevent the window from being resized at arbitrary times.

http://codereview.appspot.com/3122041/diff/8001/src/libEGL/Surface.cpp#newcod...
src/libEGL/Surface.cpp:377: // hwnd as well and did not unsubclass before
destroying its GL context. The
nit: GL context -> EGL surface

http://codereview.appspot.com/3122041/diff/8001/src/libEGL/Surface.cpp#newcod...
src/libEGL/Surface.cpp:403: if ((sizeDirty && checkResize) ||
mPresentIntervalDirty)
I think it is okay to check for a resize even if the window is subclassed. The
window can't change size until the WM_SIZE message handler has returned so the
size shouldn't be different here and predicating off checkResize seems
redundant.

http://codereview.appspot.com/3122041/diff/8001/src/libEGL/Surface.cpp#newcod...
src/libEGL/Surface.cpp:489: checkForOutOfDateSwapChain(false);
nit: checkForOutOfDateSwapChain can access mWindowSubclass directly (if you
don't think it should be removed).

[nduca, 2010-11-16 21:02:15]

http://codereview.appspot.com/3122041/diff/8001/src/libEGL/Surface.cpp
File src/libEGL/Surface.cpp (right):

http://codereview.appspot.com/3122041/diff/8001/src/libEGL/Surface.cpp#newcod...
src/libEGL/Surface.cpp:377: // hwnd as well and did not unsubclass before
destroying its GL context. The
On 2010/11/16 20:24:27, apatrick1 wrote:
> nit: GL context -> EGL surface

Done.

http://codereview.appspot.com/3122041/diff/8001/src/libEGL/Surface.cpp#newcod...
src/libEGL/Surface.cpp:403: if ((sizeDirty && checkResize) ||
mPresentIntervalDirty)
On 2010/11/16 20:24:27, apatrick1 wrote:
> I think it is okay to check for a resize even if the window is subclassed. The
> window can't change size until the WM_SIZE message handler has returned so the
> size shouldn't be different here and predicating off checkResize seems
> redundant.

Done.

http://codereview.appspot.com/3122041/diff/8001/src/libEGL/Surface.cpp#newcod...
src/libEGL/Surface.cpp:489: checkForOutOfDateSwapChain(false);
On 2010/11/16 20:24:27, apatrick1 wrote:
> nit: checkForOutOfDateSwapChain can access mWindowSubclass directly (if you
> don't think it should be removed).

Done.

[apatrick1, 2010-11-16 21:40:23]

http://codereview.appspot.com/3122041/diff/12001/src/libEGL/Surface.cpp
File src/libEGL/Surface.cpp (right):

http://codereview.appspot.com/3122041/diff/12001/src/libEGL/Surface.cpp#newco...
src/libEGL/Surface.cpp:104: void Surface::resetSwapChain(int backbufferWidth,
int backbufferHeight)
One last thing and then LGTM and I'll leave the call to Daniel. I think that,
since this destroys the contents on the back buffer, it should maybe call
InvalidateRect on the window handle with a NULL rect to indicate to the client
that it needs to repaint the whole window, not just any newly exposed region of
the window.

[nduca, 2010-11-16 21:57:37]

http://codereview.appspot.com/3122041/diff/12001/src/libEGL/Surface.cpp
File src/libEGL/Surface.cpp (right):

http://codereview.appspot.com/3122041/diff/12001/src/libEGL/Surface.cpp#newco...
src/libEGL/Surface.cpp:104: void Surface::resetSwapChain(int backbufferWidth,
int backbufferHeight)
On 2010/11/16 21:40:24, apatrick1 wrote:
> One last thing and then LGTM and I'll leave the call to Daniel. I think that,
> since this destroys the contents on the back buffer, it should maybe call
> InvalidateRect on the window handle with a NULL rect to indicate to the client
> that it needs to repaint the whole window, not just any newly exposed region
of
> the window.

Done.

[dgkoch, 2010-11-17 21:35:03]

almost there...

http://codereview.appspot.com/3122041/diff/19001/src/libEGL/Surface.cpp
File src/libEGL/Surface.cpp (right):

http://codereview.appspot.com/3122041/diff/19001/src/libEGL/Surface.cpp#newco...
src/libEGL/Surface.cpp:364: if(oldWndProc == 0) {
to determine that this really was failure and not just a previously NULL WndProc
(is that possible?) you should clear and check the LastError as described in
MSDN: http://msdn.microsoft.com/en-us/library/ms633591(VS.85).aspx

http://codereview.appspot.com/3122041/diff/19001/src/libEGL/Surface.cpp#newco...
src/libEGL/Surface.cpp:370: SetProp(mWindow, kParentWndProc,
reinterpret_cast<HANDLE>(oldWndProc));
should initialize mWindowSubclassed = true; here

http://codereview.appspot.com/3122041/diff/19001/src/libEGL/Surface.cpp#newco...
src/libEGL/Surface.cpp:378: // If this assert fails, then it is likely the
appplication has subclassed the
appplication -> application

http://codereview.appspot.com/3122041/diff/19001/src/libEGL/Surface.cpp#newco...
src/libEGL/Surface.cpp:388: RemoveProp(mWindow, kParentWndProc);
should probably set mWindowSubclassed = false; here in case this is ever called
from somewhere other than the destructor.

[nduca, 2010-11-17 21:43:45]

http://codereview.appspot.com/3122041/diff/19001/src/libEGL/Surface.cpp
File src/libEGL/Surface.cpp (right):

http://codereview.appspot.com/3122041/diff/19001/src/libEGL/Surface.cpp#newco...
src/libEGL/Surface.cpp:364: if(oldWndProc == 0) {
On 2010/11/17 21:35:03, dgkoch wrote:
> to determine that this really was failure and not just a previously NULL
WndProc
> (is that possible?) you should clear and check the LastError as described in
> MSDN: http://msdn.microsoft.com/en-us/library/ms633591%28VS.85%29.aspx

Done.

http://codereview.appspot.com/3122041/diff/19001/src/libEGL/Surface.cpp#newco...
src/libEGL/Surface.cpp:370: SetProp(mWindow, kParentWndProc,
reinterpret_cast<HANDLE>(oldWndProc));
On 2010/11/17 21:35:03, dgkoch wrote:
> should initialize mWindowSubclassed = true; here

Done.

http://codereview.appspot.com/3122041/diff/19001/src/libEGL/Surface.cpp#newco...
src/libEGL/Surface.cpp:378: // If this assert fails, then it is likely the
appplication has subclassed the
On 2010/11/17 21:35:03, dgkoch wrote:
> appplication -> application

Done.

http://codereview.appspot.com/3122041/diff/19001/src/libEGL/Surface.cpp#newco...
src/libEGL/Surface.cpp:388: RemoveProp(mWindow, kParentWndProc);
On 2010/11/17 21:35:03, dgkoch wrote:
> should probably set mWindowSubclassed = false; here in case this is ever
called
> from somewhere other than the destructor.

Done.

[dgkoch, 2010-11-17 21:51:33]

One last thing and then LGTM.

http://codereview.appspot.com/3122041/diff/24001/src/libEGL/Surface.cpp
File src/libEGL/Surface.cpp (right):

http://codereview.appspot.com/3122041/diff/24001/src/libEGL/Surface.cpp#newco...
src/libEGL/Surface.cpp:363: LONG oldWndProc = SetWindowLong(mWindow,
GWL_WNDPROC, reinterpret_cast<LONG>(SurfaceWindowProc));
you need to add a SetLastError(0) before this call in case there is a residual
error here.

[nduca, 2010-11-17 21:57:44]

http://codereview.appspot.com/3122041/diff/24001/src/libEGL/Surface.cpp
File src/libEGL/Surface.cpp (right):

http://codereview.appspot.com/3122041/diff/24001/src/libEGL/Surface.cpp#newco...
src/libEGL/Surface.cpp:363: LONG oldWndProc = SetWindowLong(mWindow,
GWL_WNDPROC, reinterpret_cast<LONG>(SurfaceWindowProc));
Sigh.

On 2010/11/17 21:51:33, dgkoch wrote:
> you need to add a SetLastError(0) before this call in case there is a residual
> error here.

[dgkoch, 2010-11-17 22:01:19]

LGTM

[nduca, 2010-11-17 22:04:32]

w00t! :)


On 2010/11/17 22:01:19, dgkoch wrote:
> LGTM