opengl errors after upgrade to trusty and xpra 0.12.5

[totaam]

Issue migrated from trac ticket # 565

component: core | priority: minor | resolution: fixed

2014-05-04 10:39:39: callegar created the issue

---

Hi, after upgrading my ubuntu machines (client and server to trusty) and after upgrading xpra too from 0.12.4 to 0.12.5, I am getting the following

2014-05-04 11:32:30,985 do_paint_rgb32 error
Traceback (most recent call last):
  File "/usr/lib/python2.7/dist-packages/xpra/client/window_backing_base.py", line 235, in do_paint_rgb32
    success = self._do_paint_rgb32(img_data, x, y, width, height, rowstride, options, callbacks)
  File "/usr/lib/python2.7/dist-packages/xpra/client/gl/gl_window_backing.py", line 427, in _do_paint_rgb32
    return self._do_paint_rgb(32, img_data, x, y, width, height, rowstride, options, callbacks)
  File "/usr/lib/python2.7/dist-packages/xpra/client/gl/gl_window_backing.py", line 492, in _do_paint_rgb
    self.present_fbo(drawable)
  File "/usr/lib/python2.7/dist-packages/xpra/client/gl/gl_window_backing.py", line 376, in present_fbo
    glEnablei(GL_BLEND, self.textures[TEX_FBO])
  File "/usr/lib/python2.7/dist-packages/OpenGL/platform/baseplatform.py", line 384, in __call__
    self.__name__, self.__name__,
NullFunctionError: Attempt to call an undefined function glEnablei, check for bool(glEnablei) before calling
2014-05-04 11:32:35,043 do_paint_rgb32 error

with this xpra does not paint its windows. Disabling opengl in the client makes xpra work.

The setup is as follows. opengl appears to be only available in the client (the server uses xdummy, but has nvidia graphics with the legacy driver and I haven't succeeded in getting opengl working with xdummy here).

[totaam]

2014-05-04 10:42:26: totaam changed owner from antoine to callegar

[totaam]

2014-05-04 10:42:26: totaam edited the issue description

[totaam]

2014-05-04 10:42:26: totaam commented

---

That's very strange as there was not a single update to the client code, let alone the opengl code.

Are you sure that there weren't other system updates from Ubuntu or nvidia?
It looks to me like OpenGL is not working on your setup. glEnablei should always be available.

[totaam]

2014-05-04 10:48:47: totaam commented

---

Can you try running the client with:

XPRA_ALPHA=0 xpra attach ...

Does it help?

[totaam]

2014-05-04 11:00:31: totaam commented

---

Please also post the output of xpra/client/gl/gl_check.py

[totaam]

2014-05-04 15:07:13: totaam commented

---

r6343 disables alpha when glEnablei is missing. I still don't understand why it would go missing, but this should workaround it.

Please let me know if that helps and I'll backport it.

[totaam]

2014-05-04 15:52:27: totaam changed owner from callegar to afarr

[totaam]

2014-05-04 15:52:27: totaam commented

---

afarr / smo: do you guys have trusty installed with opengl drivers somewhere?
callegar: please let us know your graphics card and drivers.

[totaam]

2014-05-04 21:58:25: callegar commented

---

Hi, unfortunately I will not be on that trusty machine for some days. As soon as I'm on it I'll make the test.

In the meantime, some answers.

Are you sure that there weren't other system updates from Ubuntu or nvidia?

Yes, they were, tons of them, since I moved from ubuntu saucy to trusty and at the same time I upgraded xpra.

glxinfo and glxgears appeared to work on the machine. So KDE effects. So opengl seemed fine.

The xpra info windows, finds the python opengl module and can correctly identify the opengl version (2.1, if I remember correctly).

The machine has an integrated NVIDIA Geoforce 7025 chipset and is using the proprietary NVIDIA driver.

From memory this is the best that I can do.

[totaam]

2014-05-05 07:03:34: totaam changed owner from afarr to callegar

[totaam]

2014-05-05 07:03:34: totaam commented

---

From memory this is the best that I can do.
[[BR]]
Thanks, let's wait for the rest of the info. I think r6343 is the right fix for this, I just want to understand why glEnablei is missing before I apply it.

[totaam]

2014-05-09 14:32:54: totaam commented

---

Did you have a chance to test it? Can I close this ticket?
r6343 makes sense, so I have applied it to v0.12.x in 6430.

[totaam]

2014-05-09 15:30:48: callegar commented

---

Sorry, no way to test on that machine yet, since I'm away.
I hope I'll be able to try shortly, though.

[totaam]

2014-05-10 08:57:52: tsaarni commented

---

I get the same NullFunctionError on my machine as callegar reported.

Client is ubuntu 14.04 with Intel HD Graphics (i915).

$ python /usr/lib/python2.7/dist-packages/xpra/client/gl/gl_check.py
2014-05-10 10:38:02,858 PyOpenGL warning: missing accelerate module
2014-05-10 10:38:02,858 PyOpenGL warning: missing array format handlers: numeric, vbo, vbooffset
2014-05-10 10:38:02,858 OpenGL Version: 2.1 Mesa 10.1.0
2014-05-10 10:38:02,862 
2014-05-10 10:38:02,862 
2014-05-10 10:38:02,862 OpenGL properties:
2014-05-10 10:38:02,863   pygdkglext_version       : (1, 1, 0)
2014-05-10 10:38:02,863   has_alpha                : True
2014-05-10 10:38:02,863   vendor                   : Intel Open Source Technology Center
2014-05-10 10:38:02,863   gdkgl_version            : (1, 4)
2014-05-10 10:38:02,863   shading language version : 1.20
2014-05-10 10:38:02,863   opengl                   : (2, 1)
2014-05-10 10:38:02,863   GLU extensions           : GLU_EXT_nurbs_tessellator GLU_EXT_object_space_tess 
2014-05-10 10:38:02,864   renderer                 : Mesa DRI Intel(R) Ironlake Mobile 
2014-05-10 10:38:02,864   rgba                     : True
2014-05-10 10:38:02,864   display_mode             : ['ALPHA', 'SINGLE']
2014-05-10 10:38:02,864   pyopengl                 : 3.0.2
2014-05-10 10:38:02,864   GLU version              : 1.3

So GL 2.1 here too. Isn't glEnablei() GL 3.0?

I'm using xpra for the first time now, so I don't know if it worked before trustu release.

[totaam]

2014-05-10 10:58:47: tsaarni commented

---

I fetched the trunk but it still has the bug.

The code that is conditionally calling glEnablei() uses HAS_ALPHA value that originates from GTKWindowBacking class instead of the global variable set in gl_check.py. See constructor of ClientWindowBase. It uses GTKWindowBacking.HAS_ALPHA which does not consider, nor is updated according to the gl_check variant of the HAS_ALPHA.

I see from SVN log that some refactoring has been done in recent days but this problem remains.

[totaam]

2014-05-10 13:33:11: totaam commented

---

Isn't glEnablei() GL 3.0?
[[BR]]
You're right, it is: GLAPI: glEnable
[[BR]]

The code that is conditionally calling glEnablei() uses HAS_ALPHA...
[[BR]]
You're right again.
Does r6437 fix this properly this time? (and 6438 for v0.12.x)

If so, I'll try to make new 0.12.x release next week.

[totaam]

2014-05-10 20:57:51: tsaarni commented

---

Thanks for your response,

Unfortunately it still did not fix the bug. I found two remaining problems:

1. In r6437 copy of HAS_ALPHA is made from the global variable before call to gl_check.check_GL_support() has been made. Therefore GLPixmapBacking.HAS_ALPHA is set to True which is the default value for gl_check.HAS_ALPHA.

I know this isn't nice thing to do, but following works around this problem:

--- xpra/client/gl/gl_check.py  (revision 6438)
+++ xpra/client/gl/gl_check.py  (working copy)
@@ -216,6 +216,8 @@
             log.warn("OpenGL glEnablei is not available, disabling transparency")
             global HAS_ALPHA
             HAS_ALPHA = False
+            from gl_window_backing import GLPixmapBacking
+            GLPixmapBacking.HAS_ALPHA = False            
 
         #check for framebuffer functions we need:
         from OpenGL.GL.ARB.framebuffer_object import GL_FRAMEBUFFER, \

2. Second remaining problem is that the _has_alpha value is still overwritten in call ClientWindowBase.set_metadata(metadata) since metadata["has-alpha"] is true. It will change the value back to True.

[totaam]

2014-05-11 05:59:37: totaam commented

---

Oh, dear, that's embarrassing. You're right again!

I remember thinking when I did the refactoring, that there were too many attributes carrying almost the same information, and that this led to confusion and probably also bugs:

• HAS_ALPHA in gl_check is meant to tell us when it is safe to use alpha (can be disabled by env var)
• has_alpha on the client window, is meant to represent the server-side transparency flag: whether this window application should be painted with alpha. (wrongly set to False when we fail I think)
• HAS_ALPHA on the backing class is meant to tell us if this particular backing class implementation is capable of painting with alpha (should mirror the gl_check one)
• has_alpha on the window backing is meant to tell us if alpha channel painting is actually enabled (it should not be enabled if the window does not have alpha, or if we failed the gl check - and there is one more check as we instantiate the gl backing.. not sure if it is still necessary though)
• the client property encoding.transparency is what we send to the server to tell it that it is worth sending the alpha channel (and we do this from the wrong place.. from the window, now taking into account the backing)
• the client property encodings.rgb_formats is what we send to the server to tell it what pixel formats we can accept, and if there are no compatible pixel formats with alpha, you won't be able to see it. It should probably be exposed by the backing, not the window.

So... it's messy and I need to take a good look at this. Renaming some of these variables might help. For 0.12.x backport, something more simple will have to do.

[totaam]

2014-05-11 06:50:07: tsaarni commented

---

Thanks,

For a simple temporary fix, would something like following make sense:

--- xpra/client/gl/gl_window_backing.py (revision 6438)
+++ xpra/client/gl/gl_window_backing.py (working copy)
@@ -385,7 +385,8 @@
         glBindTexture(GL_TEXTURE_RECTANGLE_ARB, self.textures[TEX_FBO])
         if self._has_alpha:
             # support alpha channel if present:
-            glEnablei(GL_BLEND, self.textures[TEX_FBO])
+            if bool(glEnablei):
+                glEnablei(GL_BLEND, self.textures[TEX_FBO])
             glBlendFunc(GL_SRC_ALPHA, GL_ONE_MINUS_SRC_ALPHA)
         glBegin(GL_QUADS)
         glTexCoord2i(0, h)

that is, move the PyOpenGL test down to the actual call site.

[totaam]

2014-05-11 14:28:40: totaam commented

---

For a simple temporary fix
[[BR]]
Yes, that would do very nicely for 0.12. Probably makes sense to also indent glBlendFunc.

Note: for trunk, I'll continue with the flag cleanup instead. Why is this important: knowing in advance if you will be able to use alpha makes a huge difference in terms of server encoding performance as none of the video encodings we use support alpha ( well, vp9 and h265 could, but they are far too slow to be relevant at this point) so would be forced to use PNG or WEBP (or even plain RGBA) to send the alpha channel (and all for nothing if the client cannot make use of it)

[totaam]

2014-05-12 10:58:51: callegar commented

---

I guess that my comments are now completely irrelevant since everything is very well diagnosed. However, I checked. And indeed the client where I was having the issue is pre-opengl 3.0. On another client with recent Intel graphics supporting opengl 3.0 everything is fine.

[totaam]

2014-05-12 14:00:52: totaam commented

---

The fix for v0.12 is in 6439. I hope. This time. For real.
[[BR]]

the client where I was having the issue is pre-opengl 3.0
[[BR]]
Still good to know.

[totaam]

2014-05-12 18:27:09: tsaarni commented

---

I fetched tags/v0.12.x and it works great!

Funny thing is that now also the self._has_alpha value in GLPixmapBacking is False as it should. So I think previous fix to v0.12.x must have already fixed it and 6439 is not even necessary. Until now, I only tested trunk, where the has_alpha value was True. Sorry for not testing v0.12.x before!

[totaam]

2014-05-13 10:42:04: totaam commented

---

Sorry for not testing v0.12.x before!
[[BR]]
No problem, thanks for testing. The extra check can't do any harm, so I'll just keep it in.

---

Here's what I did for trunk to try to clean things up (see r6444 and r6445, fixups in r6449 and r6451), it adds a little bit more code but I think it makes sense. Documenting here for lack of a better place:

• global HAS_ALPHA variable has been renamed to GL_ALPHA_SUPPORTED and GTK_ALPHA_SUPPORTED in each backing implementation module: the former is only ever set from the value we get from gl_check, the latter is a pure platform check for now. Both can still be toggled using the XPRA_ALPHA=0|1 env var.
• the widget classes (trays and windows) still use an _has_alpha flag, but now we try to ensure that this does not get updated (and log a warning if it does), and that it represents the server-side alpha availability only - nothing else
• if the server side window has alpha, and if the backing class supports it, the widget class tries to enable alpha (with GTK windows we have to find an RGBA colormap, system trays always have alpha enabled already) then it sets the window_alpha flag, and passes it to the backing class when (re)instantiating it
• the backing should try to honour the "window_alpha" flag it receives if it can (after checking availability again upon instantiation), and must set its own flag to reflect that (renamed the flag to _alpha_enabled to try to prevent confusion)
• we now let the backing implementation define what pixel formats it supports, rather than adding the logic to the window class: just call get_encoding_properties on the backing when needed.

I have also changed a few warnings to be visible by default, as I think we should now be unable to hit them, and are therefore good candidates for complete removal.

Note: we could probably handle the change in alpha for a window by re-initializing its backing, but I don't think that X11 server side windows can change their alpha without making a new window. (well at least not for the xpra X11 unix server case, which is the main target.. for shadow servers, you will have to re-connect if the bpp is change from 24 to 32)

---

Where this gets really interesting: in adding BGRX to the list of pixel formats we can upload with GL, I've uncovered two more bugs (will this ever end?):

• we ended up sending plain BGRX data but with the wrong buffer size (because of padding / stride issues), this is fixed in r6448 (probably needs backport..)
• we often end up sending BGRX raw - with the rowstride we get from the X11 image buffer... even if the pixels we actually send are much much smaller: a 1x499 pixel buffer will use the default rowstride of almost 2K and waste about 600KB! This is bad, but made much worse by the fact that it completely crashes the client during pixel upload (at least with my nvidia card) - so I've disabled BGRX in r6447 until I can fix both of these issues.

[totaam]

2014-05-15 14:07:22: totaam commented

---

• r6462 + r6468 fix the upload crash (backport in 6469)
• r6474 re-strides the RGB pixel data to avoid sending so much padding, the savings are huge:

rgb_encode: BGRX pixels re-stride saving 70% from 1996 (25948 bytes) to 600 (7800 bytes)
rgb_encode: BGRX pixels re-stride saving 100% from 1996 (630736 bytes) to 4 (1264 bytes)
rgb_encode: BGRX pixels re-stride saving 99% from 1996 (25948 bytes) to 24 (312 bytes)
rgb_encode: BGRX pixels re-stride saving 81% from 1996 (25948 bytes) to 384 (4992 bytes)
rgb_encode: BGRX pixels re-stride saving 97% from 1996 (25948 bytes) to 72 (936 bytes)

Unfortunately this one is bigger but should also be backported to v0.12.x, or newer clients (0.13 onwards) will not work too well with older servers..

[[BR]]
I think this ticket can now be closed, feel free to re-open if I've missed anything.

[totaam]

2014-05-17 10:06:32: totaam changed status from new to closed

[totaam]

2014-05-17 10:06:32: totaam changed resolution from ** to fixed

[totaam]

2014-05-17 10:06:32: totaam commented

---

I believe the backported fixes in 0.12.6 (see above + re-stride in 6483, 6460 for adding the intel driver to the blacklist) should prevent all the issues mentioned in this ticket. If not, feel free to re-open it.

Forgot cairo, fixed in r6511.

[totaam]

2014-05-27 08:50:43: totaam commented

---

Forgot toggling opengl on / off, which broke.. see #578

[totaam]

2015-12-16 03:39:57: antoine commented

---

See ClientRendering OpenGL