Add scaleFactor option to RFB options #1376
Conversation
|
What are you using on the VNC serverside to achieve good up-scaling? In my experience, support for this is lacking in Linux. If thinking about merging this I'd like to see it used in the UI settings of |
|
If the use case is HiDPI, then a simple boolean of "native" resolution or not should be sufficient. Let's try to keep things simple. |
|
ping @jalfd |
|
No response |
|
Oops, very sorry, I completely dropped the ball on this. A boolean option to toggle "native" DPI sounds reasonable. Should I create a new PR for that? |
|
Go ahead and update this one 👍 |
011ae5a to
7d06dd7
Compare
This allows the viewport to be scaled to compensate for DPI scaling on high-DPI monitors. Previously, if DPI scaling was set to 200% in Windows, for example, noVNC would set the client area to twice the dimensions of the server-side framebuffer and scale it up to fit, leading to the VNC session looking grainy and low-res. useNativeDpi can now be set to true to compensate for this, and let framebuffer pixels exactly match monitor pixels.
Done! |
CendioOssman
left a comment
There was a problem hiding this comment.
Looks mostly good. I'd also like you to update the documentation for both the app and the library with this new setting.
| const dpr = window.devicePixelRatio || 1; | ||
| const scaledWidth = Math.floor(window.innerWidth * dpr); | ||
| // Compute the exact scale factor to match the floored width/height | ||
| this._display.scale = window.innerWidth / scaledWidth; |
There was a problem hiding this comment.
I don't understand the purpose of this calculation. Why not just 1 / window.devicePixelRatio?
|
|
||
| const size = this._screenSize(); | ||
| size.w = Math.floor(size.w / this._display.scale); | ||
| size.h = Math.floor(size.h / this._display.scale); |
There was a problem hiding this comment.
I'm not sure we want to tie this in to the display scaling right now. I would prefer if you just use window.devicePixelRatio and this._nativeDpi here.
| } | ||
|
|
||
| get useNativeDpi() { return this._nativeDpi; } | ||
| set useNativeDpi(nativeDpi) { |
There was a problem hiding this comment.
Should this be useNativeDPI? What does JavaScript APIs usually do with abbreviations?
samhed
left a comment
There was a problem hiding this comment.
Works great, tested in Chrome with a scale factor of 200%! 👍
Just a few minor style comments in addition to what @CendioOssman wrote
| /* ------^------- | ||
| * /VIEW CLIPPING | ||
| * ============== | ||
| * NATIVE DPI |
There was a problem hiding this comment.
Please center this like the other section names, same goes for "/NATIVE DPI" below
| // Update native DPI scaling for the viewport. If enabled, a pixel on the screen will | ||
| // correspond to one pixel on the server framebuffer. | ||
| // If disabled, the client area will be scaled according to the DPI scaling applied by the OS | ||
| // When this is disabled on a high-DPI monitor, a smaller framebuffer will be used and the | ||
| // result scaled up to fit the client area |
There was a problem hiding this comment.
Good explanation!
Could you keep the lines a bit shorter? Most of the code in UI attempts to stay within 80 columns I believe.
| </li> | ||
| <li><hr></li> | ||
| <li> | ||
| <label><input id="noVNC_setting_native_dpi" type="checkbox">Use native DPI</label> |
There was a problem hiding this comment.
The other surrounding checkboxes have a space in front of text
|
ping @jalfd |
|
Thanks for the feedback! |
|
Any luck getting the final parts fixed up? :) |
|
@samhed Maybe we should re-add the “noresponse” label? |
|
We try to close them now instead. The label doesn't really help out in getting some action on an issue. :/ |
This allows the viewport to be scaled to compensate for DPI scaling on
high-DPI monitors. Previously, if DPI scaling was set to 200% in Windows, for example,
noVNC would request a viewport half the size of the actual screen area,
and scale it up. Leading to the VNC session looking grainy and low-res.
The caller can now set scaleFactor to 1 / window.devicePixelRatio to
compensate for this, and get a canvas scaled to the actual physical
screen DPI.