Re: OpenGL child windows (SketchUp)

[Roderick Colenbrander]

Re-adding wine-devel.

On Wed, Feb 8, 2012 at 2:42 PM, Brian Bloniarz  wrote:
> On Wed, Feb 8, 2012 at 12:33 PM, Roderick Colenbrander
>  wrote:
> > On Wed, Feb 8, 2012 at 6:31 PM, Matteo Bruni 
> > wrote:
> > > I'm not really such an expert in the area, but I think both (1) and
> > > (2) make sense. I'm not sure in (1) case about the glXWaitGL call, as
> > > technically glXSwapBuffers is not a GL call, but given that it works
> > > for you (while XSync doesn't) on Nvidia, I guess it's fine... Make
> > > sure it works correctly on all the drivers you mentioned, or even that
> > > e.g. it doesn't cause excessive slowdowns.
>
> ...
> > glXWaitGL and friends were intended for syncing GLX and X, so this may
> >
> > be the most reliable way. As you pointed out there may be bugs in DRI,
> > but this needs a careful read of the specs (will try to look at it).
> > As you said the damage extension can be used as well, but I think I
> > would lean to using the classic glX/X synchronization features first.
>
> Thanks for looking, both of you!
>
> I'm looking at the GLX1.3 spec at:
> http://www.lri.fr/~mbl/ENS/IG2/docs/glx1.3.pdf
>
> "Sequentiality" has:
> "glXSwapBuffers is in the OpenGL stream if and only if
> the display and drawable are those belonging to the calling thread's
> current context; otherwise it is in the X stream."
>
> That would put our glXSwapBuffers in the OpenGL stream,
> and hence something that should be serialized by a call
> to glXWaitGL(). I wouldn't test it in court though :)
>
> Whatever the approach, I'd be happy to test and/or benchmark
> on the 4 renderers I have access to.
>
> Thanks,
> -Brian

A patch like this is what should do the job. I'm not sure what the
penalty will be. Remember glXSwapBuffers already performs a glFlush,
so a glXWaitGL on top of that may not be too bad.

I can't quickly think of a good test application. You would like a
game or some benchmark, but I can't think of anything stressful now
which uses a child window (an application which has a menu bar,
doesn't use a child window). Google Earth always was a good test.

We need to figure out if the DRI drivers are indeed buggy. I will see
if I have time to look at it, but my time is very limited these days
:(

Roderick

OpenGL child windows (SketchUp)

[Brian Bloniarz]

Below is a patch which fixes the visuals for me, it simply uses
glXWaitForSbcOML, falling back to glFinish if that's not supported.

Partway through coding this up, I was thinking that maybe listening
for damage events would be more natural. One reason: the current
code is probably imperfect for single-buffered contexts. The window will get
redrawn any time glFlush or glFinish was called (flush_gl_drawable is
called inside both), but really apps might expect that a single-buffered
window get updated whenever rendering finishes. Damage is naturally
asynchronous (which might avoid a performance hit). I also wonder whether
vsync is easier to make work with that kind of flow (how do compositing
window managers accomplish this? they've got the same problem).

All the same, that might be a more invasive thing (i.e. can you do it
asynchronously? hook into the event loop and intercept damage events).

diff --git a/dlls/winex11.drv/opengl.c b/dlls/winex11.drv/opengl.c
index e8d0f89..c541044 100644
--- a/dlls/winex11.drv/opengl.c
+++ b/dlls/winex11.drv/opengl.c
@@ -256,6 +256,9 @@ MAKE_FUNCPTR(glXGetCurrentReadDrawable)
 static GLXContext (*pglXCreateContextAttribsARB)(Display *dpy, GLXFBConfig config, GLXContext share_context, Bool direct, const int *attrib_list);
 static void* (*pglXGetProcAddressARB)(const GLubyte *);
 static int   (*pglXSwapIntervalSGI)(int);
+static int   (*pglXSwapIntervalSGI)(int);
+static BOOL  (*pglXWaitForSbcOML)(Display *dpy, GLXDrawable drawable, int64_t target_sbc, int64_t *ust, int64_t *msc, int64_t *sbc);
+static BOOL  (*pglXGetSyncValuesOML)(Display *dpy, GLXDrawable drawable, int64_t *ust, int64_t *msc, int64_t *sbc);

/* ATI GLX Extensions */
 static BOOL  (*pglXBindTexImageATI)(Display *dpy, GLXPbuffer pbuffer, int buffer);
@@ -602,6 +605,11 @@ static BOOL has_opengl(void)
         pglXCopySubBufferMESA = pglXGetProcAddressARB((const GLubyte *) "glXCopySubBufferMESA");
     }

+    if(glxRequireExtension("GLX_OML_sync_control")) {
+        pglXWaitForSbcOML = pglXGetProcAddressARB((const GLubyte *) "glXWaitForSbcOML");
+        pglXGetSyncValuesOML = pglXGetProcAddressARB((const GLubyte *) "glXGetSyncValuesOML");
+    }
+
     X11DRV_WineGL_LoadExtensions();

wine_tsx11_unlock();
@@ -2281,6 +2289,11 @@ static void WINAPI X11DRV_wglGetIntegerv(GLenum pname, GLint* params)
     wine_tsx11_unlock();
 }

+/* For offscreen-rendered (child) windows, this copies the window
+ * content into to the onscreen window. The caller must have
+ * performed any OpenGL flushing to ensure the gl_drawable is
+ * up-to-date
+ */
 void flush_gl_drawable(X11DRV_PDEVICE *physDev)
 {
     int w, h;
@@ -2295,10 +2308,7 @@ void flush_gl_drawable(X11DRV_PDEVICE *physDev)
         Drawable src = physDev->pixmap;
         if(!src) src = physDev->gl_drawable;

-        /* The GL drawable may be lagged behind if we don't flush first, so
-         * flush the display make sure we copy up-to-date data */
         wine_tsx11_lock();
-        XFlush(gdi_display);
         XSetFunction(gdi_display, physDev->gc, GXcopy);
         XCopyArea(gdi_display, src, physDev->drawable, physDev->gc, 0, 0, w, h,
                   physDev->dc_rect.left, physDev->dc_rect.top);
@@ -3857,6 +3867,8 @@ BOOL X11DRV_SwapBuffers(PHYSDEV dev)
   wine_tsx11_lock();
   sync_context(ctx);
   if(physDev->pixmap) {
+      /* Child window w/ pixmap rendering
+       * (composite extension unavailable) */
       if(pglXCopySubBufferMESA) {
           int w = physDev->dc_rect.right - physDev->dc_rect.left;
           int h = physDev->dc_rect.bottom - physDev->dc_rect.top;
@@ -3867,14 +3879,33 @@ BOOL X11DRV_SwapBuffers(PHYSDEV dev)
           pglFlush();
           if(w > 0 && h > 0)
               pglXCopySubBufferMESA(gdi_display, drawable, 0, 0, w, h);
+      } else {
+          pglXSwapBuffers(gdi_display, drawable);
       }
-      else
+      flush_gl_drawable(physDev);
+  } else if(physDev->gl_drawable) {
+      /* Composite-redirected child window. We must wait for the buffer swap
+       * since flush_gl_drawable will be copying out the window contents. */
+      if(pglXWaitForSbcOML) {
+          /* Wait for swap buffer with GLX_OML_sync_control extension,
+           * if it's supported. Note that DRI-based renderers like r600g
+           * support this extension and wait for swap on glFinish(). */
+          int64_t ust, msc, sbc;
+          pglXGetSyncValuesOML(gdi_display, drawable, &ust, &msc, &sbc);
           pglXSwapBuffers(gdi_display, drawable);
-  }
-  else
+          pglXWaitForSbcOML(gdi_display, drawable, sbc+1, &ust, &msc, &sbc);
+      } else {
+          /* No GLX_OML_swap_method support, just assume that glFinish() will
+           * wait for the swap. This works on nv & fglrx. */
+          pglXSwapBuffers(gdi_display, drawable);
+          pglFinish();
+      }
+      flush_gl_drawable(physDev);
+  } else {
+      /* normal fullscreen window */
       pglXSwapBuffers(gdi_display, drawable);
+  }

-  flush_gl_drawable(physDev);
   wine_tsx11_unlock();

/* FPS support */

OpenGL child windows (SketchUp)

[Roderick Colenbrander]

Hi,

I have had a quick look. Although I'm not a big fan of the old OpenML
extension. It would be really nice if DRI drivers could be fixed to
get a proper glXWaitGL and friends. It still feels like the OpenML
extension does a little more than we really need. Since all DRI2
drivers seem to support it, we could just use it. I think your code
looks reasonable. In the fallback path we could use glXWaitGL instead
of glFinish, but glFinish is nearly the same anyway. Have you done any
benchmarks?

Single buffered applications are an issue, but from what I remember
Windows Vista and up have the same problem. There were some articles
on what happens on recent Windows versions and I think it was similar
to what we are doing with glFlush/glFinish. I'm not sure if we should
complicate the code too much for those usually old applications.

Roderick

On Thu, Feb 16, 2012 at 6:45 AM, Brian Bloniarz
 wrote:
> Below is a patch which fixes the visuals for me, it simply uses
> glXWaitForSbcOML, falling back to glFinish if that's not supported.
>
> Partway through coding this up, I was thinking that maybe listening
> for damage events would be more natural. One reason: the current
> code is probably imperfect for single-buffered contexts. The window will get
> redrawn any time glFlush or glFinish was called (flush_gl_drawable is
> called inside both), but really apps might expect that a single-buffered
> window get updated whenever rendering finishes. Damage is naturally
> asynchronous (which might avoid a performance hit). I also wonder whether
> vsync is easier to make work with that kind of flow (how do compositing
> window managers accomplish this? they've got the same problem).
>
> All the same, that might be a more invasive thing (i.e. can you do it
> asynchronously? hook into the event loop and intercept damage events).
>
> diff --git a/dlls/winex11.drv/opengl.c b/dlls/winex11.drv/opengl.c
> index e8d0f89..c541044 100644
> --- a/dlls/winex11.drv/opengl.c
> +++ b/dlls/winex11.drv/opengl.c
> @@ -256,6 +256,9 @@ MAKE_FUNCPTR(glXGetCurrentReadDrawable)
>  static GLXContext (*pglXCreateContextAttribsARB)(Display *dpy, GLXFBConfig config, GLXContext share_context, Bool direct, const int *attrib_list);
>  static void* (*pglXGetProcAddressARB)(const GLubyte *);
>  static int   (*pglXSwapIntervalSGI)(int);
> +static int   (*pglXSwapIntervalSGI)(int);
> +static BOOL  (*pglXWaitForSbcOML)(Display *dpy, GLXDrawable drawable, int64_t target_sbc, int64_t *ust, int64_t *msc, int64_t *sbc);
> +static BOOL  (*pglXGetSyncValuesOML)(Display *dpy, GLXDrawable drawable, int64_t *ust, int64_t *msc, int64_t *sbc);
>
> /* ATI GLX Extensions */
>  static BOOL  (*pglXBindTexImageATI)(Display *dpy, GLXPbuffer pbuffer, int buffer);
> @@ -602,6 +605,11 @@ static BOOL has_opengl(void)
>         pglXCopySubBufferMESA = pglXGetProcAddressARB((const GLubyte *) "glXCopySubBufferMESA");
>     }
> - if(glxRequireExtension("GLX_OML_sync_control")) {
> - pglXWaitForSbcOML = pglXGetProcAddressARB((const GLubyte *) "glXWaitForSbcOML");
> - pglXGetSyncValuesOML = pglXGetProcAddressARB((const GLubyte *) "glXGetSyncValuesOML");
> - }
> -
>
> X11DRV_WineGL_LoadExtensions();
>
> wine_tsx11_unlock();
> @@ -2281,6 +2289,11 @@ static void WINAPI X11DRV_wglGetIntegerv(GLenum pname, GLint* params)
>     wine_tsx11_unlock();
>  }
>
> +/* For offscreen-rendered (child) windows, this copies the window
> -
> - content into to the onscreen window. The caller must have
> -
> - performed any OpenGL flushing to ensure the gl_drawable is
> -
> - up-to-date
> - */
>
> void flush_gl_drawable(X11DRV_PDEVICE *physDev)
>  {
>     int w, h;
> @@ -2295,10 +2308,7 @@ void flush_gl_drawable(X11DRV_PDEVICE *physDev)
>         Drawable src = physDev->pixmap;
>         if(!src) src = physDev->gl_drawable;
> - /* The GL drawable may be lagged behind if we don't flush first, so
> - * flush the display make sure we copy up-to-date data */
>
> wine_tsx11_lock();
> - XFlush(gdi_display);
>
> XSetFunction(gdi_display, physDev->gc, GXcopy);
>         XCopyArea(gdi_display, src, physDev->drawable, physDev->gc, 0, 0, w, h,
>                   physDev->dc_rect.left, physDev->dc_rect.top);
> @@ -3857,6 +3867,8 @@ BOOL X11DRV_SwapBuffers(PHYSDEV dev)
>   wine_tsx11_lock();
>   sync_context(ctx);
>   if(physDev->pixmap) {
> - /* Child window w/ pixmap rendering
> - * (composite extension unavailable) */
>
> if(pglXCopySubBufferMESA) {
>           int w = physDev->dc_rect.right - physDev->dc_rect.left;
>           int h = physDev->dc_rect.bottom - physDev->dc_rect.top;
> @@ -3867,14 +3879,33 @@ BOOL X11DRV_SwapBuffers(PHYSDEV dev)
>           pglFlush();
>           if(w > 0 && h > 0)
>               pglXCopySubBufferMESA(gdi_display, drawable, 0, 0, w, h);
> - } else {
> - pglXSwapBuffers(gdi_display, drawable);
>
> }
> - else
> - flush_gl_drawable(physDev);
> - } else if(physDev->gl_drawable) {
> - /* Composite-redirected child window. We must wait for the buffer swap
> - * since flush_gl_drawable will be copying out the window contents. */
> - if(pglXWaitForSbcOML) {
> - /* Wait for swap buffer with GLX_OML_sync_control extension,
> - * if it's supported. Note that DRI-based renderers like r600g
> - * support this extension and wait for swap on glFinish(). */
> - int64_t ust, msc, sbc;
> - pglXGetSyncValuesOML(gdi_display, drawable, &ust, &msc, &sbc);
>
> pglXSwapBuffers(gdi_display, drawable);
> - }
> - else
> - pglXWaitForSbcOML(gdi_display, drawable, sbc+1, &ust, &msc, &sbc);
> - } else {
> - /* No GLX_OML_swap_method support, just assume that glFinish() will
> - * wait for the swap. This works on nv & fglrx. */
> - pglXSwapBuffers(gdi_display, drawable);
> - pglFinish();
> - }
> - flush_gl_drawable(physDev);
> - } else {
> - /* normal fullscreen window */
>
> pglXSwapBuffers(gdi_display, drawable);
> - }
> - flush_gl_drawable(physDev);
>
> wine_tsx11_unlock();
>
> /* FPS support */