Gfx mainline merge work #2891
Gfx mainline merge work #2891
Conversation
xrdp/xrdp_encoder.c
Outdated
| self->mm = mm; | ||
|
|
||
| if (client_info->jpeg_codec_id != 0) | ||
| { | ||
| LOG_DEVEL(LOG_LEVEL_INFO, "xrdp_encoder_create: starting jpeg codec session"); | ||
| LOG_DEVEL(LOG_LEVEL_INFO, "xrdp_encoder_create: " |
There was a problem hiding this comment.
General thought: would it be useful to make these LOG() rather than LOG_DEVEL()? I'm thinking ahead to triaging user problem reports regarding which codec is being loaded.
There was a problem hiding this comment.
Yeah this would be good to change.
| (self->wm->client_info->rfx_codec_id != 0))) | ||
| { | ||
| self->encoder = xrdp_encoder_create(self); | ||
| } | ||
|
|
There was a problem hiding this comment.
Do you need these tests before this call here? It looks like they're already in xrdp_encoder_create()
There was a problem hiding this comment.
It looks like the only check that doesn't overlap here is the self->wm->client_info->gfx == 0 check, which I'm not sure is actually needed... Or rather, I'm not entirely certain what it does.
I don't think we've thought much about how to modularize it or make it better. This code is largely a lift-and-shift from Jay's original prototypes that go back to 2017/2018. So if you have suggestions they'd be welcome. I think now is the time to think about how to re-flow the code. The good news with this code is it has been pretty thoroughly tested as people have been using variations of this in |
Nexarian
force-pushed
the
gfx_mainline_merge_work
branch
from
5814809 to
aafb6d3
Compare
|
To respond further to why GFX is spread out throughout the code, there are several main things this change targets:
After reading through the code I feel it's less "random" and more "invasive." GFX changes a lot of things from how capabilities are handled to how dirty regions are detected to how encoding works to how resizing works. It just touches a lot of stuff. There probably is room to simplify it. The FreeRDP codebase did a great job of this, but it's clear they've put a lot of effort into refactoring and code cleanup beyond what's probably worthwhile to do here. Resizing and xrdp_mm would be targets for simplification, if any. |
|
@Nexarian - Thanks for taking another look at it. I think we can look at re-simplifying later on. I'll happily pick up the xrdp_mm stuff. |
Nexarian
force-pushed
the
gfx_mainline_merge_work
branch
from
353d76f to
d260c26
Compare
|
Hi @Nexarian I'm running your current merges as of 8th Jan. Configuring with I'm using an xfreerdp client to control modes:- I'm not having any session connection problems with a first connection, but I'm seeing the green banding you've mentioned elsewhere. When I log out, I'm getting memory access errors. I've run with valgrind, and I'm attaching logs for xrdp and valgrind for the connection. Before logging out there are no reported memory access violations, which is good. |
|
FWIW, the valgrind issues are caused by the main xrdp process exiting while the encoder thread is still active. This patch stops the valgrind errors:- --- a/xrdp/xrdp_process.c
+++ b/xrdp/xrdp_process.c
@@ -306,6 +306,8 @@ xrdp_process_main_loop(struct xrdp_process *self)
}
/* Run end in module */
xrdp_process_mod_end(self);
+ xrdp_wm_delete(self->wm);
+ self->wm = NULL;
libxrdp_exit(self->session);
self->session = 0;
self->status = -1; |
|
Thanks, I incorporated your PR! I've also noticed that resize-on-the-fly appears to be broken, so I'll dig into that tonight. |
|
As you can see from the commit, fixed it. I missed when lifting-and-shifting the code from |
|
I'll test it. |
|
@Nexarian Does this PS includes 20 commits included in |
|
@metalefty I'm confused. There are only 7 commits beyond devel? |
|
Ah, sorry. My head was messed up due to a lack of sleep. Just ignore it. BTW, did you talk something about multimon with Jay? |
|
Jay's multimon work is here: What I can do is work on integrating these into this branch. The only bug I'm aware of in the |
|
Then, could you go on multimon work? I will look into the graphical corruption. |
|
The corruption is going to be a tough one to find. I spent a couple of hours playing with it on Monday and got nowhere - it's certainly not related to out-of-bounds memory accesses. It might be necessary to put a test program together which illustrates the fault and look at the resulting PDUs. Anyone got any other ideas? |
|
The other idea I have is to draw the damage rects on the screen before
encoding (maybe in a red rectangle). That way we can see if it's an
encoding issue or simply a problem of sending the wrong buffer. This need
not be out of bounds but rather might just be sending the wrong part of the
screen at the wrong time.
What is telling is the corruption nearly always has "radioactive green"
which from experience I've learned is the xorg way to say "unallocated
memory" meaning it's sending data it shouldn't be.
…On Thu, Jan 11, 2024, 6:27 AM matt335672 ***@***.***> wrote:
The corruption is going to be a tough one to find. I spent a couple of
hours playing with it on Monday and got nowhere - it's certainly not
related to out-of-bounds memory accesses. It might be necessary to put a
test program together which illustrates the fault and look at the resulting
PDUs. Anyone got any other ideas?
—
Reply to this email directly, view it on GitHub
<#2891 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AA4UBHIZBEUOI2NC75ZBVX3YN7EAPAVCNFSM6AAAAABBFFQQ2OVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQOBWHEZTQMJRGQ>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
|
The GFX decoder does not like rects in the list that contain tiles that didn't change. I older RemoteFX decoder was ok with that. I don't see my patch for rdpCapture2 that fixes that in gfx_mainline_merge_work. Also, the frame ack is used to 'lock' the shared memory. I think some of the dynamic monitor changes violate this assumption. For debugging, we can change xorgxrdp to always allocate a new shared memory so they can't both be using it at the same time. |
|
This patch?
jsorg71/xorgxrdp@3d05ddd
I'll add it tonight and try it out!
…On Thu, Jan 11, 2024 at 6:17 PM jsorg71 ***@***.***> wrote:
The GFX decoder does not like rects in the list that contain tiles that
didn't change. I older RemoteFX decoder was ok with that. I don't see my
patch for rdpCapture2 that fixes that in gfx_mainline_merge_work. Also, the
frame ack is used to 'lock' the shared memory. I think some of the dynamic
monitor changes violate this assumption. For debugging, we can change
xorgxrdp to always allocate a new shared memory so they can't both be using
it at the same time.
—
Reply to this email directly, view it on GitHub
<#2891 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AA4UBHMJQLDR5HXVS3TMLRLYOBXHZAVCNFSM6AAAAABBFFQQ2OVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQOBYGEYTQMJSG4>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
|
No I think it's this one. |
|
That didn't seem to help unfortunately :/ |
|
I also tried changing the rdpCapture2 to the "wyhash" variation that I have in mainline_merge_shm, no luck either. |
|
This may be subjective, but after merging @Nexarian's last two commits, I'm finding it harder to generate green areas on the display. That's good in a way, but I've yet to come up with some way of reproducing the problem effectively which allows for testing. I've knocked up some X11 drawing programs, but these all seem to be working perfectly. How are the rest of you reproducing the screen corruption? |
|
Here's what I do:
It will eventually create spots within the terminal window that, if you only move the window slightly, will not disappear. This is, presumably, because of the CRC check algorithm that doesn't update tiles that aren't changing. However, on screen they're corrupt so it's very interesting. If the edges of the terminal go over those tiles, they will be corrected. Not a very thorough set of instructions to reproduce, but it works consistently for me. |
|
With the latest gfx_mainline_merge_work commits 06e8303 and neutrinolabs/xorgxrdp@8519407 I'm still getting the occasional green banding. A couple of observations:-
--- a/xup/xup.c
+++ b/xup/xup.c
@@ -1275,7 +1275,7 @@ process_server_set_pointer_shmfd(struct mod *amod, struct stream *s)
{
Bpp = (bpp == 0) ? 3 : (bpp + 7) / 8;
shmembytes = width * height * Bpp + width * height / 8;
- if (g_file_map(fd, 1, 1, shmembytes, &shmemptr) == 0)
+ if (g_file_map(fd, 1, 0, shmembytes, &shmemptr) == 0)
{
cur_data = (char *)shmemptr;
cur_mask = cur_data + width * height * Bpp;
@@ -1368,7 +1368,7 @@ process_server_paint_rect_shmfd(struct mod *amod, struct stream *s)
{
if (num_fds == 1)
{
- if (g_file_map(fd, 1, 1, shmem_bytes, &shmem_ptr) == 0)
+ if (g_file_map(fd, 1, 0, shmem_bytes, &shmem_ptr) == 0)
{
bmpdata = (char *)shmem_ptr;
bmpdata += shmem_offset;
--- a/xup/xup.c
+++ b/xup/xup.c
@@ -1715,6 +1715,7 @@ lib_mod_process_message(struct mod *mod, struct stream *s)
{
LOG_DEVEL(LOG_LEVEL_INFO,
"lib_mod_process_message: type 3 len %d", len);
+ g_sleep(100);
for (index = 0; index < num_orders; index++)
{
phold = s->p;How is the syncing supposed to work? If a |
|
@matt335672 Here is what I did on higher latency connection to get rid of xorgxrdp overwriting the shared memory before the encoder is done. |
mstsc.exe indicates it supports GFX in the early capability flags, even if it not able to support 32 BPP. This results in a session failure if a RDPGFX_CAPS_CONFIRM_PDU is sent on the EGFX virtual channel.
Since v0.9.9, xrdp has assumed that the "drdynvc" static virtual channel is available for its exclusive use. With GFX support, it is necessary to codify this to prevent this sequence of operations:- - NeutrinoRDP target sends DVC Capabilities Request PDU - target responds wih DVC Capabilities Response PDU - xrdp processes this, starting the GFX virtual channel again In the future, if NeutrinoRDP requires access to virtual channels, data may somehow need to be passed through to the target while being parsed and handled appropriately within xrdp.
* Store EGFX state before entering resize state machine At present the EGFX state is destroyed by states WMRZ_EGFX_DELETE_SURFACE through WRMZ_EGFX_DELETE. This means that at WMRZ_EGFX_INITIALIZE we cannot distinguish between EGFX not being ever used, and EGFX having been torn down. Consequently, when running non-GFX, we don't correctly recover the session. * Allow multiple reasons for suppress_output Replaces the single boolean for suppress_output with a bitmask, to allow output to be suppressed for more than one reason * Disable output during resize * Add states to dynamic resize Adds states to the dynamic resize state machine so we wait for a Deactivation-Reactivation sequence to finish before sending pointer updates, etc. * suppress module output during the dynamic resize * Add support for dynamic resize to VNC backend xrdp_mm needs to be informed when a resize has been performed so that the resize stte machine can be updsate.
Nexarian
force-pushed
the
gfx_mainline_merge_work
branch
from
9753559 to
54acca4
Compare
|
I'm testing GFX multimon at the following versions but I got "protocol error" on mstsc. Is something missed to cherry-pick? |
|
I haven't merged Jay's changes as I haven't had a chance to test it. I suppose now is the time. I'll just send it, I'm realistically not going to have much time in the next week. |
* GFX: work on multimon * fix for non GFX multimon
|
I merged in Jay's work and did a quick test on my M1 MacBook with 3x 4k monitors, it works! 👍 |
|
Hmm, now dynamic resizing for a single monitor appears to be broken :/. We can investigate. |
|
I can look at the resizing early next week. |
|
Stack trace. Seems the mechanism to use multi monitor doesn't like when something changes via the virtual channel. |
|
I've had a quick look at the single monitor resizing. What seems to be happening is when the encoder and the EGFX surface have been removed, xorgxrdp is sending an orders command even though output is suppressed. That's causing a SEGV due to the missing encoder. I'll look into it in more detail tomorrow - not sure where the best place to fix this is. |
|
To add more detail to the stack trace:- |
|
Oh, sorry guys, I didn't test resize much. I'll check why suppress is not working. |
|
I can see what's happening on the resize. The encoder is now responsible for GFX surface management - it is instructed what to do by xorgxrdp. The resize logic isn't expecting this. The first thing is does is to stop the encoder and manually delete surfaces. When we send the mod_server_version message, xorgxrdp sends the GFX monitor layout back. We then try to pass this to the encoder which isn't there and get the SEGV. I think the solution is going to be to not stop the encoder as part of the GFX resize. We'll then end up with the version message being processed normally. All the surface management for the resize can be driven from xorgxrdp. We'll probably need to solve the issue with the MacOS client as well. I've managed to get a VM running Monterey to check this out. I'm out of time today to try things today, but I've got all day Monday to spend on this. |
This might be hard. In order to change the size of the encoders, you have to delete and re-create them. This is true from what I understand for RFX, but also for the upcoming X264, NVENC, and YAMI encoders (the width and height of all of them are read-only properties set as an argument to the constructor). However, we might be able to get away with doing it in one step of the state machine, or eliminate the state machine entirely, which leads to...
As I wrote here, the entire reason the state machine exists is because of the fact that I couldn't figure out how to not shut down the GFX channel and re-create it in order to have a stable resize with the Mac OS client. If that requirement goes away, then we can collapse a lot of this into a single atomic step that doesn't require a lot of "waiting until something is done" including re-creating the encoder at a different size. |
|
Another potential way to solve this that is probably too much work, is to queue up any surface commands from xorgxrdp until the state machine finishes what it needs to, then replay those commands back to the client after the encoder and the channel are back... |
|
rdpSendGfxMonitors does not look at suppress. Maybe it should. Anyway, just comment out the call to rdpSendGfxMonitors seems to fix it. If surface 0 is first monitor and 1 is second one etc we should be ok. |
|
That almost fixes this. But the following test case is still broken with that command commented:
|
|
This change 75b41af has the same net effect, but it simply blocks a crash if the encoder isn't there. It doesn't fix the bug I outlined earlier. |
|
I've had a look at the latest xorgxrdp merges, and I'm getting an idea of what's going on. I can see that for GFX and H.264 we're sending the GFX graphics messages directly from The bit I'm confused about is why we need to create and map the surfaces from within xorgxrdp. It's going to be harder to support [MS-RDPEGFX] 3.3.5.19 behaviour. This is probably going to have to be done in xrdp_mm.c, so we'll end up duplicating code between these two locations. Can anyone help me with why this approach has been taken? |
|
Yea, I was talking with Nex. Xorgxrdp odes not need to reset. As long as the surface ID can be predicable for each monitor. I'll do a PR to remove that. |
|
Let's merge into devel. |
No description provided.