xpra icon
Bug tracker and wiki

Opened 6 years ago

Closed 5 years ago

Last modified 4 years ago

#565 closed defect (fixed)

opengl errors after upgrade to trusty and xpra 0.12.5

Reported by: callegar Owned by: callegar
Priority: minor Milestone:
Component: core Version: 0.12.x
Keywords: Cc:

Description (last modified by Antoine Martin)

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).

Change History (24)

comment:1 Changed 6 years ago by Antoine Martin

Description: modified (diff)
Owner: changed from Antoine Martin to callegar

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?
In particular: pygtkglext, PyOpenGL, etc..

Version 0, edited 6 years ago by Antoine Martin (next)

comment:2 Changed 6 years ago by Antoine Martin

Can you try running the client with:

XPRA_ALPHA=0 xpra attach ...

Does it help?

comment:3 Changed 6 years ago by Antoine Martin

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

comment:4 Changed 6 years ago by Antoine Martin

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.

comment:5 Changed 6 years ago by Antoine Martin

Owner: changed from callegar to alas

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

comment:6 Changed 6 years ago by callegar

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.


comment:7 Changed 6 years ago by Antoine Martin

Owner: changed from alas to callegar

From memory this is the best that I can do.


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.

comment:8 Changed 6 years ago by Antoine Martin

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 r6430.

comment:9 Changed 6 years ago by callegar

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

comment:10 Changed 6 years ago by tsaarni

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.

Last edited 6 years ago by Antoine Martin (previous) (diff)

comment:11 Changed 6 years ago by tsaarni

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.

Last edited 6 years ago by tsaarni (previous) (diff)

comment:12 Changed 6 years ago by Antoine Martin

Isn't glEnablei() GL 3.0?


You're right, it is: GLAPI: glEnable

The code that is conditionally calling glEnablei() uses HAS_ALPHA...


You're right again.
Does r6437 fix this properly this time? (and r6438 for v0.12.x)

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

comment:13 Changed 6 years ago by tsaarni

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.

comment:14 Changed 6 years ago by Antoine Martin

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.

Last edited 6 years ago by Antoine Martin (previous) (diff)

comment:15 Changed 6 years ago by tsaarni

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.

comment:16 Changed 6 years ago by Antoine Martin

For a simple temporary fix


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)

comment:17 Changed 6 years ago by callegar

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.

comment:18 Changed 6 years ago by Antoine Martin

The fix for v0.12 is in r6439. I hope. This time. For real.

the client where I was having the issue is pre-opengl 3.0


Still good to know.

comment:19 Changed 6 years ago by tsaarni

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 r6439 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!

comment:20 Changed 6 years ago by Antoine Martin

Sorry for not testing v0.12.x before!


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.

comment:21 Changed 6 years ago by Antoine Martin

  • r6462 + r6468 fix the upload crash (backport in r6469)
  • 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..


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

comment:22 Changed 5 years ago by Antoine Martin

Resolution: fixed
Status: newclosed

I believe the backported fixes in 0.12.6 (see above + re-stride in r6483, r6460 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.

Last edited 5 years ago by Antoine Martin (previous) (diff)

comment:23 Changed 5 years ago by Antoine Martin

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

comment:24 Changed 4 years ago by Antoine Martin

Note: See TracTickets for help on using tickets.