-
Notifications
You must be signed in to change notification settings - Fork 111
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
use damage to track any lost screen changes #186
Conversation
First, I think some further documentation as to how xorgxrdp works is in order here, as I'll concede I also don't fully understand everything here. What is I will do some testing this week to double-check that this doesn't interfere with the screen resizing patch. However, based on my knowledge of how this works, I don't think this will be a problem. The fix I created is strictly related to direct interactions with the memory buffer. If, in the memory buffer's case, a bad update from damage detection comes in during a resize, well, that's the same issue as the rdpCapture timer firing with a bad update, and exactly what my fix is designed to prevent in the first place. In fact, it may actually improve resizing. Sometimes there are graphical glitches on resize that "go away" once you start manipulating the screen. The biggest glitch often has to do with the desktop's background looking weird, though that's more of a problem with Mate versus Gnome 3, and I didn't consider it a big of enough deal to investigate. |
My reply was a bit fuzzy on action items, here's what I would expect to close this out:
1.5: Modify the PR to link to this issue as a
I should be able to get that testing done by the end of this week at the absolute latest, and I think the documentation piece should be straightforward, so we're very close to wrapping this up. |
Tested this with https://github.com/Nexarian/xorgxrdp/tree/resize_damage_enhancement -- It works. I wonder if it will impact performance, however. Still, better to fix the bug. |
Let's wait on this one till we're done with resize. This one is small and easy to rebase. |
@jsorg71 The thing that's now blocking resize is that we're not sure how to fix the integration with NeutrinoRDP. I've looked at it, and it doesn't look easy. I'm not sure if FreeRDP 1.0.1 on which NeutrinoRDP is based supported the dynamic monitor channel. If that's the case, we need to update to FreeRDP 2.0 to make it work or have some way to disable the resize functionality elegantly with NeutrinoRDP. |
@@ -723,7 +765,8 @@ rdpScreenInit(ScreenPtr pScreen, int argc, char **argv) | |||
|
|||
RegisterBlockAndWakeupHandlers(rdpBlockHandler1, rdpWakeupHandler1, pScreen); | |||
|
|||
g_timer = TimerSet(g_timer, 0, 10, rdpDeferredRandR, pScreen); | |||
g_randr_timer = TimerSet(g_randr_timer, 0, 10, rdpDeferredRandR, pScreen); | |||
g_damage_timer = TimerSet(g_damage_timer, 0, 10, rdpDeferredDamage, pScreen); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why the 10ms timer here? Is this repeating or just a hack around something not being initialized yet, which could break if 10ms isn't long enough. Maybe add a comment?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think they are initialized just after the current call. The 10 can be anything bigger then 0. If I remember right, 0 does nothing. Let me see if there is another way to register these things at the right point.
Fixes neutrinolabs/xrdp#1964 |
@jsorg71 I see no reason to not merge this in anymore. I think I understand what's going on. Normally all of the calls to the Xorg backend are wrapped by the xorgxrdp driver. However, in spite of all the drawing primitive APIs being wrapped for our own customized damage detection, some changes are not being reported through that mechanism. This uses the X damage routine which lets X tell us when things have changed, and sometimes we need that in spite of our custom wrapping. That's annoying, but it doesn't sound like a performance hit. If you still think we need this, I'm good to go with it. @matt335672 @metalefty |
Yes, go ahead. |
fix for #171