devolutions-gateway icon indicating copy to clipboard operation
devolutions-gateway copied to clipboard

refactor(webapp): refactor VNC encoding selection settings

Open RRRadicalEdward opened this issue 3 months ago • 9 comments

  • refactor the encoding selector to select one encoding at a time (we have the same in RDM)
  • add Default encoding to the encoding selector list, which defaults to sending all encodings to the server, and let it select one (we have the same in RDM)
  • added logic to hide the pixel format option if Tight JPEG or Tight PNG is selected. These don't work well together, as the https://github.com/Devolutions/IronVNC/issues/924 bug happens.
  • removed Tight PNG encoding from the encoding selector and added a checkbox with the same appearing only when Tight encoding is selected. Tight PNG is more like an extension for Tight (like JPEG), rather than a separate thing. So, it makes sense to keep it as a setting for Tight encoding.
  • added hiding JPEG option for other encodings than Tight, as it's only matters when Tight is enabled.

Showcase:

https://github.com/user-attachments/assets/7019761b-d811-467e-8296-eec8875766b7

RRRadicalEdward avatar Sep 12 '25 07:09 RRRadicalEdward

Let maintainers know that an action is required on their side

  • Add the label https://github.com/Devolutions/devolutions-gateway/labels/release-required when you request a maintainer to cut a new release (Devolutions Gateway, Devolutions Agent, Jetsocat, PowerShell module)

  • Add the label https://github.com/Devolutions/devolutions-gateway/labels/release-blocker if a follow-up is required before cutting a new release

  • Add the label https://github.com/Devolutions/devolutions-gateway/labels/publish-required when you request a maintainer to publish libraries (Devolutions.Gateway.Utils, OpenAPI clients, etc.)

  • Add the label https://github.com/Devolutions/devolutions-gateway/labels/publish-blocker if a follow-up is required before publishing libraries

github-actions[bot] avatar Sep 12 '25 07:09 github-actions[bot]

In RDM, we have a list of "preferred encodings"

image

Also, here is what @thenextman said on this topic:

I would think hard about giving the user a list of “preferred encodings” again. Because it’s just technobabble to most people and rarely used. Something I’d discussed with David in the past was presenting it more like “Automatic”, “High quality / slower”, “medium”, “low quality / faster”. That kind of thing. With preconfigured preferences underneath.

Given this feedback, I think its not really worth supporting the "preferred encodings" of RDM, and instead provide an extension for the "Encoding Profile". Instead of disabling / enabling, we should adapt the ordering.

What do you think? Maybe we can talk more deeply on Slack.

CBenoit avatar Sep 17 '25 02:09 CBenoit

In RDM, we have a list of "preferred encodings" image

Also, here is what @thenextman said on this topic:

I would think hard about giving the user a list of “preferred encodings” again. Because it’s just technobabble to most people and rarely used. Something I’d discussed with David in the past was presenting it more like “Automatic”, “High quality / slower”, “medium”, “low quality / faster”. That kind of thing. With preconfigured preferences underneath.

Given this feedback, I think its not really worth supporting the "preferred encodings" of RDM, and instead provide an extension for the "Encoding Profile". Instead of disabling / enabling, we should adapt the ordering.

What do you think? Maybe we can talk more deeply on Slack.

I see, so it should look like ArdQualityMode which we have for ARD session instead of "preferred encodings": https://github.com/Devolutions/IronVNC/blob/a9be40a51d3f56e4fba2c32150d4babf16677a09/crates/ironvnc-cfg/src/ard_quality_mode.rs#L6-L13, and used this way https://github.com/Devolutions/IronVNC/blob/a9be40a51d3f56e4fba2c32150d4babf16677a09/crates/ironvnc-web/src/session.rs#L795-L810. I think it's a good idea. I will come back to you with the brainstormed list of encoding profiles and what encoding to include where, as I see it.

RRRadicalEdward avatar Sep 17 '25 07:09 RRRadicalEdward

In RDM, we have a list of "preferred encodings" image Also, here is what @thenextman said on this topic:

I would think hard about giving the user a list of “preferred encodings” again. Because it’s just technobabble to most people and rarely used. Something I’d discussed with David in the past was presenting it more like “Automatic”, “High quality / slower”, “medium”, “low quality / faster”. That kind of thing. With preconfigured preferences underneath.

Given this feedback, I think its not really worth supporting the "preferred encodings" of RDM, and instead provide an extension for the "Encoding Profile". Instead of disabling / enabling, we should adapt the ordering. What do you think? Maybe we can talk more deeply on Slack.

I see, so it should look like ArdQualityMode which we have for ARD session instead of "preferred encodings": https://github.com/Devolutions/IronVNC/blob/a9be40a51d3f56e4fba2c32150d4babf16677a09/crates/ironvnc-cfg/src/ard_quality_mode.rs#L6-L13, and used this way https://github.com/Devolutions/IronVNC/blob/a9be40a51d3f56e4fba2c32150d4babf16677a09/crates/ironvnc-web/src/session.rs#L795-L810. I think it's a good idea. I will come back to you with the brainstormed list of encoding profiles and what encoding to include where, as I see it.

Something like that! Really appreciated.

However, I think we should also consider having multiple "IronEncodingSet"s, to change the ordering rather than filtering out encodings. And we can probably refactor that a bit using a declarative macro. Alternatively, we extend the ironvnc-session crate and offer an API to assign weights to Encodings as a way to change the ordering when sending the SetEncodings packet.

CBenoit avatar Sep 17 '25 10:09 CBenoit

In IronEncodingSet, we also have pseudo-encoding, and with this change, we need to pay attention to keep sending them for all possible encoding sets. If we refactored IronEncodingSet to have a few smaller sets, we would have to copy-paste the pseudo-encodings to each set or come up with a way to separate them from the no-pseudo-encodings.

Alternatively, we extend the ironvnc-session crate and offer an API to assign weights to Encodings as a way to change the ordering when sending the SetEncodings packet.

It might be a better solution here, as we wouldn't have to touch the pseudo-encodings.

RRRadicalEdward avatar Sep 17 '25 11:09 RRRadicalEdward

In IronEncodingSet, we also have pseudo-encoding, and with this change, we need to pay attention to keep sending them for all possible encoding sets. If we refactored IronEncodingSet to have a few smaller sets, we would have to copy-paste the pseudo-encodings to each set or come up with a way to separate them from the no-pseudo-encodings.

I think we can factor that using a declarative macro.

Alternatively, we extend the ironvnc-session crate and offer an API to assign weights to Encodings as a way to change the ordering when sending the SetEncodings packet.

It might be a better solution here, as we wouldn't have to touch the pseudo-encodings.

But yes, I have a small preference for this solution as well. I think it’s better than completely relying on the ordering in the enum.

CBenoit avatar Sep 17 '25 16:09 CBenoit

In IronEncodingSet, we also have pseudo-encoding, and with this change, we need to pay attention to keep sending them for all possible encoding sets. If we refactored IronEncodingSet to have a few smaller sets, we would have to copy-paste the pseudo-encodings to each set or come up with a way to separate them from the no-pseudo-encodings.

I think we can factor that using a declarative macro.

Alternatively, we extend the ironvnc-session crate and offer an API to assign weights to Encodings as a way to change the ordering when sending the SetEncodings packet.

It might be a better solution here, as we wouldn't have to touch the pseudo-encodings.

But yes, I have a small preference for this solution as well. I think it’s better than completely relying on the ordering in the enum.

Another solution can be to have the pseudo-encoding in a separate set(it might be hardcoded, and user code won't be able to change it)

RRRadicalEdward avatar Sep 18 '25 07:09 RRRadicalEdward

Another solution can be to have the pseudo-encoding in a separate set (it might be hardcoded, and user code won't be able to change it)

Makes sense too! :thinking:

CBenoit avatar Sep 19 '25 12:09 CBenoit

I will come back to you with the brainstormed list of encoding profiles and what encoding to include where, as I see it.

Hi @CBenoit @thenextman. I prepared a list of VNC encoding profiles (similar to ARD quality modes in FreeVNC/IronVNC) that make sense to show to a user instead of having a "preferred encodings" list.

At first, I separated the encodings in terms of CPU and bandwidth usage, and got this table(CPU is vertical, and bandwidth is horizontal). Note that we don't have H.264, CoRRE, RRE, and ZRLE implemented in IronVNC yet.

CPU/bandwidth High Middle Low
High
  • ZlibHextile
  • ZLib
H.264
Middle TightPNG
  • TightJPEG
  • Tight
  • ZRLE
Low
  • Hextile
  • Raw
  • CoRRE
  • RRE
CopyRect

Using this table, I got the following encoding profiles (the higher the better). High CPU, low bandwidth

  • H.264 (lossy)
  • TightJPEG (lossy)
  • Tight
  • ZRLE
  • CopyRect

High CPU, middle bandwidth

  • ZlibHextile
  • Zlib
  • H.264
  • TightPNG
  • TightJPEG (lossy)
  • TIght
  • ZRLE
  • CoRRE
  • RRE
  • CopyRect

Low CPU, high bandwidth

  • Hextile
  • CoRRE
  • RRE
  • Raw
  • CopyRect

Low CPU, middle bandwidth

  • CoRRE
  • RRE
  • CopyRect

Middle CPU, middle bandwidth

  • TightJPEG (lossy)
  • TightPNG
  • Tight
  • ZRLE
  • CoRRE
  • RRE
  • CopyRect

I think allowing the user to select the desired CPU consumption, and specify the network bandwidth, is better than selecting preferred encoding, and should give the user a clear explanation of what to expect. Let me know what you think about the encoding sets 🙂.

RRRadicalEdward avatar Sep 24 '25 16:09 RRRadicalEdward