Work around for multiple displays with respective scales #245
Conversation
This should resolve Genymobile#15
|
(just to confirm I saw the PR, I will try to review soon, thank you) |
There was a problem hiding this comment.
Thank you for your PR. 👍
In the past, I tested to scale down mouse events (hidpiscale), but while it fixed the problem on some Mac, it created the inverse problem on some others.
Changing the logical size is a promising idea, and may work for all cases (unfortunately, I have currently no way to test, I don't have access to the computers where I could reproduce anymore).
Based on this idea, I applied the change I suggested in comments, and propose this patch:
diff --git a/app/src/screen.c b/app/src/screen.c
index 5d7a400..1ae6c36 100644
--- a/app/src/screen.c
+++ b/app/src/screen.c
@@ -132,6 +132,17 @@ static inline struct size get_initial_optimal_size(struct size frame_size) {
return get_optimal_size(frame_size, frame_size);
}
+// apply hidpi scaling and call SDL_RenderSetLogicalSize
+static inline SDL_bool render_set_scaled_logical_size(struct screen *screen, struct size size) {
+ int w, h, dw, dh;
+ SDL_GL_GetDrawableSize(screen->window, &w, &h);
+ SDL_GetWindowSize(screen->window, &dw, &dh);
+ // use 32 bits unsigned not to lose precision (width and height fit in 16 bits)
+ int scaled_x = (Uint32) size.width * (Uint32) w / (Uint32) dw;
+ int scaled_y = (Uint32) size.height * (Uint32) h / (Uint32) dh;
+ return SDL_RenderSetLogicalSize(screen->renderer, scaled_x, scaled_y);
+}
+
void screen_init(struct screen *screen) {
*screen = (struct screen) SCREEN_INITIALIZER;
}
@@ -163,7 +174,7 @@ SDL_bool screen_init_rendering(struct screen *screen, const char *device_name, s
return SDL_FALSE;
}
- if (SDL_RenderSetLogicalSize(screen->renderer, frame_size.width, frame_size.height)) {
+ if (render_set_scaled_logical_size(screen, frame_size)) {
LOGE("Could not set renderer logical size: %s", SDL_GetError());
screen_destroy(screen);
return SDL_FALSE;
@@ -208,7 +219,7 @@ void screen_destroy(struct screen *screen) {
// recreate the texture and resize the window if the frame size has changed
static SDL_bool prepare_for_frame(struct screen *screen, struct size new_frame_size) {
if (screen->frame_size.width != new_frame_size.width || screen->frame_size.height != new_frame_size.height) {
- if (SDL_RenderSetLogicalSize(screen->renderer, new_frame_size.width, new_frame_size.height)) {
+ if (render_set_scaled_logical_size(screen, new_frame_size)) {
LOGE("Could not set renderer logical size: %s", SDL_GetError());
return SDL_FALSE;
}Could you confirm it works for you?
app/src/screen.h
Outdated
| @@ -66,4 +68,6 @@ void screen_resize_to_fit(struct screen *screen); | |||
| // resize window to 1:1 (pixel-perfect) | |||
| void screen_resize_to_pixel_perfect(struct screen *screen); | |||
|
|
|||
| void get_window_scale(SDL_Window *window, int *scale_x, int *scale_y); | |||
There was a problem hiding this comment.
The scale is not necessary an integer.
There was a problem hiding this comment.
I don't get any non-integral scale display device for now 😅.
So I will just turn the variables into float, but some tests on such devices are still necessary.
There was a problem hiding this comment.
There is no need to use floating point values here (the ratio will lose precision), you can use a "rational" instead (cf here)
There was a problem hiding this comment.
Do you mean use rational to denote the scale factor?
But we still have to convert it to float when performing the new logical size scale calculation.
There was a problem hiding this comment.
But we still have to convert it to float when performing the new logical size scale calculation.
No, we don't: the width and height fit into 16 bits, so using 32 bits, you can get the best possible precision using only integers:
2f6db25#diff-cbb13e05de08766971505804dff53448R139
app/src/screen.c
Outdated
| int output_w, output_h; | ||
| SDL_GL_GetDrawableSize(window, &output_w, &output_h); | ||
| *scale_x = output_w / win_w; | ||
| *scale_y = output_h / win_h; |
There was a problem hiding this comment.
This loses precision if the scale is not an integer.
| screen.scale_x = scale_x; | ||
| screen.scale_y = scale_y; | ||
| SDL_RenderSetLogicalSize(screen.renderer, logical_width, logical_height); | ||
| } |
There was a problem hiding this comment.
Oh, but I'm stupid, you need it there so that it changes when you moves the window from/to the secondary screen (it still needs be done in screen.c though).
|
I just checked on my setup and it works on both screens without the workaround 👍 |
|
@AoDevBlue Thank you for your feedback. And does the workaround break it? |
|
This branch works on both screens (vertical without hdpi and horizontal with hdpi) with both the default build config and the old workaround one |
|
Do you want me to test this on macOS? |
|
Hi @rom1v! I took some time to test out both patches for your since there hasn't been any activity on this issue. I've found @wsxyeah 's patch to work perfectly for me (see setup below), but unfortunately all touch events fail for your patch; I'm not able to use it on either monitor. There were a few build warnings but it seemed to have built properly with the default build instructions. Split-View also seems to work, but there is some bugginess with re-sizing the view: clicking the split partition immediately takes you out of split-view instead of allowing you to alter it. Without this patch, it works fine (event though the touch offsets is still a problem).
SetupMacBook Pro (15-inch, 2017)
|
There was a problem hiding this comment.
As reported by @jonalmeida (#245 (comment)), your patch works. Good job 👍
There are still some tiny things which I would like to be changed, cf my comments/questions inline.
The most important is that the scale must be taken into account for two events:
- the window is moved to another screen (your patch does this using exposed/size_changed events)
- the frame size changes (cf
prepare_for_frame())
| @@ -83,6 +83,9 @@ static SDL_bool event_loop(void) { | |||
| switch (event.window.event) { | |||
| case SDL_WINDOWEVENT_EXPOSED: | |||
| case SDL_WINDOWEVENT_SIZE_CHANGED: | |||
| if (!screen_update_logical_size_if_needed(&screen)) { | |||
There was a problem hiding this comment.
Which even is triggered when the window moves from one screen to another? SDL_WINDOW_EVENT_EXPOSED or SDL_WINDOWEVENT_SIZE_CHANGED?
On my computer (on Linux/XFCE), I get both events when I move the window from one screen to another if and only if I enable "put the windows side-by-side when moved to a screen border" (my own translation). However, otherwise, I get no event at all. So my question: is one of these events always triggered on switching screen (with different scales)? (it may be a good compromise if in practice it works, though).
There was a problem hiding this comment.
As tested on my devices(an external display without any scale and mbp's builtin retina display), when I moved window from one screen to another, SDL_WINDOWEVENT_SIZE_CHANGED always gets triggered.
And listening SDL_WINDOWEVENT_EXPOSED event seems unnecessary.
There was a problem hiding this comment.
IIRC, for rendering at least, SDL_WINDOWEVENT_EXPOSED was necessary when switching fullscreen on some platforms. I'm not sure it's necessary for detecting scaling changes.
| logical_height *= scale_y / screen->scale_y; | ||
| screen->scale_x = scale_x; | ||
| screen->scale_y = scale_y; | ||
| if (SDL_RenderSetLogicalSize(screen->renderer, logical_width, logical_height)) { |
There was a problem hiding this comment.
The logical size is rewritten on frame size changes (in prepare_for_frame), so it should take the scale into account here too.
| get_window_scale(screen->window, &scale_x, &scale_y); | ||
| if (scale_x != screen->scale_x || scale_y != screen->scale_y) { | ||
| int logical_width, logical_height; | ||
| SDL_RenderGetLogicalSize(screen->renderer, &logical_width, &logical_height); |
There was a problem hiding this comment.
Maybe I'm nitpicking, but I'm not fan of computing the new value based on the previous scaled rounded value. It should be computed from the non-scaled value IMO.
| SDL_bool screen_update_logical_size_if_needed(struct screen *screen) { | ||
| float scale_x, scale_y; | ||
| get_window_scale(screen->window, &scale_x, &scale_y); | ||
| if (scale_x != screen->scale_x || scale_y != screen->scale_y) { |
There was a problem hiding this comment.
Comparing float values for equality is fragile.
Since the "update" must also happen on frame size changes, maybe we could just set the logical size whenever SDL_WINDOW_EVENT_EXPOSED or SDL_WINDOWEVENT_SIZE_CHANGED (the one which works) or when the frame size changes? (without checking if the scale actually changed)
What do you think?
There was a problem hiding this comment.
I agree that comparing float values for equality is fragile.
Updating logical size when the frame size changes is ok, but updating on SDL_WINDOWEVENT_SIZE_CHANGED is unnecessary.
There was a problem hiding this comment.
OK, then we can keep the last width and height passed to SDL_RenderSetLogicalSize() (instead of the scale), and call it only when the parameters have changed.
|
@jonalmeida Thank you very much 👍
Great, good news 👍
In my patch, I did not updated the logical size on screen change (#245 (comment)). Maybe that if you add this additional patch, it may work (if there are no other mistakes): diff --git a/app/src/scrcpy.c b/app/src/scrcpy.c
index 54a7f99..2e2e911 100644
--- a/app/src/scrcpy.c
+++ b/app/src/scrcpy.c
@@ -88,6 +88,7 @@ static SDL_bool event_loop(void) {
switch (event.window.event) {
case SDL_WINDOWEVENT_EXPOSED:
case SDL_WINDOWEVENT_SIZE_CHANGED:
+ screen_update_scale(&screen);
screen_render(&screen);
break;
}
diff --git a/app/src/screen.c b/app/src/screen.c
index 1ae6c36..1666a78 100644
--- a/app/src/screen.c
+++ b/app/src/screen.c
@@ -313,3 +313,7 @@ void screen_resize_to_pixel_perfect(struct screen *screen) {
LOGD("Resized to pixel-perfect");
}
}
+
+void screen_update_scale(struct screen *screen) {
+ render_set_scaled_logical_size(screen, screen->frame_size);
+}
diff --git a/app/src/screen.h b/app/src/screen.h
index 13103ea..67dcb29 100644
--- a/app/src/screen.h
+++ b/app/src/screen.h
@@ -66,4 +66,8 @@ void screen_resize_to_fit(struct screen *screen);
// resize window to 1:1 (pixel-perfect)
void screen_resize_to_pixel_perfect(struct screen *screen);
+// recompute the scale in case the window moved from/to another screen with a
+// different HiDPI
+void screen_update_scale(struct screen *screen);
+
#endif(This is frustrating not to be able to reproduce/test.)
Thanks, fixed. eca99d5
I'm not sure to understand how the render size have an effect on the split-view behavior.
Arf, this is weird. 😞 |
|
@rom1v I'll apply this last patch on your branch and give you an update on Monday. Regarding the split-view, I'm not sure if that's something that can be solved easily without a reproducing environment for yourself. If I get a chance, I'll try to get some repro steps for you in a separate issue. Maybe someone else will have a better chance of fixing it then. 👍 |
|
@rom1v I've tried your branch Also, the release build was failing for lint warnings so I tested it using the debug variant instead: I also tested out your other branch Please ping me if there's anything else you'd like me to test out - I'd be happy to help! |
OK, thank you for your feedbacks. Instead of "stop working", I guess their coordinates are scaled. For instance, if you click at (100,200), it will generate a click at (200, 400) (or the reverse). (0,0) is at top-left. See gif in #15.
This warning is supposed to be fixed by fca806e. I'm interested in your value of
I cannot reproduce :(
That's weird, because Also, when you say "touch events", you mean "click events", right? Touch events may not work as expected (#22).
Thank you 😉 |
Sorry about not being clear. By stopped working, I meant the scaling issue still existed.
Correct, click events.
You're right! I closed my laptop screen, and then opened scrcpy on the external monitor where it works as expected. Re-opening the laptop screen put the scrcpy window back on the laptop screen and also confirms the actual clickable surface area as you can see in the screenshot below.
EDIT: Interestingly, after the image above happened, I moved the scrcpy window back to the external monitor and now it works on both! I'm guessing the issue is scaling up rather than scaling down? |
|
Interesting.
Which branch? |
Ideally, it would be nice to have a branch that has everything applied so that I can verify that I've got the right setup. |
OK, and this PR, it works in every cases? |
Correct. |
|
I'd be happy to help with testing, but have no means of building on my side. Is there a way to publish betas for brew? |
|
Does SDL 2.0.9 fix the issue on MacOS? There is a commit from Oct 12th: |
|
I don't believe it does. |
|
Really looking forward to the fix. When I test the application, I feel uncomfortable that scrcpy occupies the working space of the laptop, and not the second monitor. |
|
Hi, This PR has been left aside (partly my fault) and has conflicts with current master. A new PR attempts to fix the issue (#848). But I plan to avoid using the "logical size" at all, to compute correct mouse coordinates directly (if it works). For now, I have a branch, which works without hidpi, but still have problems with hidpi on secondary monitor: see #848 (comment). @wsxyeah @jonalmeida your help is welcome :) Thank you |
|
Please check #1408, it should fix the issue. |
It's just a workaround to resolve #15
What this pr did is to re-set a new logical size when the window scale factor got changed.
In consequence, the subsequent SDL events will get the coordinates what we wants,
which are consistent across all the displays even if there are different scales.