Skip to content

GUACAMOLE-288: Add support for multi-monitor connections. - #560

Draft
corentin-soriano wants to merge 6 commits into
apache:mainfrom
corentin-soriano:GUACAMOLE-288_multi_monitors
Draft

GUACAMOLE-288: Add support for multi-monitor connections.#560
corentin-soriano wants to merge 6 commits into
apache:mainfrom
corentin-soriano:GUACAMOLE-288_multi_monitors

Conversation

@corentin-soriano

@corentin-soriano corentin-soriano commented Oct 20, 2024

Copy link
Copy Markdown
Member

Allows as is to add multiple monitors of the same dimensions in RDP protocol if the server supports it.
I haven't looked at the VNC part yet.

Client side PR : apache/guacamole-client#1061

POC:
image

Comment thread src/protocols/rdp/channels/disp.c Outdated
Comment thread src/protocols/rdp/channels/disp.c Outdated
Comment thread src/protocols/rdp/channels/disp.c Outdated
@corentin-soriano
corentin-soriano force-pushed the GUACAMOLE-288_multi_monitors branch from 4ee5e1b to ac08c95 Compare February 22, 2025 15:41
@Pierre-Gronau-ndaal

Copy link
Copy Markdown

it will be great if we bring that to main

@necouchman

necouchman commented Apr 15, 2025

Copy link
Copy Markdown
Contributor

it will be great if we bring that to main

Yes, that is the plan, once the code is finalized and has been fully reviewed.

@corentin-soriano
corentin-soriano force-pushed the GUACAMOLE-288_multi_monitors branch 2 times, most recently from f4716ce to 1fa8ce3 Compare May 23, 2025 14:58
@corentin-soriano
corentin-soriano force-pushed the GUACAMOLE-288_multi_monitors branch 2 times, most recently from afbaa06 to d44d527 Compare May 30, 2025 13:23
@corentin-soriano
corentin-soriano force-pushed the GUACAMOLE-288_multi_monitors branch 4 times, most recently from 9fb030d to 7a8e74a Compare June 8, 2025 13:43
@corentin-soriano
corentin-soriano force-pushed the GUACAMOLE-288_multi_monitors branch from eda5067 to 5294c1b Compare June 17, 2025 15:49
@Vertganti

Copy link
Copy Markdown

Frequently resizing a monitor window, e.g. by using Maximize and Restore down of the window in quick succession, can cause a RDP disconnect with the following message in the guacd logs:

GUAC_ASSERT in guac_rdp_gdi_desktop_resize() failed at gdi.c:172.

Assertion code

Is it possible to wait for the flush or just discard the affected window instead of crashing the connection?

@corentin-soriano

Copy link
Copy Markdown
Member Author

Frequently resizing a monitor window, e.g. by using Maximize and Restore down of the window in quick succession, can cause a RDP disconnect with the following message in the guacd logs:

GUAC_ASSERT in guac_rdp_gdi_desktop_resize() failed at gdi.c:172.

Assertion code

Is it possible to wait for the flush or just discard the affected window instead of crashing the connection?

Which version of freerdp are you using?
If you are on v3, can you update it to at least version 3.8.0?

@Vertganti

Vertganti commented Jul 7, 2025

Copy link
Copy Markdown

I'm using the Dockerfile from the repository which defaults to FreeRDP 2. The exact version is 2.11.7. I can try using FreeRDP 3 to see if that improves anything.

@corentin-soriano

Copy link
Copy Markdown
Member Author

I'm using the Dockerfile form the repository which defaults to FreeRDP 2. The exact version is 2.11.7. I can try using FreeRDP 3 to see if that improves anything.

This is potentially a bug in FreeRDP. This assertion is mandatory in version 2.
Are you sure you can't reproduce this with a single monitor?

@Vertganti

This comment was marked as resolved.

@Vertganti

Vertganti commented Jul 7, 2025

Copy link
Copy Markdown

I'm using the Dockerfile form the repository which defaults to FreeRDP 2. The exact version is 2.11.7. I can try using FreeRDP 3 to see if that improves anything.

This is potentially a bug in FreeRDP. This assertion is mandatory in version 2. Are you sure you can't reproduce this with a single monitor?

I was able to reproduce it with one monitor with the FreeRDP 2 build.

EDIT: It does not reproduce with FreeRDP 3, independent of the number of monitors.

Comment thread src/protocols/rdp/channels/disp.c
Comment thread src/protocols/rdp/client.c
Comment thread src/protocols/rdp/gdi.c
/* Make json string containing monitor information */
char json[JSON_BUFFER_SIZE];
int pos = 0;
pos += snprintf(json + pos, JSON_BUFFER_SIZE - pos, "{");

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: While there may be concerns about grabbing an external dependency, when it comes to "never re-invent the wheel" any manual string manipulation to do json generation/parsing is king. Libraries like

https://github.com/nlohmann/json

will allow "C++ like" json parsing and generation that covers all the corner cases that JSON hides from the average user.

Comment thread src/protocols/rdp/gdi.c
}

/* Append monitor information to JSON string */
pos += snprintf(json + pos, JSON_BUFFER_SIZE - pos,

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A small piece of my soul is severed and sent to the depths of Hades every time I see sprintf generating json.

@lirannisani

lirannisani commented Sep 4, 2025

Copy link
Copy Markdown

@necouchman @corentin-soriano
Thanks for adding this feature!
Do you plan to merge it into the main branch?

@corentin-soriano

Copy link
Copy Markdown
Member Author

@necouchman @corentin-soriano Thanks for adding this feature! Do you plan to merge it into the main branch?

I still have some things to finalize before we can merge.
I haven't had time to continue lately but the subject has not been forgotten!

@necouchman necouchman mentioned this pull request Sep 24, 2025
@corentin-soriano
corentin-soriano force-pushed the GUACAMOLE-288_multi_monitors branch from 5294c1b to 5989c14 Compare November 3, 2025 11:40
@corentin-soriano
corentin-soriano force-pushed the GUACAMOLE-288_multi_monitors branch from 5989c14 to ce23d60 Compare November 6, 2025 08:40
ciroiriarte added a commit to ciroiriarte/guacamole-server that referenced this pull request Jul 4, 2026
Ports the reusable, protocol-agnostic multi-monitor support added
upstream in apache#560 (GUACAMOLE-288) by Corentin
Soriano (@corentin-soriano):

  * guac_user_size_handler gains per-monitor x_position/top_offset
  * new "multimon-layout" default-layer parameter (a JSON monitor map)
  * __guac_handle_size parses the new size-instruction fields
  * guac_rect_shrink guards against divide-by-zero

Every protocol's size handler is updated to the new signature; the new
per-monitor arguments are ignored by all protocols except SPICE (see the
following commit), preserving existing single-monitor behavior.

Original work by @corentin-soriano, adapted for this fork:
apache#560
ciroiriarte added a commit to ciroiriarte/guacamole-server that referenced this pull request Jul 4, 2026
Builds on the ported libguac multi-monitor primitive to add
client-driven multi-monitor support to the SPICE protocol, following the
design of the RDP implementation in apache#560 (by
@corentin-soriano) adapted to SPICE's model, where a QXL guest presents
all heads as regions of a single combined framebuffer.

  * settings: new "secondary-monitors" parameter (max_secondary_monitors,
    default 0 = disabled)
  * the size handler tracks a per-monitor array (fixed, up to
    GUAC_SPICE_MAX_MONITORS) keyed by x_position, tiling monitors
    left-to-right and computing each monitor's left_offset; a
    non-positive size closes a secondary monitor
  * resize pushes the whole layout to the guest agent in a single
    monitors config (enabling/positioning active monitors and disabling
    removed ones)
  * on each combined-surface (re)create, the current layout is sent to
    the client as the "multimon-layout" parameter on the default layer
    so a multi-monitor client can split it into per-monitor windows
  * joining users are told the permitted secondary-monitor count via a
    "secondary-monitors" argv stream

The monitor state uses a fixed array (no dynamic allocation), avoiding
the pointer-ownership concerns raised in review of the upstream RDP
change, and the default layer is referenced via GUAC_DEFAULT_LAYER
rather than casting a guac_display_layer.
ciroiriarte added a commit to ciroiriarte/guacamole-server that referenced this pull request Jul 10, 2026
Ports the reusable, protocol-agnostic multi-monitor support added
upstream in apache#560 (GUACAMOLE-288) by Corentin
Soriano (@corentin-soriano):

  * guac_user_size_handler gains per-monitor x_position/top_offset
  * new "multimon-layout" default-layer parameter (a JSON monitor map)
  * __guac_handle_size parses the new size-instruction fields
  * guac_rect_shrink guards against divide-by-zero

Every protocol's size handler is updated to the new signature; the new
per-monitor arguments are ignored by all protocols except SPICE (see the
following commit), preserving existing single-monitor behavior.

Original work by @corentin-soriano, adapted for this fork:
apache#560
ciroiriarte added a commit to ciroiriarte/guacamole-server that referenced this pull request Jul 10, 2026
Builds on the ported libguac multi-monitor primitive to add
client-driven multi-monitor support to the SPICE protocol, following the
design of the RDP implementation in apache#560 (by
@corentin-soriano) adapted to SPICE's model, where a QXL guest presents
all heads as regions of a single combined framebuffer.

  * settings: new "secondary-monitors" parameter (max_secondary_monitors,
    default 0 = disabled)
  * the size handler tracks a per-monitor array (fixed, up to
    GUAC_SPICE_MAX_MONITORS) keyed by x_position, tiling monitors
    left-to-right and computing each monitor's left_offset; a
    non-positive size closes a secondary monitor
  * resize pushes the whole layout to the guest agent in a single
    monitors config (enabling/positioning active monitors and disabling
    removed ones)
  * on each combined-surface (re)create, the current layout is sent to
    the client as the "multimon-layout" parameter on the default layer
    so a multi-monitor client can split it into per-monitor windows
  * joining users are told the permitted secondary-monitor count via a
    "secondary-monitors" argv stream

The monitor state uses a fixed array (no dynamic allocation), avoiding
the pointer-ownership concerns raised in review of the upstream RDP
change, and the default layer is referenced via GUAC_DEFAULT_LAYER
rather than casting a guac_display_layer.
ciroiriarte added a commit to ciroiriarte/guacamole-server that referenced this pull request Jul 10, 2026
Ports the reusable, protocol-agnostic multi-monitor support added
upstream in apache#560 (GUACAMOLE-288) by Corentin
Soriano (@corentin-soriano):

  * guac_user_size_handler gains per-monitor x_position/top_offset
  * new "multimon-layout" default-layer parameter (a JSON monitor map)
  * __guac_handle_size parses the new size-instruction fields
  * guac_rect_shrink guards against divide-by-zero

Every protocol's size handler is updated to the new signature; the new
per-monitor arguments are ignored by all protocols except SPICE (see the
following commit), preserving existing single-monitor behavior.

Original work by @corentin-soriano, adapted for this fork:
apache#560
ciroiriarte added a commit to ciroiriarte/guacamole-server that referenced this pull request Jul 10, 2026
Builds on the ported libguac multi-monitor primitive to add
client-driven multi-monitor support to the SPICE protocol, following the
design of the RDP implementation in apache#560 (by
@corentin-soriano) adapted to SPICE's model, where a QXL guest presents
all heads as regions of a single combined framebuffer.

  * settings: new "secondary-monitors" parameter (max_secondary_monitors,
    default 0 = disabled)
  * the size handler tracks a per-monitor array (fixed, up to
    GUAC_SPICE_MAX_MONITORS) keyed by x_position, tiling monitors
    left-to-right and computing each monitor's left_offset; a
    non-positive size closes a secondary monitor
  * resize pushes the whole layout to the guest agent in a single
    monitors config (enabling/positioning active monitors and disabling
    removed ones)
  * on each combined-surface (re)create, the current layout is sent to
    the client as the "multimon-layout" parameter on the default layer
    so a multi-monitor client can split it into per-monitor windows
  * joining users are told the permitted secondary-monitor count via a
    "secondary-monitors" argv stream

The monitor state uses a fixed array (no dynamic allocation), avoiding
the pointer-ownership concerns raised in review of the upstream RDP
change, and the default layer is referenced via GUAC_DEFAULT_LAYER
rather than casting a guac_display_layer.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants