xorgxrdp race to paint window

[ejolson2005]

It would appear that

d9850d1

may make worse a race condition related to waiting for more updates on slower computers. What I'm seeing is menu items randomly appearing blank in the fvwm mouse menus and occasional lines missing from an xterm (for example when top is running). My understanding is that

• The application paints the background.
• xorgxrdp waits 4ms and starts sending the update.
• The application paints the foreground.
• The foreground is missing from the remote session.

This problem is particularly noticeable when running xorgxrdp-0.2.14 along with xrdp-0.9.14 on a Rock64 single board computer under AARCH64 Ubuntu Linux with the fvwm window manager. More than 1/2 the time the mouse menus are missing the text. Similarly, xterm updates while running top exhibit blank lines about 1/10 of the time.

As this appears to be a timing issue, the exact speed of this single-board computer coupled with the way fvwm writes the mouse menus and xterm updates it's window while running top may be important for tracking down and fixing the bug.

As a reference, the same problem is almost non-existent with xorgxrdp-0.2.13 running on the same system. It is, however, still possible to slide the mouse through an fvwm menu multiple times up and down until the paint repaint sequences lead to an entry in which the background appears but the foreground text does not. Rather than 1/2 the time, this may happen more like 1/20 of the time (from non-scientific test related to how many times I need to wiggle the mouse before the screen update race results in missing text).

While not having so much missing text is worth one extra frame of latency in my opinion, there may be a better solution that actually fixes this. I'll be looking at the patch mentioned above and checking whether, for example, an 8ms delay makes a noticeable improvement. Any help on fixing the root cause, however, would be appreciated.

[ejolson2005]

As a follow up, increasing the wait as

#define MIN_MS_TO_WAIT_FOR_MORE_UPDATES 8

didn't help much. There is frequently one line of text missing in the fvwm mouse menus.

However, with

#define MIN_MS_TO_WAIT_FOR_MORE_UPDATES 30

all the text appears in the mouse menus almost every time. It is still possible to scan though the menu with the mouse multiple times until the highlighting and rewriting leads to a case where the background appears without the foreground text written on it.

Note that

#define MIN_MS_TO_WAIT_FOR_MORE_UPDATES 16

also seems to work.

Athough reducing the latency is nice, my recommendation is to revert the entire patch, as the extra complication of dual timers does not seem useful until the root cause of why the foreground text sometimes goes missing is solved.

[jsorg71]

I don't think it's a bug in d9850d1 but it exposes another bug in the drawing tracking. Can I post a patch for you to test? If ok, I can make a PR.

[ejolson2005]

I'd be happy to test a patch. Thanks for looking into this!

[jsorg71]

A quick and dirty patch here, I can make a cleaner one of ok.

diff --git a/module/rdp.h b/module/rdp.h
index 48b0054..1871a5e 100644
--- a/module/rdp.h
+++ b/module/rdp.h
@@ -329,6 +329,9 @@ struct _rdpRec
     int fd;
     /* egl */
     void *egl;
+
+    DamagePtr damage;
+
 };
 typedef struct _rdpRec rdpRec;
 typedef struct _rdpRec * rdpPtr;
diff --git a/xrdpdev/xrdpdev.c b/xrdpdev/xrdpdev.c
index 2dcce60..b9f2ec1 100644
--- a/xrdpdev/xrdpdev.c
+++ b/xrdpdev/xrdpdev.c
@@ -367,6 +367,26 @@ rdpResizeSession(rdpPtr dev, int width, int height)
     return ok;
 }
 
+/*****************************************************************************/
+static void
+xorgxrdpDamageReport(DamagePtr pDamage, RegionPtr pRegion, void *closure)
+{
+    rdpPtr dev;
+    ScreenPtr pScreen;
+
+    LLOGLN(10, ("xorgxrdpDamageReport:"));
+    pScreen = (ScreenPtr)closure;
+    dev = rdpGetDevFromScreen(pScreen);
+    rdpClientConAddAllReg(dev, pRegion, &(pScreen->root->drawable));
+}
+
+/*****************************************************************************/
+static void
+xorgxrdpDamageDestroy(DamagePtr pDamage, void *closure)
+{
+    LLOGLN(0, ("xorgxrdpDamageDestroy:"));
+}
+
 /******************************************************************************/
 /* returns error */
 static CARD32
@@ -450,7 +470,16 @@ rdpDeferredRandR(OsTimerPtr timer, CARD32 now, pointer arg)
 
     RRScreenSetSizeRange(pScreen, 256, 256, 16 * 1024, 16 * 1024);
     RRTellChanged(pScreen);
-
+#if 1
+    dev->damage = DamageCreate(xorgxrdpDamageReport, xorgxrdpDamageDestroy,
+                               DamageReportRawRegion, TRUE,
+                               pScreen, pScreen);
+    if (dev->damage != NULL)
+    {
+        DamageSetReportAfterOp(dev->damage, TRUE);
+        DamageRegister(&(pScreen->root->drawable), dev->damage);
+    }
+#endif
     return 0;
 }

[ejolson2005]

Success! The patch applied

$ patch -p1 <../xorgxrdp.patch 
patching file module/rdp.h
Hunk #1 succeeded at 325 (offset -4 lines).
patching file xrdpdev/xrdpdev.c

with a slight offset on one hunk. Did you make it against version 0.2.14 or current development?

At any rate. I set the update wait back to

#define MIN_MS_TO_WAIT_FOR_MORE_UPDATES 4

recompiled and installed. So far it's looking great. I haven't seen even one missing line of text!

[ejolson2005]

Did the patch ever get cleaned up? I haven't seen any changes to the development repository. Did the new patch go someplace else for testing?

[ejolson2005]

Is there any news about this patch? I'd like to close this issue if a patch that fixes things is now in the repository.

[ejolson2005]

I see that a new version is available as a few a few weeks ago. Does that version include something equivalent to the patch above or does one still need to update the patch?

[jsorg71]

Sorry, I didn't get to this yet. I didn't forget about it. I want to enable Damage extension if it exists and use to find where we're missing a screen change. The calls should all be wrapped so it's a bug that it's needed. Short term it might be just easier to enable Damage all the time and when I can get time to debug the drawing issue, then disable it.

[Nexarian]

For those who might be interested in trying their hand at implementing a fix, can you give us pointers on where we might start to look at it?

…

On Mon, Jan 11, 2021 at 7:20 PM jsorg71 ***@***.***> wrote: Sorry, I didn't get to this yet. I didn't forget about it. I want to enable Damage extension if it exists and use to find where we're missing a screen change. The calls should all be wrapped so it's a bug that it's needed. Short term it might be just easier to enable Damage all the time and when I can get time to debug the drawing issue, then disable it. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#171 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AA4UBHO2PHWSNAMYW63S5U3SZOITJANCNFSM4RL6OPFA> .

[jsorg71]

I made PR #186 for this

[ejolson2005]

I just installed xorgxrdp-0.2.18 (along with xrdp-0.9.19) on a Raspberry Pi Zero.

I am again having the same race condition. Random lines displayed by xterm are blank (about 1 our of 10 refreshes while running top) and the mouse menus in fvwm occasionally have blank entries (about 1 in 5 times for certain menus). It appears to be the same race as I reported before.

Was the code reverted or are there timing differences on the Zero (slower than most anything people use for remote desktop) that illustrate the previous patch was insufficient?

[matt335672]

Hi @ejolson2005

We never merged the change - there was some debate as to whether it was needed or not. I'll ask around.

[ejolson2005]

Hi @ejolson2005

We never merged the change - there was some debate as to whether it was needed or not. I'll ask around.

Thanks for the quick reply. Please let me know if you need any testing done or details about my setup.

[matt335672]

Merge is done - sorry I dropped it.