Resize surface on the first draw call to the surface rather than just-before ...

Resize surface on the first draw call to the surface rather than just-before present. This avoids corruption during resize.

[apatrick1, 2010-11-11 21:23:54]

Can you add an explanation of what the corruption issue is to the description?

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

http://codereview.appspot.com/3038042/diff/1/src/libEGL/Surface.cpp#newcode100
src/libEGL/Surface.cpp:100: resetSwapChain(windowRect.right - windowRect.left,
No need to wrap at 80 chars.

http://codereview.appspot.com/3038042/diff/1/src/libEGL/Surface.cpp#newcode128
src/libEGL/Surface.cpp:128: if (!GetClientRect(getWindowHandle(), &windowRect))
No need to call GetClientRect here. windowRect is not used.

http://codereview.appspot.com/3038042/diff/1/src/libEGL/Surface.cpp#newcode354
src/libEGL/Surface.cpp:354: bool Surface::checkForResizedWindow()
This also resets the presentation interval if it has changed but that is not
reflected in the function name.

http://codereview.appspot.com/3038042/diff/1/src/libEGL/Surface.cpp#newcode371
src/libEGL/Surface.cpp:371: resetSwapChain(clientWidth,clientHeight);
Nit: space after comma.

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

http://codereview.appspot.com/3038042/diff/1/src/libEGL/Surface.h#newcode45
src/libEGL/Surface.h:45: virtual bool checkForResizedWindow(); // returns true
if swapchain resized
Looks like it will return true if only the presentation interval changed as
well.

http://codereview.appspot.com/3038042/diff/1/src/libGLESv2/Context.cpp
File src/libGLESv2/Context.cpp (right):

http://codereview.appspot.com/3038042/diff/1/src/libGLESv2/Context.cpp#newcod...
src/libGLESv2/Context.cpp:2394: if (mCurrentSurface && getDrawFramebuffer() ==
mFramebufferMap[0]) {
I think mCurrentSurface should be egl::getCurrentDrawSurface(). Unfortunately
that can't directly be called here.

http://codereview.appspot.com/3038042/diff/1/src/libGLESv2/Context.cpp#newcod...
src/libGLESv2/Context.cpp:2646: if (mCurrentSurface && getDrawFramebuffer() ==
mFramebufferMap[0]) {
See above.

http://codereview.appspot.com/3038042/diff/1/src/libGLESv2/Context.cpp#newcod...
src/libGLESv2/Context.cpp:2712: if (mCurrentSurface && getDrawFramebuffer() ==
mFramebufferMap[0]) {
See above.

http://codereview.appspot.com/3038042/diff/1/src/libGLESv2/Context.h
File src/libGLESv2/Context.h (right):

http://codereview.appspot.com/3038042/diff/1/src/libGLESv2/Context.h#newcode433
src/libGLESv2/Context.h:433: egl::Surface* mCurrentSurface;
The current surface is distinct from the current context. I'm not sure if the
notion of a context "having" a current surface is right.

[nduca, 2010-11-11 21:36:16]

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

http://codereview.appspot.com/3038042/diff/1/src/libEGL/Surface.cpp#newcode100
src/libEGL/Surface.cpp:100: resetSwapChain(windowRect.right - windowRect.left,
On 2010/11/11 21:23:54, apatrick1 wrote:
> No need to wrap at 80 chars.

Done.

http://codereview.appspot.com/3038042/diff/1/src/libEGL/Surface.cpp#newcode128
src/libEGL/Surface.cpp:128: if (!GetClientRect(getWindowHandle(), &windowRect))
On 2010/11/11 21:23:54, apatrick1 wrote:
> No need to call GetClientRect here. windowRect is not used.

Done.

http://codereview.appspot.com/3038042/diff/1/src/libEGL/Surface.cpp#newcode354
src/libEGL/Surface.cpp:354: bool Surface::checkForResizedWindow()
checkForResizedWindowAndUpdateInterval?

On 2010/11/11 21:23:54, apatrick1 wrote:
> This also resets the presentation interval if it has changed but that is not
> reflected in the function name.

http://codereview.appspot.com/3038042/diff/1/src/libEGL/Surface.cpp#newcode371
src/libEGL/Surface.cpp:371: resetSwapChain(clientWidth,clientHeight);
On 2010/11/11 21:23:54, apatrick1 wrote:
> Nit: space after comma.

Done.

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

http://codereview.appspot.com/3038042/diff/1/src/libEGL/Surface.h#newcode45
src/libEGL/Surface.h:45: virtual bool checkForResizedWindow(); // returns true
if swapchain resized
On 2010/11/11 21:23:54, apatrick1 wrote:
> Looks like it will return true if only the presentation interval changed as
> well.

Done.

http://codereview.appspot.com/3038042/diff/1/src/libGLESv2/Context.cpp
File src/libGLESv2/Context.cpp (right):

http://codereview.appspot.com/3038042/diff/1/src/libGLESv2/Context.cpp#newcod...
src/libGLESv2/Context.cpp:2394: if (mCurrentSurface && getDrawFramebuffer() ==
mFramebufferMap[0]) {
Yeah, that's why I'm storing the current surface instead. I'm totally open to
other options... ideas?

On 2010/11/11 21:23:54, apatrick1 wrote:
> I think mCurrentSurface should be egl::getCurrentDrawSurface(). Unfortunately
> that can't directly be called here.

http://codereview.appspot.com/3038042/diff/1/src/libGLESv2/Context.cpp#newcod...
src/libGLESv2/Context.cpp:2646: if (mCurrentSurface && getDrawFramebuffer() ==
mFramebufferMap[0]) {
On 2010/11/11 21:23:54, apatrick1 wrote:
> See above.

Done.

http://codereview.appspot.com/3038042/diff/1/src/libGLESv2/Context.cpp#newcod...
src/libGLESv2/Context.cpp:2646: if (mCurrentSurface && getDrawFramebuffer() ==
mFramebufferMap[0]) {
On 2010/11/11 21:23:54, apatrick1 wrote:
> See above.

Done.

http://codereview.appspot.com/3038042/diff/1/src/libGLESv2/Context.cpp#newcod...
src/libGLESv2/Context.cpp:2712: if (mCurrentSurface && getDrawFramebuffer() ==
mFramebufferMap[0]) {
On 2010/11/11 21:23:54, apatrick1 wrote:
> See above.

Done.

http://codereview.appspot.com/3038042/diff/1/src/libGLESv2/Context.h
File src/libGLESv2/Context.h (right):

http://codereview.appspot.com/3038042/diff/1/src/libGLESv2/Context.h#newcode433
src/libGLESv2/Context.h:433: egl::Surface* mCurrentSurface;
Iirw, the API we're dealing with here is:
void glMakeCurrent(gl::Context *context, egl::Display *display, egl::Surface
*surface);

To a certain extent, one could say that you always have three globally current
things --- the surface, display and context. So, I suppose an alternative to
mCurrentSurface on the context would be a TLS variable ~= currentTLS in
main.cpp...

On 2010/11/11 21:23:54, apatrick1 wrote:
> The current surface is distinct from the current context. I'm not sure if the
> notion of a context "having" a current surface is right.

[apatrick1, 2010-11-11 22:05:59]

Hi Daniel,

Nat has made some changes to avoid a rendering artifact during window resizing.
The change to esUtil_win32.c, without the rest of the patch, demonstrates the
problem. The rest of the patch fixes it.

There is an issue that libGLESv2 now needs to determine the current surface when
clearing or drawing and I'm not comfortable caching it in the Context from
MakeCurrent but I don't immediately see a better way given that libGLESv2 is not
dependent on libEGL and getCurrentDrawSurface cannot be made virtual due to it
being a free function.

Can you review?

Thanks,

Al

[dgkoch, 2010-11-12 20:52:20]

Hi Nat,
Can you please add yourself to the following groups so that your comments &
reviews make it down the mailing list:
http://groups.google.com/group/angleproject
http://groups.google.com/group/angleproject-review

Perhaps I'm missing something, but can you explain why this can't be solved just
by re-ordering the operations in Surface::swap()?  
 - Wouldn't moving the checkForOutOfDateSwapChain() down after the Present call
have largely the same affect as doing in on the first drawing operation?  
 - If that doesn't work, the other idea would be to do the resize in two steps. 
Draw the existing contents to the flipTexture, then resize (everything but the
flip texture), draw the contents of the texture, and finally resize the texture.

Either of these also have the added bonus of not needing to involve the Context
in any of this resize checking. 

Daniel

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

http://codereview.appspot.com/3038042/diff/9001/src/libEGL/Surface.cpp#newcod...
src/libEGL/Surface.cpp:345: bool
Surface::checkForResizedWindowAndUpdateInterval()
why rename this function at all?

http://codereview.appspot.com/3038042/diff/9001/src/libEGL/Surface.cpp#newcod...
src/libEGL/Surface.cpp:360: if (clientWidth != mWidth || clientHeight != mHeight
|| mPresentIntervalDirty)
it would be better to still use getWidth() and getHeight() here instead of using
mWidth and mHeight directly.

http://codereview.appspot.com/3038042/diff/9001/src/libEGL/Surface.cpp#newcod...
src/libEGL/Surface.cpp:412: // Texcoords are chosen to flip the renderTarget
about its Y axies.
axies -> axis

http://codereview.appspot.com/3038042/diff/9001/src/libEGL/Surface.cpp#newcod...
src/libEGL/Surface.cpp:428: device->SetViewport(&vp);
the SetViewport call shouldn't be necessary here. The "flipState" state block
should have already encapsulated the proper viewport.

http://codereview.appspot.com/3038042/diff/9001/src/libGLESv2/Context.cpp
File src/libGLESv2/Context.cpp (right):

http://codereview.appspot.com/3038042/diff/9001/src/libGLESv2/Context.cpp#new...
src/libGLESv2/Context.cpp:2394: if (mCurrentSurface && getDrawFramebuffer() ==
mFramebufferMap[0]) {
If we really need to do this, I'd prefer you add a getFramebufferZero() similar
to setFramebufferZero() instead of accessing mFramebufferMap directly.

For symmetry with the draw calls, this should probably be down after the error
checking -- just before the applyRenderTarget call.

[nduca, 2010-11-12 21:01:29]

> Perhaps I'm missing something, but can you explain why this can't be solved
just
> by re-ordering the operations in Surface::swap()?  

Consider the following sequence:
1- glDraw 1... N assuming w0,h0
2- glSwap
3- WM_SIZE to w1,h1
4- glDRaw 1... N assumign w1,h1
5- glSwap

The problem with doing the checkFor in glSwap is that you wont' see w1,h1 until
step 5. For cases where w1 > w0 || h1 > h0 (eg growing window), this leads to us
losing pixels.

>  - If that doesn't work, the other idea would be to do the resize in two
steps. 
Perhaps I'm running low on caffiene, but doesn't this approach still drop pixels
because of the above issue? Sorry if I'm missing something...


I'll address the individual comments in a separate reply.

[nduca, 2010-11-12 21:18:43]

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

http://codereview.appspot.com/3038042/diff/9001/src/libEGL/Surface.cpp#newcod...
src/libEGL/Surface.cpp:345: bool
Surface::checkForResizedWindowAndUpdateInterval()
Good point! Silly me.

On 2010/11/12 20:52:20, dgkoch wrote:
> why rename this function at all?

http://codereview.appspot.com/3038042/diff/9001/src/libEGL/Surface.cpp#newcod...
src/libEGL/Surface.cpp:360: if (clientWidth != mWidth || clientHeight != mHeight
|| mPresentIntervalDirty)
On 2010/11/12 20:52:20, dgkoch wrote:
> it would be better to still use getWidth() and getHeight() here instead of
using
> mWidth and mHeight directly.

Done.

http://codereview.appspot.com/3038042/diff/9001/src/libEGL/Surface.cpp#newcod...
src/libEGL/Surface.cpp:412: // Texcoords are chosen to flip the renderTarget
about its Y axies.
On 2010/11/12 20:52:20, dgkoch wrote:
> axies -> axis

Done.

http://codereview.appspot.com/3038042/diff/9001/src/libEGL/Surface.cpp#newcod...
src/libEGL/Surface.cpp:428: device->SetViewport(&vp);
On 2010/11/12 20:52:20, dgkoch wrote:
> the SetViewport call shouldn't be necessary here. The "flipState" state block
> should have already encapsulated the proper viewport.

Done.

http://codereview.appspot.com/3038042/diff/9001/src/libGLESv2/Context.cpp
File src/libGLESv2/Context.cpp (right):

http://codereview.appspot.com/3038042/diff/9001/src/libGLESv2/Context.cpp#new...
src/libGLESv2/Context.cpp:2394: if (mCurrentSurface && getDrawFramebuffer() ==
mFramebufferMap[0]) {
On 2010/11/12 20:52:20, dgkoch wrote:
> If we really need to do this, I'd prefer you add a getFramebufferZero()
similar
> to setFramebufferZero() instead of accessing mFramebufferMap directly.
> 
> For symmetry with the draw calls, this should probably be down after the error
> checking -- just before the applyRenderTarget call.

Done.

[dgkoch, 2010-11-12 21:22:29]

On 2010/11/12 21:01:29, nduca wrote:
> > Perhaps I'm missing something, but can you explain why this can't be solved
> just
> > by re-ordering the operations in Surface::swap()?  
> 
> Consider the following sequence:
> 1- glDraw 1... N assuming w0,h0
> 2- glSwap
> 3- WM_SIZE to w1,h1
> 4- glDRaw 1... N assumign w1,h1
> 5- glSwap
> 
> The problem with doing the checkFor in glSwap is that you wont' see w1,h1
until
> step 5. For cases where w1 > w0 || h1 > h0 (eg growing window), this leads to
us
> losing pixels.

But that'll only work if you get the WM_SIZE message before the first draw call.
 You added a limiter so that the resize check can only happen once per-frame.
As your patch is -- if the WM_SIZE message comes in sometime between glDraw 2…N,
the resize won't get noticed until the first draw call after the next swap.  So
you might as well just move the checkForOutOfDateSwapChain further down in the
swap function immediately after the d3d->present call.   

If you don't have a limiter in, and check *every* draw call, I believe you'll
*still* have lost pixels unless you force a redraw of anything pending when we
get the resize.  But that won't work either since it will likely only be a part
of a scene.  

I believe the only reasonable options are:
1) draw the scene at the old size (with either stretching, scaling or clamping)
2) discard the frame that is being rendered when the window resizes.

> 
> >  - If that doesn't work, the other idea would be to do the resize in two
> steps. 
> Perhaps I'm running low on caffiene, but doesn't this approach still drop
pixels
> because of the above issue? Sorry if I'm missing something…

It all comes back to suddenly adding pixels to the framebuffer in the middle of
the frame -- it isn't well-defined how that should work.

[nduca, 2010-11-15 20:55:51]

OK, I have a new approach figured out where we trap the WM_SIZE command directly
using window subclassing rather than polling for it. This avoids your concerns
about the timing of the WM_SIZE relative to draw calls, but still avoids
artifacts.

Unfortunately, I seem to have confused gcl by removing files from the
changelist. So I'm going to create a new issue for the new approach.

On 2010/11/12 21:22:29, dgkoch wrote:
> On 2010/11/12 21:01:29, nduca wrote:
> > > Perhaps I'm missing something, but can you explain why this can't be
solved
> > just
> > > by re-ordering the operations in Surface::swap()?  
> > 
> > Consider the following sequence:
> > 1- glDraw 1... N assuming w0,h0
> > 2- glSwap
> > 3- WM_SIZE to w1,h1
> > 4- glDRaw 1... N assumign w1,h1
> > 5- glSwap
> > 
> > The problem with doing the checkFor in glSwap is that you wont' see w1,h1
> until
> > step 5. For cases where w1 > w0 || h1 > h0 (eg growing window), this leads
to
> us
> > losing pixels.
> 
> But that'll only work if you get the WM_SIZE message before the first draw
call.
>  You added a limiter so that the resize check can only happen once per-frame.
> As your patch is -- if the WM_SIZE message comes in sometime between glDraw
2…N,
> the resize won't get noticed until the first draw call after the next swap. 
So
> you might as well just move the checkForOutOfDateSwapChain further down in the
> swap function immediately after the d3d->present call.   
> 
> If you don't have a limiter in, and check *every* draw call, I believe you'll
> *still* have lost pixels unless you force a redraw of anything pending when we
> get the resize.  But that won't work either since it will likely only be a
part
> of a scene.  
> 
> I believe the only reasonable options are:
> 1) draw the scene at the old size (with either stretching, scaling or
clamping)
> 2) discard the frame that is being rendered when the window resizes.
> 
> > 
> > >  - If that doesn't work, the other idea would be to do the resize in two
> > steps. 
> > Perhaps I'm running low on caffiene, but doesn't this approach still drop
> pixels
> > because of the above issue? Sorry if I'm missing something…
> 
> It all comes back to suddenly adding pixels to the framebuffer in the middle
of
> the frame -- it isn't well-defined how that should work.