Multimonitor functionality #219

gilbsfr24 · 2018-01-03T10:48:35Z

No description provided.

uglym8

Thank you for your contribution.
Looks good but needs small changes.

I'll check the [MS-RDPBCGR] related changes shortly.

uglym8 · 2018-01-03T13:21:50Z

xwin.c

+		XRRCrtcInfo *xrrci;
+		int i;
+		g_num_monitors = xrrr->ncrtc;
+		 printf("nb crt %d\n",g_num_monitors);


Remove call to printf(), please, or consider to use logger() instead.

uglym8 · 2018-01-03T13:23:13Z

xwin.c

+
+#ifdef HAVE_XRANDR
+		// MultiMonitors : get geometry from xrand If we have
+		XRRScreenResources *xrrr = XRRGetScreenResources(g_display, DefaultRootWindow(g_display));


What do you think about making this separate function as you also use the same code in xwin.c +1987?

uglym8 · 2018-01-03T13:25:16Z

secure.c

@@ -59,6 +59,8 @@ uint16 g_server_rdp_version = 0;
 static int g_sec_encrypt_use_count = 0;
 static int g_sec_decrypt_use_count = 0;

+int g_num_monitors = 0;
+rdp_monitors g_monitors[16];


I can't find any comparison to the size of this array anywhere in the code.
Yes, I'm sure that it's unlikely to have more than 16 monitors plugged in simultaneously
but anyway:)

Yes, I will make change asap

uglym8 · 2018-01-03T14:18:47Z

Basically it conforms to [MS-RDPBCGR].

I guess for it to be perfect it should contain two small additions:

According to https://msdn.microsoft.com/en-us/library/dd305336.aspx we should make sure that server does support Extended Client Data Blocks before sending out TS_UD_CS_MONITOR
We should check for width and height constraints (see next paragraph).

Funny enough but I don't know what to do in the case when our total virtual size is greater than allowed maximum or less than allowed minimum. Maybe ATM we could show warning and later check with Microsoft what have to be done in this case?

- g_monitors dynamic array - separate function to get monitors - logger : no printf

… multimon

gilbsfr24 · 2018-01-05T10:51:51Z

Bug detected on some computer. I have a Xlib: extension "RANDR" missing on display ":0.0".
then a segment fault because of the NULL result of XRRGetScreenResources.
Segment fault solved but origin is being investigate ...

astrand · 2018-01-09T10:13:53Z

See also Issue #205.

gilbsfr24 · 2018-01-09T11:39:28Z

for hp thin client T5545 randr is disabled, /etc/X11/xorg.conf contains
Option "xinerama" "on"
Option "RandR" "false"
so it's fall in 1 monitor mode

uglym8 · 2018-01-09T12:00:43Z

Yes, we should definitely differentiate between case when XRandR package is available (compile time checks) and case when XRandR is enabled for the current X Windows server instance (runtime checks).

Basically, I'm thinking about XRRQueryExtension(3).

Regarding changes for this PR: I'll comment a little bit later.

hean01-cendio · 2018-01-09T14:56:46Z

Nice work, related issue is #205 just for back reference.

I have not tested this PR yet, but I have one concern; Does toggling between full screen and window mode work as expected ?

Additional to this, it would be nice if you one could decide if one want to use all monitor or only current for fullscreen, see issue #205 for use case.

gilbsfr24 · 2018-01-09T16:04:40Z

Yes , tests I have made toggling between full screen and window mode work as expected, resizing too...
For the option to choose 1 or more monitors, I do not know enough about the main rdesktop window to do it

gilbsfr24

done

gilbsfr24 · 2018-01-09T16:16:22Z

xwin.c

+
+#ifdef HAVE_XRANDR
+		// MultiMonitors : get geometry from xrand If we have
+		XRRScreenResources *xrrr = XRRGetScreenResources(g_display, DefaultRootWindow(g_display));


uglym8

Nice work.
Thank you.

Need more small changes though.
I suggest that you don't change irrelevant parts in this PR. It makes reviewing harder.
If you feel that it should be fixed - make another PR.

TIA.

uglym8 · 2018-01-12T11:36:21Z

iso.c

@@ -26,13 +26,14 @@ extern RD_BOOL g_encryption_initial;
 extern RDP_VERSION g_rdp_version;
 extern RD_BOOL g_use_password_as_pin;

-static RD_BOOL g_negotiate_rdp_protocol = True;
-


I suggest that you don't change irrelevant parts in this PR.
If you feel that it should be fixed - make another PR.

if g_rdp_version >= RDP_V5, we have to negotiate protocol to have the extended_data reply , so variable not needed.

uglym8 · 2018-01-12T11:37:01Z

iso.c

@@ -62,7 +63,7 @@ iso_send_connection_request(char *username, uint32 neg_proto)
 	STREAM s;
 	int length = 30 + strlen(username);

-	if (g_rdp_version >= RDP_V5 && g_negotiate_rdp_protocol)
+	if (g_rdp_version >= RDP_V5 )


I think that you should not change this line in this PR.

if g_rdp_version >= RDP_V5, we still have to have to negotiate protocol to have the extended_data reply ,

uglym8 · 2018-01-12T11:37:14Z

iso.c

@@ -83,7 +84,7 @@ iso_send_connection_request(char *username, uint32 neg_proto)
 	out_uint8(s, 0x0d);	/* cookie termination string: CR+LF */
 	out_uint8(s, 0x0a);

-	if (g_rdp_version >= RDP_V5 && g_negotiate_rdp_protocol)
+	if (g_rdp_version >= RDP_V5)


I think that you should not change this line in this PR.

if g_rdp_version >= RDP_V5, we still have to have to negotiate protocol to have the extended_data reply ,

uglym8 · 2018-01-12T11:37:21Z

iso.c

-	g_negotiate_rdp_protocol = True;
-
-	neg_proto = PROTOCOL_SSL;
+	neg_proto = *selected_protocol;


I think that you should not change this line in this PR.

g_negotiate_rdp_protocol not needed.
Because your suggestion is good, I do not change the initialization of the variable neg_proto.
But I will open another problem about this in order to solve the problem of the reconnection which tries to renegotiate each time

uglym8 · 2018-01-12T11:37:30Z

iso.c

@@ -302,7 +302,7 @@ iso_connect(char *server, char *username, char *domain, char *password,
 				}

 				logger(Core, Notice, "Retrying with plain RDP.");
-				g_negotiate_rdp_protocol = False;
+				neg_proto = PROTOCOL_RDP;


I think that you should not change this line in this PR.

Retrying with plain RDP so the neg_proto = PROTOCOL_RDP, g_negotiate_rdp_protocol is unnecessary

uglym8 · 2018-01-12T11:40:21Z

iso.c

 extern char *g_sc_csp_name;
 extern char *g_sc_reader_name;
 extern char *g_sc_card_name;
 extern char *g_sc_container_name;

+// MultiMonitors : external declaration - see xwin.c for variables
+extern RD_BOOL g_monitors_supported;


As this variable is only dependable upon EXTENDED_CLIENT_DATA_SUPPORTED flag maybe we should call
it "g_extended_data_supported" instead?

yes, good suggestion

uglym8 · 2018-01-12T11:41:02Z

secure.c

@@ -935,6 +970,7 @@ sec_connect(char *server, char *username, char *domain, char *password, RD_BOOL
 	struct stream mcs_data;

 	/* Start a MCS connect sequence */
+	selected_proto = PROTOCOL_SSL; //prefered protocol


I think that you should not change this line in this PR.

Because your suggestion is good, I do not change the initialization of the variable neg_proto.
But I will open another problem about this in order to solve the problem of the reconnection which tries to renegotiate each time

# Conflicts: # types.h

revert unwanted change of the neg_proto usage

gilbsfr24 · 2018-01-13T14:13:19Z

git fetch upstream
git merge upstream/master
git push origin master

have kill my work..

gilbsfr24 · 2018-01-14T02:18:53Z

I have to test new merge, because of geomertry changes

gilbsfr24

done

gilbsfr24 · 2018-01-13T07:42:38Z

iso.c

-	g_negotiate_rdp_protocol = True;
-
-	neg_proto = PROTOCOL_SSL;
+	neg_proto = *selected_protocol;


g_negotiate_rdp_protocol not needed.
Because your suggestion is good, I do not change the initialization of the variable neg_proto.
But I will open another problem about this in order to solve the problem of the reconnection which tries to renegotiate each time

gilbsfr24 · 2018-01-13T07:44:53Z

secure.c

@@ -935,6 +970,7 @@ sec_connect(char *server, char *username, char *domain, char *password, RD_BOOL
 	struct stream mcs_data;

 	/* Start a MCS connect sequence */
+	selected_proto = PROTOCOL_SSL; //prefered protocol


Because your suggestion is good, I do not change the initialization of the variable neg_proto.
But I will open another problem about this in order to solve the problem of the reconnection which tries to renegotiate each time

uglym8 · 2018-01-14T19:37:51Z

Hi @gilbsfr24!

As you've already found out: next time do not merge master into your branch - rebase you branch upon master instead. You can merge upstream/master into your local master to make yout master and upstream master even.

Check these articles:
https://github.com/edx/edx-platform/wiki/How-to-Rebase-a-Pull-Request
https://www.digitalocean.com/community/tutorials/how-to-rebase-and-update-a-pull-request
https://anavarre.net/how-to-rebase-a-github-pull-request/

I feel that I'm partially responsible for this mess as there's no info about this in https://github.com/rdesktop/rdesktop/wiki/contribution-guide
Sorry about that.

I've picked your changes to the following branch https://github.com/uglym8/rdesktop/tree/gilbsfr24_multimon
Feel free to base your further works upon this branch (the code is yours anyway).
You can just take a patch and apply to your updated master.
Do not forget to change Author/Committer;)

Check that everything works as intended.
I slightly modified your code:

code style consistency
removed changes not belonging (IMHO) to this PR (g_negotiate_rdp_protocol removal etc)
This way it is easier to review and reason about changes.
revert g_monitors to static array as according to MS-RDPBCGR 2.2.1.3.6 the maximum number of monitors is 16.

I'll comment tomorrow about XRandR and a number of currently connected monitors with additional code samples.

hean01-cendio · 2018-01-15T09:32:40Z

@gilbsfr24

Let us know if you need further help to cleanup your branch and fix the commit history, when you are done and the history looks good, you can force push it, to rewrite the history of this PR, using -f flag with git push.

gilbsfr24 · 2018-01-15T20:08:07Z

Thanks for your help. I would do that next time.

If the corrections are right for you. I transfer you my code. I do not need to be an author for your project, so I'm sure you'll make good use of it. I will do additional tests because of corrections made in parallel. I'll keep you informed

- code style consistency - revert g_monitors to static array as according to MS-RDPBCGR 2.2.1.3.6 the maximum number of monitors is 16. BUT I keep the modification of the g_negotiate_rdp_protocol (that is not needed) WE HAVE to negotiate, because the need the response

gilbsfr24 · 2018-01-15T22:17:18Z

The deletion of the ui_init_connection function and its replacement with setup_user_requested_session_size no longer allows the determination of the fullscreen mode.
The xwin_toggle_fullscreen function does not use the new g_window_size_type variable.
I think "Merge pull request # 232 from rdesktop / dynamic_resize_fixes" makes a real regression

I'm still looking for where to connect my function xwin_get_monitors.
Maybe that the team "dynamic_resize_fixes" knows where this connection should be made

uglym8 · 2018-01-17T08:31:36Z

@gilbsfr24, I guess I owe you an apology.
I'm sorry for commenting the changes in negotiation process without actually trying it out myself:(

Yes, sometimes we do need to renegotiate to get flags.
I'm not ready to say whether your approach is 100% correct or not though:)
Need more testing.

Thank you.

hean01-cendio · 2018-01-17T09:13:01Z

@gilbsfr24

The deletion of the ui_init_connection function and its replacement with
setup_user_requested_session_size no longer allows the determination of the fullscreen mode.

I guess you are talking about the population of g_monitors code that you added to ui_init_connection(), that code could probably be broken out into a ui_get_monitors() function and called from the case of Fullscreen in setup_user_requested_session_size(). Are there any details I miss in this conclusion ?

The xwin_toggle_fullscreen function does not use the new g_window_size_type variable.

No, it uses g_fullscreen which reflects the current windows state, you should see g_window_size_type as the user request of session size.

hean01-cendio · 2018-01-17T09:14:09Z

@uglym8 I do not understand why the there i an modification of negotiation process, how is that related to multimon support ?

uglym8 · 2018-01-17T09:55:20Z

I was puzzled too until I encountered the case when you have to renegotiate to get flags.

Example:
We're connecting to the server which yields RDP_NEG_FAILURE (with the SSL_NOT_ALLOWED_BY_SERVER as a reason) (https://github.com/rdesktop/rdesktop/blob/master/iso.c#L272).
We set retry_without_neg = True; there and below (https://github.com/rdesktop/rdesktop/blob/master/iso.c#L305) we set g_negotiate_rdp_protocol = False;.

After that we retry to connect again.

In iso_send_connection_request() (https://github.com/rdesktop/rdesktop/blob/master/iso.c#L86) as
g_negotiate_rdp_protocol is False we don't send optional RDP protocol negotiation request for RDPv5 thus no flags upon successful connection.

gilbsfr24 · 2018-01-18T11:05:56Z

when to want PROTOCOL_SSL or PROTOCOL_HYBRID then you make a protocol negotiation request for >= RDPv5.
I have to do it too to get EXTENDED_CLIENT_DATA_SUPPORTED flag.
So If you dont want to negociate for SSL nor HYBRID
WE have to negociate then basic PROTOCOL_RDP
Hope It helps

gilbsfr24 · 2018-01-18T11:12:04Z

hean01-cendio>No, it uses g_fullscreen which reflects the current windows state

what does mean
switch(g_window_size_type)
{
case Fullscreen:
ui_get_screen_size(&g_requested_session_width, &g_requested_session_height);
break;

uglym8 · 2018-01-24T09:26:05Z

@gilbsfr24, do you need any help to finish this off?
What's the current status?

gilbsfr24 · 2018-01-25T02:14:37Z

yes I need help to set the call of ui_get_monitors according the functionnality setup_user_requested_session_size. But I dont really understand if we are in fullscreen or windowed mode.
Have you solve for the request negotiation protocol ?

hean01-cendio · 2018-01-25T08:24:57Z

@gilbsfr24

yes I need help to set the call of ui_get_monitors according the functionnality
setup_user_requested_session_size. But I dont really understand if we are in fullscreen or windowed > mode.

Ok, as what i understand is that the new ui_get_monitors() queries and populates g_monitors[] array, this could be done once at startup of rdesktop in a proper place in main(). As i understand this is the prerequisite for enabling multiple monitors. This call probably uses g_display and needs to be called after ui_init() and before before entering the main loop in main(). Note, this is before any window is created.

setup_user_requested_session_size() is only used for setting up initial startup size, the variable g_window_size_type only holds startup type, not a state of the current window. When a user toggles between fullscreen and windowed mode the rdesktop window is teared down and recreated, the variable g_fullscreen is used to steer what kind of window to create and also reflects the state.

Have you solve for the request negotiation protocol ?

I haven't looked into the changes for negotiation protocol, do you have a bad feeling about your changes and want someone else to dig into this topic ?

uglym8 · 2018-01-25T08:42:34Z

Yes, @hean01-cendio is right.
When I checked your PR I just added a call to xwin_get_monitors() to ui_init() (at https://github.com/rdesktop/rdesktop/blob/master/xwin.c#L1960).

Have you solve for the request negotiation protocol ?

Not yet. I just quick patched it to get the required flags.

gilbsfr24 · 2018-01-29T23:12:56Z

the call to xwin_get_monitors() is needed only when fullscreen mode is asked
So xwin.c#L1960 can be the good place for the call

toggle window mode to fullscreen has to call xwin_get_monitors
toggle fullscreen to window mode has to set g_num_monitors =1;

tuxflo · 2019-11-27T11:44:13Z

Hi! Is there any progress on this? I build rdesktop with the changes in the branch but I can't figure out how to specify the monitor settings.

add Multimonitor functionnality

890f77a

uglym8 requested changes Jan 3, 2018

View reviewed changes

uglym8 added the tester wanted label Jan 3, 2018

gilbsfr24 added 8 commits January 3, 2018 15:49

MultiMonitors

5e32cd3

- g_monitors dynamic array - separate function to get monitors - logger : no printf

multimon struct RDP_NEG_REQ_FLAGS - EXTENDED_CLIENT_DATA_SUPPORTED

f9ff4fe

change renegotiate to get EXTENDED_CLIENT_DATA_SUPPORTED

b6f3e6b

set monitors only if supported by the server

d6699d1

set monitors only if supported by the server

d3c16ef

Merge remote-tracking branch 'upstream/master' into multimon

d4228e9

presentation

75038fd

Merge branch 'multimon' of https://github.com/gilbsfr24/rdesktop into…

f528384

… multimon

astrand changed the title ~~Multimonitors functionnality~~ Multimonitor functionality Jan 9, 2018

test result of XRRGetScreenResources (if xrandr not active)

3f3a3ec

gilbsfr24 added 2 commits January 9, 2018 13:36

Add Xinerama if randr not avaible

1f9ffaa

add XRRQueryExtension test

4f372b7

gilbsfr24 commented Jan 9, 2018

View reviewed changes

uglym8 requested changes Jan 12, 2018

View reviewed changes

gilbsfr24 added 3 commits January 13, 2018 14:11

Merge remote-tracking branch 'upstream/master' into multimon

5e99b26

# Conflicts: # types.h

change g_monitors_supported to g_extended_data_supported

c0565e1

revert unwanted change of the neg_proto usage

???

3abfe79

gilbsfr24 closed this Jan 13, 2018

???

2a12880

gilbsfr24 added 4 commits January 13, 2018 23:05

???

de49122

???

8576fe1

????

322eb86

I should not change this line in this PR.

802deac

gilbsfr24 reopened this Jan 14, 2018

gilbsfr24 commented Jan 14, 2018

View reviewed changes

hean01-cendio added this to the rdesktop-1.8.4 milestone Jan 15, 2018

hean01-cendio modified the milestones: rdesktop-1.9.0, Next Jan 23, 2019

Multimonitor functionality #219

Are you sure you want to change the base?

Multimonitor functionality #219

Conversation

gilbsfr24 commented Jan 3, 2018

uglym8 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

uglym8 commented Jan 3, 2018

gilbsfr24 commented Jan 5, 2018

astrand commented Jan 9, 2018

gilbsfr24 commented Jan 9, 2018

uglym8 commented Jan 9, 2018

hean01-cendio commented Jan 9, 2018

gilbsfr24 commented Jan 9, 2018

gilbsfr24 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

uglym8 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gilbsfr24 commented Jan 13, 2018

gilbsfr24 commented Jan 14, 2018

gilbsfr24 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

uglym8 commented Jan 14, 2018

hean01-cendio commented Jan 15, 2018

gilbsfr24 commented Jan 15, 2018 • edited

gilbsfr24 commented Jan 15, 2018 • edited

uglym8 commented Jan 17, 2018

hean01-cendio commented Jan 17, 2018

hean01-cendio commented Jan 17, 2018 • edited

uglym8 commented Jan 17, 2018

gilbsfr24 commented Jan 18, 2018

gilbsfr24 commented Jan 18, 2018

uglym8 commented Jan 24, 2018

gilbsfr24 commented Jan 25, 2018

hean01-cendio commented Jan 25, 2018

uglym8 commented Jan 25, 2018

gilbsfr24 commented Jan 29, 2018

tuxflo commented Nov 27, 2019

gilbsfr24 commented Jan 15, 2018 •

edited

gilbsfr24 commented Jan 15, 2018 •

edited

hean01-cendio commented Jan 17, 2018 •

edited