Opened 5 months ago

Closed 3 months ago

Last modified 3 months ago

#16193 closed defect (fixed)

remove ASSERT nearly always triggered in wxGLCanvas::SetCurrent

Reported by: Remdul Owned by: VZ
Priority: normal Milestone:
Component: OpenGL Version:
Keywords: assert SetCurrent wxGLCanvas Cc: dgud@…
Blocked By: Blocking:
Patch: yes

Description

A race condition causes an ASSERT to be triggered (sometimes multiple times in succession) when wxGLCanvas::SetCurrent is called in typical usage. This happens mainly on Linux (tested Ubuntu, Linux Mint). This happens because on most Linux platforms, because X does not guarantee a window to be shown, even when it is instructed by wx through ::Show (or other wx methods).

The comment in the code is also incorrect. On Linux, MakeCurrent can be called just fine when a window is invisible. IIRC, it is on *Windows*, that this may be a problem, not Linux.

Secondly, the ASSERT is logically, technically and practically impossible to workaround. While it is possible to delay GL initialization by calling ::SetCurrent in the first canvas ::Paint() even, on all platforms I tested ::Paint() is often triggered *before* the window is visible (e.g. when a window state is changed to maximized on startup). All attempts to force the window to be shown before ::Paint() will fail, because X by design and specification does not guarantee a window to be shown even when it is specifically instructed to do so. ::IsShownOnScreen() will return false, even after a call to ::Show (again because the window manager/X etc hasn't actually made it visible yet; it simply doesn't guarantee that).
Calling wxYield after ::Show will also not help in this situation. Even putting xwYield in a loop, and repeating it for several (milli)seconds will not work. The window is simply never be guaranteed to be visible, ever, and thus the assert WILL be triggered, always. It is only by accident that it won't be triggered; when the X finishes the command queue and shows the window just in time *before* ::SetCurrent is invoked). I suppose thats why this bug hasn't been caught in tests for so long. On all systems I tested the bug happens ~80% to 100%, more frequent depending on CPU usage (the busier X server is, the more frequent). XFCE systems are particularly prone to this issue.

Of course, all of this is moot, since the ASSERT is needless. MakeCurrent can just be called even when the window is invisible, at least on Linux. All applications work fine with this patch applied, including all of my own software.

This bug has been an issue for me since wx2.8.0. I assumed it would be fixed by someone, but strangely it still hasn't. So here it is. I believe this bug was first introduced in wx2.8, presumably because the behavior of ::IsShownOnScreen was changed (judging by SVN log of glcmn.cpp, the assert has always been there).

Attachments (1)

fixglassert.patch download (640 bytes) - added by Remdul 4 months ago.
fix assert

Download all attachments as: .zip

Change History (15)

comment:1 Changed 5 months ago by dgud

  • Cc dgud@… added

I have not had the problem in 2.8 but happened in 2.9 and forward.

I'm using wx on erlang where the call is done asynchronous to the gui thread which makes
things more timing dependent.

But it seems that creating windows is slower in 3.0 branch on linux then it was before,
so I tried messing around with deferred creation but did manage to solve anything.

In the end I worked around it by doing setCurrent in wxShowEvent.

But it would be good if it could be solved.

comment:2 Changed 4 months ago by vadz

Looking at the history (see r45388), I believe the assert is really supposed to check that the window is realized. I.e. I think calling SetCurrent() before Show() is called for the first time is not going to work under Linux (could someone please test this?). So perhaps we really need to check for IsShown() here instead of IsShownOnScreen(), would this fix your problem?

comment:3 Changed 4 months ago by Remdul

vadz, yes it seems the code checks whether the window is visible (to the user). I presume this is was done because this is (was?) a requirement on Windows, where IIRC there must be a window created before a GL context can be created. On Windows this led to a lot of hackery, for example creating a temporary off-screen window+context just to get GL hardware info, enumerating display resolutions etc before actually creating the actual application window. So, big mess. This is also the reason why 99% of most games need to restart when display settings are changed, on Windows this means the GL context must be destroyed (for various reasons I won't go into here). I wrote an article about that here: http://www.bytehazard.com/code/sdlres.html

The cool thing is that this is unnecessary in Linux, because here things 'just work'. One can create a GL context without the need to tie it to a visible window.

Another problem is that X11 doesn't guarantee a window to be ever visible. It seems mind boggling, but I've found various comments in XFCE code that make this claim. I have not actually bothered to look at X11 code to verify this (for obvious reasons to anyone familiar with the X11 code base). From practice, this claim does seem to hold true, on at least two entirely different machines with different desktop environments/window managers (Linux Mint XFCE, Cinnamon) I tested.

So I don't think exchanging IsShownOnScreen() with IsShown() will help much. The window may sometimes be visible (in the rare case where X11 is paying attention), but in other cases may not be. Hence, the ASSERT will still trigger.

So any GL context creation would theoretically need to be delayed to eternity. This is a problem for my application, which is an image viewer. It loads the image at startup and displays it in an wxGL canvas. It also automatically resizes the window to fit the content, or the content to the window. So essentially, with this unnecessary ASSERT my application could never be realized, because it turns into a chicken vs egg problem.

Of course, without the ASSERT (see patch) everything works fine. The ASSERT creates an unnecessary artificial restriction for an apparently non-existing problem.

I would appreciate if others can test my patch on other Linux distributions. I will try to test it under Windows as well.

I will also try your suggestion vadz, just in case I missed something.

comment:4 Changed 4 months ago by vadz

So what you're saying is that actually we just need to check for whether the window is really created at the underlying toolkit level, i.e. GetHandle() != 0? If so, all the better, of course, but I'd really like to test that this works by creating a window, hiding it, making its context current and then showing it.

If you could please test this under both platforms and confirm that it works, it would be great. TIA!

comment:5 Changed 4 months ago by Carsten

Hi all,

it seems that it was essentially my patch that originally introduced the assert that this ticket is about:

According to http://trac.wxwidgets.org/changeset/41031/wxWidgets/trunk/src/msw/glcanvas.cpp, the relevant change was

void wxGLCanvas::SetCurrent()
{
  // although on MSW it works even if the window is still hidden, it doesn't
  // under wxGTK and documentation mentions that SetCurrent() can only be
  // called for a shown window, so check it
  wxASSERT_MSG( GetParent()->IsShown(),
                    _T("can't make hidden GL canvas current") );

  if (m_glContext)
  {
    m_glContext->SetCurrent();
  }
}

Well, I really don't remember all the details after this time and especially not what documentation the above comment was referring to (maybe that of glXMakeContextCurrent()), but as mentioned in the comment, it seems like the problem was exactly opposite from what Remdul proposes.

I'm not sure how or why the above check for IsShown() became IsShownOnScreen() later, but I'm quite sure that, as stated in the comment, Windows was not the problem. (The MSDN documentation linked in the thread mentioned in #7553 does no longer exist, but I believe that I still have a printout somewhere, and iirc it was a concise and detailed explanation of GL context handling under MSW.)

Maybe this problem was specific to GTK (or maybe even to some closed-source vendor driver) at this time (it was 8 years ago, after all), and possibly today it is enough to confirm that this problem no longer exists under (wx)GTK, i.e. that a GL context can be made current even if the targeted window is not (yet) shown.

I agree with Remdul that this assert forces in user code a quite cumbersome approach for OpenGL resource initialization ("lazy" approach, defer e.g. until before the first frame is rendered), which at this time, was however the only option to make GL code work under GTK.

Last edited 4 months ago by Carsten (previous) (diff)

comment:6 Changed 4 months ago by Carsten

Just for additional clarification:

  • I'm using wxGTK 3.0 too, and under Ubuntu from 10.04 to 14.04 have never experienced the problem described by Remdul, and have never heard users of my applications (also with other Linux distributions) complain in this regard.
  • The GL context handling described for Windows is wrong. You need a window in order to create a GL context under MSW, but thereafter, the GL is "self-contained" and the window is no longer needed. I've no readily available proof, but the GL context including all its resources should even survive changes of screen resolution.
  • Remdul, have you tried with IsShown() rather than IsShownOnScreen()?

comment:7 Changed 4 months ago by vadz

  • Priority changed from high to normal

I'm still not sure what to do about this. Having a reproducible test case would be the single most useful thing to do, but failing that, testing with IsShown() would be the second one.

comment:8 Changed 4 months ago by Remdul

Ok, I've tested exchanging IsShownOnScreen() with IsShown(), and...it appears to solve the problem on Linux ! So I was probably wrong in some of my claims above, I must have made some mistakes during testing (note to self: ldconfig!). I'm now going to test the fix on Windows...

@Carsten:

  • Even on my two systems, the problem occurs inconsistently (although frequently). Repro code wouldn't help much; this issue hinges on environment conditions.

The sample on wxWiki would produce this issue though, this bug is probably the source of the problems mentioned on that page: http://wiki.wxwidgets.org/WxGLCanvas

  • Yes, lazy initialization causes boatloads of problems. For example, in one of my other applications I was forced to load configuration files twice. Once to read GL framebuffer settings, then wait until GL is initialized and then load the config file again to load the resources (shaders, texture etc). I ended up rewriting my config file parser, locking/unlocking files and account for race conditions due to the unpredictable startup sequence etc...
  • "[...] but the GL context including all its resources should even survive changes of screen resolution [...]". Yes and no. AFAIK the WGL spec (MSDN) claims that when the pixel format changes, the context may become invalid (i.e. needs to be destroyed). In practice however this often did not happen if only the framebuffer size changed (as opposed to bit depth), which allowed the hack of my article. But I've received at least two reports in which case the driver did implement the spec strictly, and where it failed. I suspect this is also why no vendor ever implemented wglCopyContext. Anyway, it's always been a gray area filled with bugs and deviations from spec. But it's been a while since I abandoned Windows, so AFAIK, IIRC, YMMV etc. Not particularly relevant to this bug, but it's taught me to take the specs/docs with a grain of salt, even though they may be factually correct.

comment:9 Changed 4 months ago by Remdul

Ok, I wasn't able to reproduce the problem without the fix on Windows, but there are no side effects with the fix applied.

So the solution is exchanging IsShownOnScreen() with IsShown.

Changed 4 months ago by Remdul

fix assert

comment:10 Changed 4 months ago by Carsten

I too think that the change to IsShown() is the right thing to do -- but I also agree that a reproducible test case is very important and really the best way to proceed.

Unfortunately, coming up with such a test case that is in fact reproducible is (or at least seams to me) quite difficult, because we would have to test for the failure case, i.e. create a window, create a GL rendering context, then (try to) make it current before the window is shown (expecting failure), then optionally also at various "stages" of showing the window. The failure would indicate that the IsShown() assert is indeed warranted, whereas success is likely not generally useful.

@Remdul: The wxWiki page looks not very useful to me, most of the information and code seems doubtful and of little use. The minimal samples in the wxWidgets repository are a much better basis for test cases or other discussion.

I'll try to come up with a test case, but, sorry, no promises on the deadline.

comment:11 Changed 4 months ago by Remdul

I was going to write a repro, but I decided to give up on wxWidgets, and rewrite my app with Qt. I'm spending more time writing workarounds and fixing bugs than *developing*. I'm trying to write a simple image viewer. Single app window, loads image and displays it. That's all. But I realize this is not possible with wx.

  • I'm getting more than one EVT_SHOW events at app startup, when the window is still invisible. When it does become visible, there's no EVT_SHOW event.
  • I'm getting at least four WX_SIZE events on Linux/X11/GTK. First a resize to 10x10 (?!), then to 20x20 (?!?!), then to the default window size (?), and then to the maximized state(!) I wished for.
  • The EVT_MAXIMIZE event is never triggered on Linux (GTK). ::IsMaximized() returns true when the window is still at 10x10 size (Linux GTK) in WX_SHOW or WX_SIZE. I added a workaround by detecting the desktop size and compare it to the window size in WX_SIZE events, to determine if the windows is maximized. Which no doubt will fail on with multi-display setups...
  • And then of course the wxGL constructor invocation before apparently the parent window exists X server side (this bug).

This makes it impossible to do simple things like image zoom-to-fit in my app, because I can never be sure when the window is at its final size and shown to the user. On Windows of course, the events occur with different frequency and in different order (though more consistently). wxWidgets makes the mistake to rely directly on GTK/X11 API to retrieve the current window state, rather than cache the state internally (while GTK/X11 goes about in its natural chaos in the background) as to produce a reliable set/get interface. wxWidgets should also filter duplicate events to produce a reliable cross-platform in-order event stream.

On topic: I think it's best to just go with the fix I suggested. At worst, it will crash on something else than the assert. I don't think it can make the process less reliable than it already is.

Last edited 4 months ago by Remdul (previous) (diff)

comment:12 Changed 4 months ago by Carsten

Hi Remdul,

I'm somewhat sad to hear this, because while I too have experienced some of the inconsistencies that you mention, the overall situation has in all my time with OpenGL and wxWidgets certainly never presented itself as badly as yours. In fact, this seems all the more a reason to come up with a test case that can reproduce these issues, for example by expanding the OpenGL sample application to log all the events that you mentioned. (This is much easier than addressing the actual GL context issue this ticket is about.)

I think it's best to just go with the fix I suggested. At worst, it will crash on something else than the assert. I don't think it can make the process less reliable than it already is.

While I can very much understand your frustration, I'm sorry to say that I strongly disagree with this conclusion.

comment:13 Changed 3 months ago by VZ

  • Owner set to VZ
  • Resolution set to fixed
  • Status changed from new to closed

In 76643:

Relax checking window visibility in wxGLCanvas::SetCurrent().

It is enough for the window to be shown for SetCurrent() to work, it doesn't
have to be actually visible on screen, and checking for this using
IsShownOnScreen() resulted in false positives.

Closes #16193.

comment:14 Changed 3 months ago by VZ

In 76644:

Relax checking window visibility in wxGLCanvas::SetCurrent().

It is enough for the window to be shown for SetCurrent() to work, it doesn't
have to be actually visible on screen, and checking for this using
IsShownOnScreen() resulted in false positives.

Closes #16193.

Note: See TracTickets for help on using tickets.