Add Hi DPI Support for GLFW #20584

ypujante · 2023-10-31T18:09:12Z

This is the PR following the discussion Support for 4k/retina displays

This is also my first PR and I tried to be as conservative as possible, read the contributing notes, ran as many tests I could on my local machine... I have also tried to minimize the amount of changes.

In the end I have implemented support for Hi DPI environment with webgpu/glfw. I can work in adding other frameworks (SDL, glut, etc...) in the future if this PR is successful and you think it would be worthwhile.

As described in the discussion, I ended up adding a property to the Browser object called isHiDPIAware which is false by default and can be set by calling setHiDPIAware (which the demos use). It is also set automatically by glfw depending on the value of Module.isHiDPIAware (in glfwInit()).

I have created 2 public examples demonstrating the changes (which ends up using webgpu/glfw under the cover).

This demos work with Chrome (119+), Firefox (119+), Safari (16.6+). If you are on a 4k monitor then you will see a difference when checking the isHiDPIAware checkbox (it's 100% dynamic). Note that if you move the window from a 4k screen to a 2k screen, the change happens automatically which is pretty cool.

The reason of these 2 demos is because, when there is a shell, the canvas is non-resizable in its own but you can make it fullscreen by clicking the Fullscreen button. The other demo, shows an example where the canvas size is dynamic and take up the full window (completely different code paths, as onCanvasResize gets called on every change). In this demo, you can select the Google Chrome menu to go Fullscreen. Note that this use-case was triggering an error in the code library_glfw.js line 986 because calling setSize with the exact size of the monitor would end up calling Browser.requestFullscreen which triggers an error when it is not triggered by the user explicitly (I added a comment in the code with the error).

I did not add any tests because I do not know how to test it since it depends on the tests running on Google Chrome (no other browser to my knowledge supports webgpu) on a 4k monitor. I have no idea if the environment is that way. I will be more than happy to add test(s) if you give me some hints on what to do.

I will also gladly change anything you want me to change to abide by any rules I might have overlooked.

Thank you

- added Browser.isHiDPIAware flag that gets initialized from Module.isHiDPIAware - from the webgpu samples (https://webgpu.github.io/webgpu-samples/samples/helloTriangle), the gist of the logic is to scale canvas.width (resp. canvas.height) by the devicePixelRatio and set canvas.clientWidth to the width - coordinates are then scaled down to reflect what the client is expecting

- calling Browser.requestFullscreen raises "Failed to execute 'requestFullscreen' on 'Element': API can only be initiated by a user gesture"

- handle case where the canvas size is fixed (default shell) - handle case where request full screen is issued by the user (default shell)

sbc100

Thanks for details writeup and great looking first PR!

I'm not too familiar with this part of the codebase but the general direction looks good.

src/library_browser.js

src/library_glfw.js

- created issue emscripten-core#20600 to implement in a separate PR

- the rationale is that the client can simply call Module.setHiDPIAware based on the needs

ypujante · 2023-11-02T18:40:24Z

I just did a push that addresses the issues:

fixed code style
reverted changes about fullscreen (created issue Calling GLFW.setWindowSize with the screen size leads to browser error #20600)
reverted Browser.isHiDPIAware initialization entirely

src/library_browser.js

- ironically, although the call was wrong, it ended up doing the right thing...

ypujante · 2023-11-02T20:23:08Z

@sbc100 Reverting the changes related to issue #20600 makes fullscreen/exit fullscreen no longer work in my test scenarios. Since I reverted the changes I had made, I assume it works like it used to. But since I don't have a good way to verify it, I don't feel very comfortable with the state of the PR at this moment. I will let you know my findings after I spend more time with it.

ypujante · 2023-11-02T21:10:51Z

Well good news. I did some more local testing and by using the default shell that comes with emscripten, the "Fullscreen" button works as expected with the changes in this PR. I can also see that all the tests have passed. So I am now confident with the state of this PR.

Of course, the other use case we discussed in issue #20600 still remain a problem, but it is the same issue before this PR. So I am going to spend some time now and try to see if I can address it. But we have decoupled it from this PR which I think was the right call.

src/library_browser.js

sbc100 · 2023-11-02T21:52:21Z

@juj WDYT?

ypujante · 2023-11-03T13:42:11Z

I can see that github says that there is a conflict that must be resolved. When I click "Resolve conflicts", it doesn't look like I can do anything about it. Am I supposed to fix it on my fork and push again?

sbc100 · 2023-11-03T18:47:05Z

Yes, I would fix locally then then push again. You can use either merge or rebase as you prefer.

# Conflicts: # src/library_glfw.js

ypujante · 2023-11-03T23:39:10Z

@sbc100 I did a merge and pushed it. I am not sure I understand why there are 90 files changed. Let me know if this is normal or if I screwed something up. My branch does not look like that...

sbc100 · 2023-11-04T05:53:31Z

@sbc100 I did a merge and pushed it. I am not sure I understand why there are 90 files changed. Let me know if this is normal or if I screwed something up. My branch does not look like that...

I think you screwed something up yes... Im not sure exactly what but it doesn't look right :-/

ypujante · 2023-11-04T12:11:18Z

@sbc100 I believe it is fixed now as the PR only shows the 2 modified files (I used github "Sync" branch on my fork this time). I also checked that the changes to the 2 files are what I was expecting and double checked that the expected behavior is the same on my local test playground.

# Conflicts: # src/library_browser.js

ypujante · 2023-11-06T18:04:52Z

Fixed the merge conflict introduced by the latest check ins and pushed the branch. It looks like this time I did it right the first time...

ypujante · 2023-11-06T19:28:11Z

As far as I can tell the failing tests have nothing to do with my changes:

ports:INFO: retrieving port: mpg123 from https://www.mpg123.de/download/mpg123-1.26.2.tar.bz2
Traceback (most recent call last):
  File "/usr/lib/python3/dist-packages/urllib3/connection.py", line 141, in _new_conn
    (self.host, self.port), self.timeout, **extra_kw)
  File "/usr/lib/python3/dist-packages/urllib3/util/connection.py", line 83, in create_connection
    raise err
  File "/usr/lib/python3/dist-packages/urllib3/util/connection.py", line 73, in create_connection
    sock.connect(sa)
TimeoutError: [Errno 110] Connection timed out

During handling of the above exception, another exception occurred:

Do I need to do anything about this?

ypujante · 2023-12-01T18:43:54Z

Tests failing seems to have nothing with my changes:

FAIL [121.015s]: test_async_mainloop (test_browser.browser)
FAIL [120.957s]: test_async_jspi_wasm_bigint (test_browser.browser)
FAIL [121.059s]: test_async_returnvalue_em_js_bad (test_browser.browser)
FAIL [122.875s]: test_async_jspi (test_browser.browser)
FAIL [120.880s]: test_async_returnvalue_empty_list (test_browser.browser)

juj · 2023-12-04T10:52:11Z

This sounds very interesting. Could you point me to some documentation about it?

What I mean here is that user JS libraries no longer have limitations that they used to, and users can author JS libraries that have all the capabilities that system JS libraries have.

The mechanism I was referring to landed in this PR: https://github.com/emscripten-core/emscripten/pull/18849/files , although there was no documentation added (so follow the syntax that exists in the current JS libraries)

juj · 2023-12-04T10:55:57Z

src/library_glfw.js

@@ -105,6 +107,7 @@ var LibraryGLFW = {
    versionString: null,
    initialTime: null,
    extensions: null,
+    devicePixelRatioMQS: null,


What is MQS short for?

It was supposed to be MediaQuerySearch but I realized that window.matchMedia returns a MediaQueryList not MediaQuerySearch... so I just renamed it devicePixelRatioMQL and added a comment for clarity

src/library_glfw.js

juj · 2023-12-04T11:09:47Z

Looks good except for that one double-checking question.

Nice to see the existing GLFW attribute GLFW_SCALE_TO_MONITOR be used.

# Conflicts: # src/library_glfw.js

ypujante · 2023-12-05T13:54:58Z

Yay! Thank you @juj

I fixed the merge conflict and hopefully the tests will pass!

Let me know if you want me to update release notes/documentation, etc... when you are ready for the merge.

ypujante · 2023-12-05T15:10:32Z

The dreaded random tests failures are back:

FAIL [122.412s]: test_async_2 (test_browser.browser64_4gb)
FAIL [120.975s]: test_async_asyncify (test_browser.browser64_4gb)
FAIL [121.017s]: test_async_bad_list (test_browser.browser64_4gb)
FAIL [120.942s]: test_async_compile (test_browser.browser64_4gb)
FAIL [121.248s]: test_async_in_pthread (test_browser.browser64_4gb)

But the primary test for this PR is passing:

  test_glfw3_hi_dpi_aware (test_browser.browser64) ... Clearing existing test directory
cache:INFO: generating system asset: symbol_lists/2b7e561b0bade8e7f068e21c61f5fbc67208cca1.json... (this will be cached in "/root/cache/symbol_lists/2b7e561b0bade8e7f068e21c61f5fbc67208cca1.json" for subsequent builds)
cache:INFO:  - ok
ok (0.936s)

ypujante · 2023-12-05T16:23:32Z

@juj as a side note regarding your comment:

b) how big is your appetite to further develop library_glfw.js? If you're enthusiastic for championing further development, maybe this might be the time to migrate library_glfw.js into its own repository and maybe use the ports system to distribute it? (that's ok if not, just bringing up the idea in general)

I have started looking into this... and toying around with implementation (c++ like your webgpu library)... :)

ypujante · 2023-12-06T14:56:41Z

After merging with latest branch as of this morning, all tests are now passing. Yay!

sbc100 · 2023-12-06T20:29:09Z

Great! Landing this now.

ypujante · 2023-12-06T20:30:24Z

@sbc100 Thank you! Let me know if you want me to update any release notes or documentation! (I can do a PR just for that purpose)

sbc100 · 2023-12-06T20:32:33Z

We don't really have any specific docs about our GLFW implementations (at least not that I know of). If you like you could add a ChangeLog entry.

ypujante · 2023-12-09T19:53:38Z

@juj I must admit I have been enjoying working on a glfw3 port... although I am far from finished, I already do support multiple canvases each with their own hi dpi aware flag! I had no idea if I could make it work, but it does (as can be seen in the screenshot). So exciting!!!

// from main.cpp
  glfwWindowHint(GLFW_SCALE_TO_MONITOR, GLFW_TRUE);
  glfwWindowHintString(GLFW_EMSCRIPTEN_CANVAS_SELECTOR, "#canvas1");
  auto window1 = glfwCreateWindow(300, 200, "4k", nullptr, nullptr);

  glfwWindowHint(GLFW_SCALE_TO_MONITOR, GLFW_FALSE);
  glfwWindowHintString(GLFW_EMSCRIPTEN_CANVAS_SELECTOR, "#canvas2");
  auto window2 = glfwCreateWindow(300, 200, "2k", nullptr, nullptr);

// from shell.html
<canvas class=emscripten id=canvas1 oncontextmenu=event.preventDefault() tabindex=-1></canvas>
<canvas class=emscripten id=canvas2 oncontextmenu=event.preventDefault() tabindex=-1></canvas>

ypujante added 5 commits October 29, 2023 06:58

fixed fullscreen mode

7c457a4

- calling Browser.requestFullscreen raises "Failed to execute 'requestFullscreen' on 'Element': API can only be initiated by a user gesture"

removed log statements

6c0e378

Make sure it works for "default" shell

cbeb0e1

- handle case where the canvas size is fixed (default shell) - handle case where request full screen is issued by the user (default shell)

fixes fullscreen mode (user triggered)

ba13c6f

ypujante mentioned this pull request Oct 31, 2023

[core] Very low rendering quality on 4K displays on PLATFORM_WEB raysan5/raylib#3372

Closed

sbc100 reviewed Nov 1, 2023

View reviewed changes

src/library_browser.js Outdated Show resolved Hide resolved

src/library_browser.js Outdated Show resolved Hide resolved

src/library_glfw.js Outdated Show resolved Hide resolved

src/library_glfw.js Outdated Show resolved Hide resolved

sbc100 requested a review from juj November 1, 2023 22:13

[PR Review]: Fixes code style

9f7a2a2

ypujante mentioned this pull request Nov 2, 2023

Calling GLFW.setWindowSize with the screen size leads to browser error #20600

Closed

ypujante added 2 commits November 2, 2023 09:00

[PR Review]: Reverting changes (separate issue)

454cd0f

- created issue emscripten-core#20600 to implement in a separate PR

[PR Review]: getting rid of initialization

15f1ff1

- the rationale is that the client can simply call Module.setHiDPIAware based on the needs

sbc100 reviewed Nov 2, 2023

View reviewed changes

src/library_browser.js Outdated Show resolved Hide resolved

[PR Review]: fix bad api call

e38a34c

- ironically, although the call was wrong, it ended up doing the right thing...

sbc100 reviewed Nov 2, 2023

View reviewed changes

src/library_browser.js Outdated Show resolved Hide resolved

Merge branch 'hi-dpi-aware'

b291873

# Conflicts: # src/library_glfw.js

Merge branch 'emscripten-core:main' into hi-dpi-aware

51f3288

sbc100 requested a review from kripken November 5, 2023 18:04

Merge remote-tracking branch 'origin/main' into hi-dpi-aware

08a5b69

# Conflicts: # src/library_browser.js

sbc100 mentioned this pull request Dec 1, 2023

Browser tests often fail with "[no http server activity]" #20820

Closed

juj reviewed Dec 4, 2023

View reviewed changes

src/library_glfw.js Outdated Show resolved Hide resolved

juj reviewed Dec 4, 2023

View reviewed changes

src/library_glfw.js Show resolved Hide resolved

juj reviewed Dec 4, 2023

View reviewed changes

src/library_glfw.js Show resolved Hide resolved

ypujante added 4 commits December 4, 2023 05:20

fixed missing ;

66c6e2a

Merge remote-tracking branch 'origin/main' into hi-dpi-aware

4187761

renamed attribute

822f55d

added descriptive comment

8a58de4

juj approved these changes Dec 5, 2023

View reviewed changes

juj mentioned this pull request Dec 5, 2023

Fix Glfw window resize #20831

Closed

ypujante added 2 commits December 5, 2023 05:28

fixed failing test (due to variable rename)

be0979e

Merge remote-tracking branch 'origin/main' into hi-dpi-aware

1b87dd7

# Conflicts: # src/library_glfw.js

Merge remote-tracking branch 'origin/main' into hi-dpi-aware

a3fb66d

sbc100 approved these changes Dec 6, 2023

View reviewed changes

sbc100 merged commit c26fc38 into emscripten-core:main Dec 6, 2023
27 checks passed

ypujante mentioned this pull request Dec 18, 2023

examples/example_emscripten_wgpu: Initialize and change the size of the viewport and framebuffer according to the size of the canvas ocornut/imgui#6751

Closed

luigifcruz mentioned this pull request Jan 6, 2024

Handle canvas framebuffer scaling. #19844

Closed

ypujante deleted the hi-dpi-aware branch March 23, 2024 18:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Hi DPI Support for GLFW #20584

Add Hi DPI Support for GLFW #20584

ypujante commented Oct 31, 2023 •

edited

Loading

sbc100 left a comment

ypujante commented Nov 2, 2023

ypujante commented Nov 2, 2023

ypujante commented Nov 2, 2023

sbc100 commented Nov 2, 2023

ypujante commented Nov 3, 2023

sbc100 commented Nov 3, 2023

ypujante commented Nov 3, 2023 •

edited

Loading

sbc100 commented Nov 4, 2023

ypujante commented Nov 4, 2023

ypujante commented Nov 6, 2023

ypujante commented Nov 6, 2023

ypujante commented Dec 1, 2023

juj commented Dec 4, 2023

juj Dec 4, 2023

ypujante Dec 4, 2023

juj commented Dec 4, 2023

ypujante commented Dec 5, 2023

ypujante commented Dec 5, 2023

ypujante commented Dec 5, 2023

ypujante commented Dec 6, 2023

sbc100 commented Dec 6, 2023

ypujante commented Dec 6, 2023

sbc100 commented Dec 6, 2023

ypujante commented Dec 9, 2023

Add Hi DPI Support for GLFW #20584

Add Hi DPI Support for GLFW #20584

Conversation

ypujante commented Oct 31, 2023 • edited Loading

sbc100 left a comment

Choose a reason for hiding this comment

ypujante commented Nov 2, 2023

ypujante commented Nov 2, 2023

ypujante commented Nov 2, 2023

sbc100 commented Nov 2, 2023

ypujante commented Nov 3, 2023

sbc100 commented Nov 3, 2023

ypujante commented Nov 3, 2023 • edited Loading

sbc100 commented Nov 4, 2023

ypujante commented Nov 4, 2023

ypujante commented Nov 6, 2023

ypujante commented Nov 6, 2023

ypujante commented Dec 1, 2023

juj commented Dec 4, 2023

juj Dec 4, 2023

Choose a reason for hiding this comment

ypujante Dec 4, 2023

Choose a reason for hiding this comment

juj commented Dec 4, 2023

ypujante commented Dec 5, 2023

ypujante commented Dec 5, 2023

ypujante commented Dec 5, 2023

ypujante commented Dec 6, 2023

sbc100 commented Dec 6, 2023

ypujante commented Dec 6, 2023

sbc100 commented Dec 6, 2023

ypujante commented Dec 9, 2023

ypujante commented Oct 31, 2023 •

edited

Loading

ypujante commented Nov 3, 2023 •

edited

Loading