Implement a updateXTime compatible with Xcb and XWayland
AbandonedPublic

Authored by meven on Apr 8 2020, 8:52 AM.

Details

Summary

updateXTime now forces a property change and we use Application::updateX11Time to then catch the time of the corresponding response event.

Test Plan

Works in a X session (Window resize and video works).
Works with XWayland windows.

Diff Detail

Repository
R108 KWin
Branch
arcpatch-D28674
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 25231
Build 25249: arc lint + arc unit
meven created this revision.Apr 8 2020, 8:52 AM
Restricted Application added a project: KWin. · View Herald TranscriptApr 8 2020, 8:52 AM
Restricted Application added a subscriber: kwin. · View Herald Transcript
meven requested review of this revision.Apr 8 2020, 8:52 AM
meven updated this revision to Diff 79646.Apr 8 2020, 2:32 PM

Move SyncFilter to Platform

meven edited the test plan for this revision. (Show Details)Apr 8 2020, 2:32 PM
meven retitled this revision from WIP: Implement a updateXTime compatible with Xcb and XWayland to Implement a updateXTime compatible with Xcb and XWayland.
zzag requested changes to this revision.Apr 14 2020, 10:38 AM
zzag added inline comments.
CMakeLists.txt
513

Rename the class to SyncAlarmNotifyX11Filter or something.

main.cpp
376–378

It would be nice to have a comment that explains what is going on here.

platform.cpp
54–60

Move/resize operations is something that Workspace needs to be concerned about. Please create the filter in Workspace::initWithX11().

508–515

While you're on this, could you please re-format this comment?

/**
 * Heart touching doxygen comment.
 */
void Platform::updateXTime()
{
}
This revision now requires changes to proceed.Apr 14 2020, 10:38 AM
meven updated this revision to Diff 80095.Apr 14 2020, 1:11 PM
meven marked 4 inline comments as done.

Rename SyncFilter to SyncAlarmNotifyX11Filter, move its instance to Workspace, improve comments

zzag requested changes to this revision.Apr 14 2020, 2:45 PM
zzag added inline comments.
CMakeLists.txt
513

Please keep filenames in alphabetical order. Tip: open vim, select relevant lines in the visual mode, and run the sort command.

platform.cpp
39

This include must come after #include "abstract_output.h"

platform.h
44

Unrelated change, please remove it.

workspace.cpp
453–455

This if must be in initWithX11.

workspace.h
674

I know that QScopedPointer sucks, but I suggest to use it anyway for the consistency sake.

This revision now requires changes to proceed.Apr 14 2020, 2:45 PM
meven updated this revision to Diff 80129.Apr 14 2020, 3:13 PM
meven marked 4 inline comments as done.

Sort includes, sort CMakeLists, use QScopedPointer

meven updated this revision to Diff 80130.Apr 14 2020, 3:14 PM
meven marked an inline comment as done.

Remove blank line

meven added inline comments.Apr 14 2020, 3:14 PM
CMakeLists.txt
513

I am using qtcreator btw.

workspace.h
674

Did not know this about QScopedPointer
std::unique_ptr is used in this file std::unique_ptr<Xcb::Tree> m_xStackingQueryTree;

zzag added inline comments.Apr 14 2020, 4:45 PM
workspace.cpp
451–457

You don't need a connect and m_syncFilter is shadowed.

if (Xcb::Extensions::self()->isSyncAvailable()) {
    m_syncFilter.reset(new SyncAlarmNotifyX11Filter);
}
meven updated this revision to Diff 80183.Apr 15 2020, 10:16 AM
meven marked an inline comment as done.

use QScopedPointer::reset to instanciate SyncAlarmNotifyX11Filter

While this works ok (resizing is smoother for X windows than before), there is more flickering with XWayland than with X.
I tried a few things but I am not sure where or why we drop frames, I am not familiar with X synchronization.
Any advice ?

zzag added a comment.Apr 16 2020, 7:17 AM

You see flickering because we update the window pixmap in X11Client::handleSync(). I need some time to think about how to fix it.

meven added a comment.Apr 16 2020, 7:33 AM
In D28674#649416, @zzag wrote:

You see flickering because we update the window pixmap in X11Client::handleSync(). I need some time to think about how to fix it.

Also currently my new implementation of updateXTime() has become asynchronous : it does not set immediately to the current timestamp, but will set it forcefully soon rather, when Application::updateX11Time gets called again.
Thanks for the pointer.

resizing is smoother for X windows than before

@zzag is it worth merging this now and building upon it?

zzag added a comment.EditedApr 16 2020, 8:32 AM
In D28674#649416, @zzag wrote:

You see flickering because we update the window pixmap in X11Client::handleSync(). I need some time to think about how to fix it.

Also currently my new implementation of updateXTime() has become asynchronous : it does not set immediately to the current timestamp, but will set it forcefully soon rather, when Application::updateX11Time gets called again.

Yeah... On Wayland, we cannot update kwin's timestamp as Qt's XCB plugin does, bad things may happen. On the other hand, we might face some problems if updateXTime() is updated asynchronously on X11, e.g. kwin refuses to activate some windows. We need to be very careful with such things.

zzag added a comment.Apr 16 2020, 8:33 AM

resizing is smoother for X windows than before

@zzag is it worth merging this now and building upon it?

I don't think so. Merging this patch as is now may result in numerous regressions.

meven added inline comments.Apr 16 2020, 1:36 PM
x11client.cpp
2912

Is it necessary to call this here ?
Added in https://phabricator.kde.org/D26914
I have tried removing it and in X it seems unoticeable and in Wayland gets rid of the flickering and my XWayland window matches Xorg behavior.

zzag added inline comments.Apr 24 2020, 1:06 PM
x11client.cpp
2912

Yes, because user can resize a window more than once during a vblank interval and it's not guaranteed that there won't be any pending sync requests before the next compositing cycle starts.

zzag requested changes to this revision.EditedApr 24 2020, 2:17 PM

After some thinking, it's probably better not to merge this patch because updating current X11 time stamp asynchronously may break a lot of things on X11. On the other hand, it's highly undesired to make roundtrips to Xwayland.

I suggest to split this task into two sub-tasks:
(a) Implement updateXTime() on Wayland. After digging through Xwayland's internals, it appears like we could use the system monotonic time as Xwayland's current X11 time stamp. For what it's worth, Mutter does the same thing.
(b) Enable synchronized resizing for Xwayland clients. This will be a more subtle problem since Xwayland attaches a null buffer after the frame window has been resized. I don't know how to fix it yet.

I also would like to apologize for spotting issues with the patch too late in the code review process.

This revision now requires changes to proceed.Apr 24 2020, 2:17 PM
meven abandoned this revision.May 1 2020, 1:31 PM

Replaced by D29250