Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Need to redraw twice on resize on osx #1301

Closed
SDLBugzilla opened this issue Feb 10, 2021 · 0 comments
Closed

Need to redraw twice on resize on osx #1301

SDLBugzilla opened this issue Feb 10, 2021 · 0 comments

Comments

@SDLBugzilla
Copy link
Collaborator

SDLBugzilla commented Feb 10, 2021

This bug report was migrated from our old Bugzilla tracker.

Reported in version: 2.0.1
Reported for operating system, platform: Mac OS X 10.8, x86

Comments on the original bug report:

On 2014-01-08 09:42:41 +0000, Daniel Bünzli wrote:

After a resize event on osx. The window needs to be redrawn twice, otherwise the window shows garbage. The following code reproduces the problem.

---- minc.c ----
/*
Compile with:
gcc -o minc minc.c sdl2-config --cflags --libs -framework OpenGL
*/

#include <unistd.h>
#include <assert.h>
#include <stdio.h>
#include <OpenGL/gl3.h>
#include "SDL.h"

void draw (SDL_Window *w)
{
glClearColor (1.0, 0.0, 0.0, 1.0);
glClear (GL_COLOR_BUFFER_BIT);
SDL_GL_SwapWindow (w);
}

void reshape (SDL_Window *w, int width, int height)
{
glViewport (0, 0, width, height);
}

int main(int argc, char** argv)
{
assert (SDL_Init(SDL_INIT_VIDEO) == 0);

SDL_GL_SetAttribute(SDL_GL_CONTEXT_PROFILE_MASK, SDL_GL_CONTEXT_PROFILE_CORE);
SDL_GL_SetAttribute(SDL_GL_CONTEXT_MAJOR_VERSION, 3);
SDL_GL_SetAttribute(SDL_GL_CONTEXT_MINOR_VERSION, 2);
SDL_GL_SetAttribute(SDL_GL_DOUBLEBUFFER, 1);

SDL_Window *w =
SDL_CreateWindow ("SDL OpenGL (C)",
SDL_WINDOWPOS_UNDEFINED, SDL_WINDOWPOS_UNDEFINED,
640, 480,
SDL_WINDOW_OPENGL | SDL_WINDOW_RESIZABLE);
assert (w);

SDL_GLContext *c = SDL_GL_CreateContext(w);
assert (c);

draw (w);
while (1)
{
SDL_Event e;
assert (SDL_WaitEvent (&e));
switch (e.type) {
case SDL_KEYUP: if (e.key.keysym.sym == SDLK_ESCAPE) exit(0); break;
case SDL_WINDOWEVENT:
if (e.window.event == SDL_WINDOWEVENT_RESIZED)
{
reshape (w, e.window.data1, e.window.data2);
draw (w);
/* draw (w) */
}
break;
default: break;
}
}

SDL_GL_DeleteContext (c);
SDL_DestroyWindow (w);
SDL_Quit ();
return 0;
}


On 2014-02-25 20:06:29 +0000, Daniel Bünzli wrote:

Ok. So I found the bug I think, the current NSOpenGLContext update logic that SDL uses seems to be wrong.

That's what currently happens:

Before SDL sends a resize event it schedules an NSOpenGLContext update call:

https://hg.libsdl.org/SDL/file/49af9be342cd/src/video/cocoa/SDL_cocoawindow.m#l376

Then it sends the event, however this scheduled update is not going to happen before the next call to Cocoa_GL_SwapWindow through this call:

https://hg.libsdl.org/SDL/file/49af9be342cd/src/video/cocoa/SDL_cocoaopengl.m#l383

which eventually calls NSOpenGLContext's update method. This means that this update call happens after you have issued your gl commands, when you swap buffers. But the NSOpenGLContext's update call must be done before new redrawing calls are being issued and hence before the resize event is delivered to the client.

This can be easily seen by changing this line:

https://hg.libsdl.org/SDL/file/49af9be342cd/src/video/cocoa/SDL_cocoawindow.m#l55

to simply [context update], it fixes the problem, no garbage can be seen:

hg diff src/video/cocoa/SDL_cocoawindow.m
diff -r f97b5166c158 src/video/cocoa/SDL_cocoawindow.m
--- a/src/video/cocoa/SDL_cocoawindow.m Mon Feb 24 23:09:35 2014 -0800
+++ b/src/video/cocoa/SDL_cocoawindow.m Tue Feb 25 21:04:15 2014 +0100
@@ -52,7 +52,7 @@
NSMutableArray *contexts = data->nscontexts;
@synchronized (contexts) {
for (SDLOpenGLContext *context in contexts) {

  •        [context scheduleUpdate];
    
  •        [context update];
       }
    
    }
    }

So the question is why this scheduling business and updateIfNeeded was introduced ? Is a [context update] update call too heavy ?

In any case it would be nice to have a fix in for 2.0.2.

On 2014-02-25 20:15:33 +0000, Alex Szpakowski wrote:

The commit which added the deferred updating explains why it's necessary: https://hg.libsdl.org/SDL/rev/6abcf951af68

On 2014-02-25 20:34:42 +0000, Daniel Bünzli wrote:

Thanks Alex for the reference. So my understanding is that the ScheduleContextUpdates function should be used for the functions of SDL_cocoawindow.m that the client can directly access (e.g. Cocoa_SetWindowSize) in case it calls them from another thread.

However they should not be used by the methods registered for getting windows notifications, my Cocoa is a little bit rusty and I can't find a reference right now, but IIRC those methods are guaranteed to the be called by the main thread, so there should be no problem in having direct calls to NSOpenGLContext's update in there which is needed to correctly handle resize events.

On 2014-02-25 20:38:43 +0000, Alex Szpakowski wrote:

A typical situation with OpenGL is to do rendering on a completely separate thread from everything else. In that case, the only SDL functions called in that thread would be SDL_GL_SwapWindow (and SDL_GL_MakeCurrent at least once.)

On 2014-02-25 20:50:31 +0000, Daniel Bünzli wrote:

In fact the commit you referred to is trying to do too much. There's no need to call NSOpenGLContext's update on the main thread. According to the docs the only thing one needs to do in a multithreaded setting is to serialize the calls update. See update on this page:

https://developer.apple.com/library/mac/documentation/Cocoa/Reference/ApplicationKit/Classes/NSOpenGLContext_Class/Reference/Reference.html

On 2014-02-25 22:35:03 +0000, (disabled) Jørgen Tjernø wrote:

The docs may be vague or lacking, but we definitely can't call -[NSOpenGLContext update] while someone is doing GL operations on the given NSOpenGLContext. I know this from first hand experiences. Doing so can cause crashes inside the OpenGL driver.

In order to prevent that, we have to find a "synchronization point" during which the caller is saying "Hey, I'm not doing work, so now present this context." They'll do so on the thread where their OpenGL context is active, so any calls into -[NSOpenGLContext update] will be serialized with their other OpenGL calls.

Because of this, we can't just do the update immediately.

On 2014-02-25 22:42:05 +0000, (disabled) Jørgen Tjernø wrote:

The reason it's done inside SDL_GL_SwapWindow is that it's the only entry point that's a) guaranteed to be called by the application, and b) is guaranteed to be serialized with your OpenGL calls.

A "common case improvement" might be to call the update immediately if the context is current on the current thread. I don't know of a good solution to solve this in the general sense without having a SDL_GL_Viewport() call that wraps glViewport(), or a new SDL_GL_UpdateContextShape() that you can call in reshape().

On 2014-02-25 22:56:34 +0000, Daniel Bünzli wrote:

But isn't the current implementation of multihreaded OpenGL with SDL broken on osx anyways ?

From Apple's doc it seems that if you want to issue opengl commands from another thread on an OpenGL context you need to take/release a lock with CGLLockContext/CGLUnlockContext. I see no use of such functions in SDL.

It seems impossible to link to individual chapter in apple docs. But see the section Concurrency and Opengl -> Guidelines for Threading OpenGL Applications in this programming guide:

https://developer.apple.com/library/mac/documentation/graphicsimaging/Conceptual/OpenGL-MacProgGuide/opengl_intro/opengl_intro.html

On 2014-02-25 23:14:14 +0000, Daniel Bünzli wrote:

So actually the answer maybe to provide SDL_GL_{Lock,Unlock}Context and the resizing should try to take the lock to make the call to update. I don't know how it works on other platforms but OpenGL itself is not supposed to be reentrant so a protection is needed anyways if you want to do fancy things with threads and contexts.

On 2014-02-25 23:20:13 +0000, (disabled) Jørgen Tjernø wrote:

The docs you refer to talk about NSOpenGLView, not NSOpenGLContext. They say "Mutex locking is necessary because unless you override the default behavior, the main thread may need to communicate with the view for such things as resizing." -- this is what our -[NSOpenGLContext update] calls do, so that is why we're making sure they don't happen concurrently with OpenGL rendering calls.

Depending on what kind of multithreaded OpenGL code you're talking about, it definitely works. In all Source engine games all OpenGL calls are being made on a separate thread (if you have a multicore system), and we just do the SDL_GL_SwapWindow on the main thread. This caused crashes in the past before the commit Alex linked to, if there were windowing events happening at the same time.

You can work around this by calling SDL_GL_SwapWindow() before glViewport etc. I will change the code so that this is only a requirement if you're using multithreaded rendering, by adding a check for +[NSOpenGLContext currentContext] == context and then doing the -[NSOpenGLContext update] inline instead of using scheduleUpdate.

On 2014-02-25 23:21:39 +0000, Alex Szpakowski wrote:

(In reply to Daniel Bünzli from comment # 8)

But isn't the current implementation of multihreaded OpenGL with SDL broken
on osx anyways ?

From Apple's doc it seems that if you want to issue opengl commands from
another thread on an OpenGL context you need to take/release a lock with
CGLLockContext/CGLUnlockContext. I see no use of such functions in SDL.

What you're describing is if you want to use the same context on multiple threads (which is dangerous!) This is generally the least used method for multi-threading with OpenGL - and it's still possible with SDL using its locking APIs.

Other more popular methods include using multiple completely independent GL contexts on separate threads simultaneously (possible via SDL_GL_MakeCurrent), using multiple shared contexts on separate threads simultaneously (again via SDL_GL_MakeCurrent as well as the shared context SDL_GL_Attribute), and using a single GL context on a separate worker thread independent from the main window's thread with its own render loop, again using SDL_GL_MakeCurrent.

The fact that a context is current on a thread-local basis allows for the multi-threading strategies above. They're outlined here:
https://developer.apple.com/library/mac/documentation/graphicsimaging/Conceptual/OpenGL-MacProgGuide/opengl_threading/opengl_threading.html

On 2014-02-25 23:38:35 +0000, (disabled) Jørgen Tjernø wrote:

(In reply to Daniel Bünzli from comment # 1)

This can be easily seen by changing this line:

https://hg.libsdl.org/SDL/file/49af9be342cd/src/video/cocoa/SDL_cocoawindow.
m#l55

to simply [context update], it fixes the problem, no garbage can be seen

Can you try this: https://hg.libsdl.org/SDL/rev/569354dec4e9

On 2014-02-25 23:48:31 +0000, Daniel Bünzli wrote:

The docs you refer to talk about NSOpenGLView, not NSOpenGLContext. They say "Mutex locking is necessary because unless you override the default behavior, the main thread may need to communicate with the view for such things as resizing." -- this is what our -[NSOpenGLContext update] calls do, so that is why we're making sure they don't happen concurrently with OpenGL rendering calls.

Right, exactly, understood now.

I will change the code so that this is only a requirement if you're using multithreaded rendering, by adding a check for +[NSOpenGLContext currentContext] == context and then doing the -[NSOpenGLContext update] inline instead of using scheduleUpdate.

Yes, I think that's already going to be a nice improvement. Of course I guess most people do not see the problem because they redraw constantly at a given frame rate anyways; the problem shows up only if you don't constantly redraw.

Can you try this: https://hg.libsdl.org/SDL/rev/569354dec4e9

Works !

On 2014-02-25 23:55:52 +0000, (disabled) Jørgen Tjernø wrote:

Resolving as FIXED with https://hg.libsdl.org/SDL/rev/569354dec4e9

On 2014-02-25 23:57:14 +0000, (disabled) Jørgen Tjernø wrote:

Daniel(In reply to Daniel Bünzli from comment # 13)

Yes, I think that's already going to be a nice improvement. Of course I
guess most people do not see the problem because they redraw constantly at a
given frame rate anyways; the problem shows up only if you don't constantly
redraw.

Actually, both Team Fortress 2 and Dota 2 had a corrupted loading screen for a long time, because of this exact bug. It was fixed by just drawing twice.

(The loading screen is just rendered once while things are being loaded.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant