Replace Xlib with XCB for linux sfml-window #319

Closed
wants to merge 4 commits into
from

Projects

None yet
@Lukas-W
Contributor
Lukas-W commented Nov 25, 2012

This patch closes #200

I've ported the linux implementation of sfml-window to xcb as far as it's possible.
As already said in the dicussion of the issue, this is not possible for GLX.
Hence this implementation uses Xlib/Xcb to mix calls of both libraries.

I know this is a very huge commit, so take your time. ;)

Lukas-W added some commits Nov 25, 2012
@Lukas-W Lukas-W Added FindXCB.cmake script 1e568d7
@Lukas-W Lukas-W Added AutoPointer wrapper for automatically free'ing pointers aed64d4
@Lukas-W Lukas-W Huge commit: Ported linux implementation of sfml-window to xcb
* Xcb is now used for window creation, event loop etc
* As GLX is linked to Xlib, that part of the implementation
  still uses Xlib.
  Also, some keyboard related (such as XLookupString) stuff
  is still Xlib, as xcb does not have it (yet?).
d012677
@Lukas-W Lukas-W Replaced some enums with the xcb equivalents 89b5a18
@LaurentGomila
Member

This is huge, thank you!

I'll need some time to check your modifications, so don't expect this pull request to be processed soon.

@Lukas-W
Contributor
Lukas-W commented Nov 25, 2012

Hmm... I think I stumbled upon a bug:

sf::Window window;
window.create(sf::VideoMode(1366, 768), "Window");
/* ... Later on ... */
window.create(sf::VideoMode(1366, 768), "Window");
// For some reason, this does not work on the second window:
window.setMouseCursorVisible(false);

It does work though, when calling sf::pollEvent(sf::Event& event) between window.create and window.setMouseCursorVisible. I honestly have no idea why but I will be looking into it.

Just to let you know that it should not be merged like how it is now.

Edit: Seems like a while(xcb_generic_event_t event = xcb_poll_for_event(m_connection)); (emptying the event queue)
in the WindowImplX11 constructor fixes it, I consider this a workaround though :/

@mjbshaw
mjbshaw commented May 14, 2013

Two questions after reading a bit of the diff:

Shouldn't the file author get the copyright (so AutoPointer.cpp/hpp would be Copyright Lukas instead of Laurant)?
Is AutoPointer really necessary? It's missing a copy constructor, doesn't check for self assignment, and as far as I can tell, doesn't have any advantage over std::auto_ptr.

@retep998

std::auto_ptr is deprecated in C++11 so there's no reason to use that.
It would be much better to use std::unique_ptr or std::shared_ptr, but we can't use those until SFML uses C++11.
Thus we're stuck in the situation where the old stuff shouldn't be used and the new stuff can't be used, so I assume that's why AutoPointer is used.

@LaurentGomila
Member

I think you're too strict. std::auto_ptr is deprecated if you can use C++11 classes. If not, it's the only option so it's better to use it rather than writing your own buggy class.

@vonj
vonj commented May 14, 2013

For larger patches it is very important that the author states license.

@LaurentGomila
Member

I don't want to deal with a giant mess of different pieces of code from different authors using different license. Patches that I merge are integrated to the sources as if I had written them myself. The only thing that can be done is to add your name to the authors, in case you're maintaining your own part (like the OS X port, for example).

@vonj
vonj commented May 14, 2013

Sorry, copyright does not work that way. The normal thing for projects to do, is that the project (you) is to keep track in the top of the file substantial authors. Also, it is common for the project (you) to state, explicitly, that only patches with a certain license are accepted, and perhaps also that any submitted patches are assumed to be under a certain license. This way, there is no mess of licenses. For instance the Wine project requires that first time submitters declare on their mailing list that all their patches are licensed under the LGPL license. But the files still contain the authors, because without attributed authors copyright law falls down.

The other, less common way, is for the project (you) to request that authors give up their copyright. This is actually trickier because it depends on jurisdiction how you do that. For instance the Free Software Foundation requires that you send in a piece of paper giving up copyright on your patches, before you are allowed to contribute to their core projects. They will then license their work copyright the Free Software Foundation.

Of course, any project on github is free to not be very particular about licensing, but any project which hopes to be used commercially, should have the licensing in order.

@mantognini
Member

It's probably not the right place to discuss such matter but here are my two cents anyway:

If someone creates a pull request that changes some files, but (s)he did not edit the copyright headers, then (s)he tacitly gives the property or her/his code to Laurent when Laurent accepts the pull request – that should make the licence still valid.

(note: I've nothing to back up this thought..)

@vonj
vonj commented May 14, 2013

It all comes down to interpretation of law, but I would agree with that the license should be considered unchanged.

But keep in mind that giving copyright is something ENTIRELY different. In US law only possible with paper work, in Swedish law only partially possible. ("Economic copyright" can be transferred, but not the "origin copyright".) It's best not to transfer copyrights but instead focus on the license.

Also I would state in some kind of FAQ and README that only the zlib/png license is acceptable to use by contributors and that no patches should be sent which are licensed with zlib/png license.

@mjbshaw
mjbshaw commented May 14, 2013

vonj said pretty much everything I would have.

Original authors of files should have copyright. Contributors who have made substantial modifications to a file might also get copyright. Transferring copyright is tricky, internationally.

For example, FFmpeg's fft.c file is copyrighted by both Fabrice Bellard (the original author) and Loren Merritt (who has made many significant modifications to the file), and still licensed under the LGPL. I think it's a decent example that things can be done such that you don't have to mess with tricky copyright transfer issues, but still license everything under one license and don't have to deal with multiple licenses. As maintainer, you can always reject something that's not under the right license.

My 2¢.

@Lukas-W
Contributor
Lukas-W commented May 14, 2013

About the comments on AutoPointer:
As the XCB library is a C library, memory of returned pointers has been allocated using malloc(). std::auto_ptr uses delete in its destructor. As far as I know, using delete on memory allocated using malloc() results in undefined behaviour. That's why I wrote a wrapper that's using free() instead.
The implementation might be buggy, I'll have a look on it if no one else does in the near future.

About the copyright discussion:
I'd say Marco (mantognini) is right. I created this pull request and wrote the code with putting not my name but Laurent's in it. By doing this I obviously gave the code to Laurent, granting him any right and giving him the permission to use the code at will. I don't consider this a very big deal, just usual informal stuff.

@mjbshaw
mjbshaw commented May 14, 2013

Woops, you're right about free() vs delete with std::auto_ptr. I forgot it doesn't allow a custom deleter. But yeah, it does need a copy constructor (should probably make it non-copyable, or make the pointer mutable and mutate the original AutoPointer in the copy constructor to transfer ownership) and to check for self assignment.

@retep998

When you do eventually move to C++11, std::unique_ptr supports having a custom deleter.

@archshift

What's the status on this pull request?

@Mischa-Alff
Contributor

This really needs to be merged. SFML claims to be modern, but Xlib hasn't been updated since 2002.

Can one of the devs please give an update?

@TankOs
Member
TankOs commented Apr 24, 2014

I will look into this.

@Bromeon Bromeon assigned TankOs and unassigned LaurentGomila Apr 24, 2014
@Bromeon
Member
Bromeon commented Apr 24, 2014

TankOs, with the "Assignee" feature of GitHub, you can easily get an overview of your tasks :)

I think you already know this though (and assigned you now). Thanks, by the way!

@TankOs
Member
TankOs commented Apr 24, 2014

Aye, thanks. ;-)

@TankOs
Member
TankOs commented Apr 24, 2014

I rebased the pull request onto master and fixed some bugs. There are still some minor things to do:

  • Remove AutoPointer (this has been discussed already), use free() instead.
  • Fix X11 example. I'm not that experienced with XCB. If there's somebody who can do this rather quickly, then feel free to do it. Just make sure to drop a message here!

You can test the code by fetching the feature/xcb branch.

@Bromeon
Member
Bromeon commented Apr 25, 2014

Generally, I'm for RAII wrappers, but the one here is too specific and not really reusable, so I'd use free() as well. But we should definitely reconsider the use of std::unique_ptr as soon as SFML starts to use C++11.

By the way, tell me if I can help you with the free() conversion. Unfortunately, I have no experience with XCB, either...

@TankOs
Member
TankOs commented Apr 25, 2014

I agree and that's also what I had in mind. I think the conversion won't be the problem, it's the X11 that's a little bit more difficult. ;)

@TankOs TankOs removed their assignment May 9, 2014
@TankOs
Member
TankOs commented May 9, 2014

I'm almost done, but I'm really stuck with the X11 example. It does run on my system and displays everything properly, but as soon as I press a key, it crashes the X server and freezes my whole system.

I'd like to let someone else with more experience in this field make investigations. You can base your work on the feature/xcb branch.

@Lukas-W
Contributor
Lukas-W commented May 9, 2014

Even though I sadly don't have the time to help, I thought I'd just say how nice it is that there's finally something happening with this issue. 👍

Has someone looked at the bug I mentioned in my first comment?

@TankOs
Member
TankOs commented May 9, 2014

Nope, but I guess I will give this one another try -- can't give up. ;) Hopefully this is runnable in a virtual machine, otherwise I will have to do a lot of reboots. ;)

@TankOs TankOs was assigned by Lukas-W May 9, 2014
@Mischa-Alff
Contributor

The X11 example segfaults and closes for me. No error, no X server crashes. The window opens, segfault happens, and it closes.

I'm on Archlinux x86_64 (not in a VM), using nvidia drivers and XServer version 1.15.1.

@eXpl0it3r
Member

@Mischa-Alff What's the call stack from the segfault?

@Mischa-Alff
Contributor

Here's my backtrace:

Program received signal SIGSEGV, Segmentation fault.
0x00007ffff622a134 in xcb_send_request () from /usr/lib/libxcb.so.1
(gdb) bt
#0  0x00007ffff622a134 in xcb_send_request () from /usr/lib/libxcb.so.1
#1  0x00007ffff622ecbe in xcb_intern_atom () from /usr/lib/libxcb.so.1
#2  0x00007ffff7bcbecf in sf::priv::WindowImplX11::initialize (this=0x83bee0) at /tmp/SFML/src/SFML/Window/Unix/WindowImplX11.cpp:591
#3  0x00007ffff7bca6ca in sf::priv::WindowImplX11::WindowImplX11 (this=0x83bee0, handle=65011714) at /tmp/SFML/src/SFML/Window/Unix/WindowImplX11.cpp:124
#4  0x00007ffff7bc6423 in sf::priv::WindowImpl::create (handle=65011714) at /tmp/SFML/src/SFML/Window/WindowImpl.cpp:78
#5  0x00007ffff7bc5a9a in sf::Window::create (this=0x7fffffffe140, handle=65011714, settings=...) at /tmp/SFML/src/SFML/Window/Window.cpp:134
#6  0x00007ffff7bc586e in sf::Window::Window (this=0x7fffffffe140, handle=65011714, settings=...) at /tmp/SFML/src/SFML/Window/Window.cpp:72
#7  0x0000000000401c15 in main () at /tmp/SFML/examples/X11/X11.cpp:156
@LaurentGomila
Member

A potential problem is that XSetEventQueueOwner(XCBOwnsEventQueue) is called, but the X11 example uses X functions for event handling. This is clearly disallowed in the documentation of the function.

The problem is �that SFML will call XCB event functions internally, but this cannot be done if X event functions are used in user code.

Or maybe I'm plain wrong because each side has its own separate connection to the X server?

@TankOs
Member
TankOs commented May 10, 2014

@LaurentGomila I actually even thought of that and have started to use XCB calls instead of Xlib calls in the example, but I failed to get it to work. I guess I will have to try again. Thanks for investigating, also to @Mischa-Alff.

@binary1248 binary1248 added the Linux label May 19, 2014
@abodelot
Contributor

I've tested the branch feature/xcb, here's some feedback:

  • A window created with sf::Style::None is still displayed with its title bar (but the "maximize" button is disabled).
  • A window created with sf::Style::Fullscreen doesn't stretch to fit the screen resolution, but keeps the same size than the sf::VideoMode resolution (and the window is unmovable and without title bar).
  • Alt-tab is still not possible (seems issue #425 is holding on this one...).
@TankOs
Member
TankOs commented May 27, 2014

@abodelot Thanks for testing, I'm going to investigate that.

@computerquip

Just for comments, Xlib actually uses xcb internally now adays. It's nice to use xcb directly but when dealing with GLX and when you don't take advantage of xcb's asynchronous nature, there's not a whole lot of benefit.

@Mischa-Alff
Contributor

Actually, there is.
I started working on Clipboard support for Win32. This was trivial, took a few minutes. Then I tried working with X11. Xlib's outdated, archaic API makes this a very stupid procedure, and so I looked for alternatives. XCB came up, and I wanted to find out why this branch hadn't been merged.

Anyways, XCB is much more modern, and although you can argue that the switch is currently useless, it makes updating SFML's windowing module much easier.

@Lukas-W
Contributor
Lukas-W commented Jun 8, 2014

Xlib actually uses xcb internally now adays.

No, that's not true afaik. I guess you misunderstood something there.

@computerquip

From http://xorg.freedesktop.org/wiki/guide/xlib-and-xcb/

Xlib and XCB compatibility was achieved by rebuilding libX11 as a layer on top of libxcb. Xlib and XCB share the same X server connection and pass control of it back and forth. That option was introduced in libX11 1.2, and is now always present (no longer optional) since the 2010 release of libX11 1.4.

@Lukas-W
Contributor
Lukas-W commented Jun 30, 2014

Hm, okay. Are you sure this paragraph is not about XlibXcb?

@TankOs
Member
TankOs commented Aug 27, 2014

Seems like XCB support is now complete. I've squelched the remaining bugs and adjusted the X11 example to use XCB calls.

It would be nice if someone can test this (especially the example "X11"). Works for me on Arch Linux 64 bit.

Code is here: https://github.com/LaurentGomila/SFML/tree/feature/xcb

@Bromeon Bromeon referenced this pull request Aug 27, 2014
@Lukas-W @TankOs Lukas-W + TankOs Huge commit: Ported linux implementation of sfml-window to xcb
* Xcb is now used for window creation, event loop etc
* As GLX is linked to Xlib, that part of the implementation
  still uses Xlib.
  Also, some keyboard related (such as XLookupString) stuff
  is still Xlib, as xcb does not have it (yet?).
52f9a53
@dabbertorres
Contributor

Works for me on Arch Linux 64 bit with Cinnamon 2.2.14.

@TankOs
Member
TankOs commented Aug 28, 2014

Thanks for testing, @dabbertorres. It's also working on Ubuntu, tested by an IRC user. I'm going to clean up the commits and send in a pull request.

@TankOs
Member
TankOs commented Aug 28, 2014

Thanks a lot for your work @Lukas-W, I've created a new pull request with the fixes etc: #694. Will close this for now -- I still have the original commits if someone needs them.

@TankOs TankOs added the s:superseded label Aug 28, 2014
@TankOs TankOs closed this Aug 28, 2014
@eXpl0it3r eXpl0it3r added this to the 2.3 milestone Jan 7, 2015
@TankOs TankOs was unassigned by Lukas-W Apr 30, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment