Previously used x11 plugin does not compile with Qt5, because Qt5 does not use Xlib, it uses xcb. Rewrite screen capture plugin from Xlib to xcb.
I made xcb libs compile required dependency, but availability of X shared memory extension is checked at runtime. It is used to effectively get image pixels data, instead of transfering 8Mb over the wire. Xdamage is used to limit image getting operations only within actually changed rectangles of screen.
BUG: 377998
Tested on single-monitor system and dual-monitor, where primary monitor does not start at (0,0) coordinate. Image transfer works fine.
Dual-monitor only has problems with receiving mouse cursor position and clicks, but this should be fixed outside of framebuffer plugin.
Compiles with gcc-4.9.4 and gcc-5.4.0.
Video of testing: https://www.youtube.com/watch?v=muGqjLWLrZA
| Lint Skipped |
| Unit Tests Skipped |
Very general comment: in the desktop file, does the name now overlap with the existing (non-functional?) x11 plugin?
| framebuffers/xcb/xcb_framebuffer.cpp | ||
|---|---|---|
| 44 | Perhaps a bool, since it only seems to be set to 0 or 1 | |
| 290 | Please add { } even to single-statement if()s, to be consistent with e.g. line 316 in XCBFrameBuffer::depth() | |
| 386 | Why not use qDebug()s here? | |
I just copied .desktop and .json files from framebuffers/x11 directory, only changed "x11" to "xcb" where needed, didn't touch name/description/comment fields. Since x11 plugin will be excluded from build, it should not overlap? Also, maybe I should delete whole framebuffers/x11 subdirectory too...
Why not use qDebug()s here?
I forgot, probably because I didn't care, I used these blocks with #ifdef _DEBUG only for local testing, cmake does not automatically define _DEBUG even in Debug build, unless you manually cpecify -D_DEBUG in CMAKE_C_FLAGS_DEBUG / CMAKE_CXX_FLAGS_DEBUG, so this should never compile into binary. But yes, will fix.
Thanks for review!
Fixed @adridg 's comments. Also synchronized .desktop and .json files with master branch
Looks very good, a small issue valgrind found though, it seems that on destruction time the things are not destroyed "in the right order"? https://paste.kde.org/prnowaody
Can you have a look to see if you can fix it?
Fix stupid use-after-free mistake, Shame on me. Move the deletion of d->x11EvtFilter later, after the last usage.
( this fix is also on https://github.com/minlexx/krfb/commit/882436913bad13ad249d6d660d195d8a4978ed22 )
Please commit :)
There's a small problem, but that already existed before so no reason to block on it, if you have time maybe you can give it go in a separate patch.
There's no way for users to choose the plugin they use in the UI. I think it may make sense to add the option somewhere mentioning xcb is "the preferred most performant" but that Qt is a backup they can use if for some reason the xcb one does not work.
What do you think?
No developer account... sorry, can you?
There's no way for users to choose the plugin they use in the UI. I think it may make sense to add the option somewhere mentioning xcb is "the preferred most performant" but that Qt is a backup they can use if for some reason the xcb one does not work.
What do you think?
I totally agree. There is also one problem: users who may have installed old version of krfb, and old "x11" plugin selected and saved in config. They will have to manually edit config file, without GUI option in settings.. so yes, I think it should be done.
Please get one, you're virtually the krfb maintainer now :D
https://community.kde.org/Infrastructure/Get_a_Developer_Account
There's no way for users to choose the plugin they use in the UI. I think it may make sense to add the option somewhere mentioning xcb is "the preferred most performant" but that Qt is a backup they can use if for some reason the xcb one does not work.
What do you think?
I totally agree. There is also one problem: users who may have installed old version of krfb, and old "x11" plugin selected and saved in config. They will have to manually edit config file, without GUI option in settings.. so yes, I think it should be done.