Skip to content

Conversation

RRRadicalEdward
Copy link
Collaborator

  • 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:

encoding-selector-refactoring.mp4

Copy link

Let maintainers know that an action is required on their side

  • Add the label release-required Please cut a new release (Devolutions Gateway, Devolutions Agent, Jetsocat, PowerShell module) when you request a maintainer to cut a new release (Devolutions Gateway, Devolutions Agent, Jetsocat, PowerShell module)

  • Add the label release-blocker Follow-up is required before cutting a new release if a follow-up is required before cutting a new release

  • Add the label publish-required Please publish libraries (`Devolutions.Gateway.Utils`, OpenAPI clients, etc) when you request a maintainer to publish libraries (Devolutions.Gateway.Utils, OpenAPI clients, etc.)

  • Add the label publish-blocker Follow-up is required before publishing libraries if a follow-up is required before publishing libraries

* refactor the encoding selector to is able 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 doesn't work well together as Devolutions/IronVNC#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 seperate thign. 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.
@RRRadicalEdward RRRadicalEdward force-pushed the webapp-vnc-encoding-selector-refactor branch from 07f6cb6 to e92a6a4 Compare September 12, 2025 07:53
@RRRadicalEdward RRRadicalEdward self-assigned this Sep 12, 2025
@CBenoit
Copy link
Member

CBenoit commented Sep 17, 2025

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.

@RRRadicalEdward
Copy link
Collaborator Author

RRRadicalEdward commented Sep 17, 2025

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.

@CBenoit
Copy link
Member

CBenoit commented Sep 17, 2025

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.

@RRRadicalEdward
Copy link
Collaborator Author

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.

@CBenoit
Copy link
Member

CBenoit commented Sep 17, 2025

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.

@RRRadicalEdward
Copy link
Collaborator Author

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)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants