fix for resizing HIDPI MacOS windows

[lpkruger]

Workaround for #15

The bug manifests in multi-monitor configurations mixing HiDPI and non-HiDPI monitors.

This fix is a little brute force - it reinitializes the SDL_Renderer context after each resize. But it seems to work in every combination of resizing, fullscreening, window dragging, etc I could think of.

Testing in an ASAN debugging build too just to check the initialization isn't leaking stuff.

[rom1v]

Thank you for your contribution. I agree scrcpy must have a workaround for this.

Sorry for the delay to review (I needed to find some time to read the context and what had been done before).

This fix is a little brute force

Yes, a little. Since it impacts all users (not only those mixing Hi-DPI and non-HiDPI monitors on MacOS), I'd like a minimal solution (which does not involve recreating the renderer and the texture).

There was #245 updating only the "logical size", and it seemed to work. Unfortunately, it was not ready to be merged (#245 (review) + comparing floats #245 (comment)). Since I cannot test by myself (I won't buy a Mac + a secondary screen just for that), this bug has been left aside. It's time to fix it :)

After reading #245 again and the source code of SDL_RenderSetLogicalSize(), I'm wondering if just calling this function would fix the problem:

diff --git a/app/src/scrcpy.c b/app/src/scrcpy.c
index defcb75..9dfe78f 100644
--- a/app/src/scrcpy.c
+++ b/app/src/scrcpy.c
@@ -144,8 +144,10 @@ handle_event(SDL_Event *event, bool control) {
             break;
         case SDL_WINDOWEVENT:
             switch (event->window.event) {
-                case SDL_WINDOWEVENT_EXPOSED:
                 case SDL_WINDOWEVENT_SIZE_CHANGED:
+                    screen_refresh_logical_size(&screen);
+                    // fall through
+                case SDL_WINDOWEVENT_EXPOSED:
                     screen_render(&screen);
                     break;
             }
diff --git a/app/src/screen.c b/app/src/screen.c
index e34bcf4..16504da 100644
--- a/app/src/screen.c
+++ b/app/src/screen.c
@@ -127,6 +127,14 @@ screen_init(struct screen *screen) {
     *screen = (struct screen) SCREEN_INITIALIZER;
 }
 
+void
+screen_refresh_logical_size(struct screen *screen) {
+    if (!SDL_RenderSetLogicalSize(screen->renderer, screen->frame_size.width,
+                                  screen->frame_size.height)) {
+        LOGW("Could not refresh logical size");
+    }
+}
+
 static inline SDL_Texture *
 create_texture(SDL_Renderer *renderer, struct size frame_size) {
     return SDL_CreateTexture(renderer, SDL_PIXELFORMAT_YV12,
diff --git a/app/src/screen.h b/app/src/screen.h
index bc18918..4fbc633 100644
--- a/app/src/screen.h
+++ b/app/src/screen.h
@@ -76,4 +76,9 @@ screen_resize_to_fit(struct screen *screen);
 void
 screen_resize_to_pixel_perfect(struct screen *screen);
 
+// workaround for SDL bug #15
+// <https://github.com/Genymobile/scrcpy/issues/15>
+void
+screen_refresh_logical_size(struct screen *screen);

Could you test please? If it does not, could you try to adapt your solution to update only the "logical size"?

Thank you for your help.

[lpkruger]

Hi @rom1v I tested with the SDL_RenderSetLogicalSize patch you posted and I'm sad to report that it has no effect on the underlying issue - after window resize the ratio between the SDL window size and renderer pixel size is not preserved. (the logical size is separate from the pixel size - the logical size is application defined whereas the pixel size is physically related to the window size and HIDPI status - in hidpi mode on MacOS the renderer pixel size is supposed to be double the window size - this is the ratio that gets broken during resizing and causes this bug).

I improved my version of this PR to be more precise and only reinitialize the renderer when absolutely necessary - in particular, in testing with single monitors the LOGW I left never prints and the renderer is never reinitialized.

[lpkruger]

I still think this is an upstream SDL bug btw . :) . I'm not confident enough to try to fix it there. :( . But there does not exist a SDL_SetRendererOutputSize call which would be necessary - that's supposed to be managed by the SDL drivers and it's query only with SDL_GetRendererOutputSize. SDL_SetLogicalOutputSize exists because applications are supposed to use that to set an application-specific useful size and let hardware GPUs take care of rescaling.

I'm not really an SDL expert, but I'm not sure if there's an actual proper fix short of fixing the upstream SDL code. Re-initializing the renderer when necessary does work though, I'm not sure if any other mitigations could help.

[rom1v]

I tested with the SDL_RenderSetLogicalSize patch you posted and I'm sad to report that it has no effect on the underlying issue - after window resize the ratio between the SDL window size and renderer pixel size is not preserved

Thank you for your feedback!

I improved my version of this PR to be more precise and only reinitialize the renderer when absolutely necessary

Thank you 👍 See comments inline :)

[rom1v]

This function returns a status (which needs to be checked). And in practice, for me, it never succeeds (it returns a non-zero value) :/
I think SDL_GL_GetDrawableSize is better for this (moreover, it may not fail).

[lpkruger]

Great, I'll test with the SDL_GL_GetDrawableSize call.
What do you think of the idea of calling either and storing the one that does not fail? I'm not sure what the other call will do if there is no GL renderer, but I'll test it.

[rom1v]

What do you think of the idea of calling either and storing the one that does not fail?
SDL_GL_GetDrawableSize() may not fail (it returns void).

[rom1v]

To avoid any rounding issues, we could store two "rationals" (a struct of two ints) scale_x = {renderer_w, window_w} and scale_y = {renderer_h, window_h} (in pseudo-code).

Then, when we need to compare:

bool scale_x_changed = new_scale_x.num * old_scale_x.den == old_scale_x.num * new_scale_x.den;
bool scale_y_changed = new_scale_y.num * old_scale_y.den == old_scale_y.num * new_scale_y.den;

[lpkruger]

👍

[rom1v]

Just for info, why is it necessary to check (and possibility recreate the renderer) when we switch fullscreen? (same question for resize_to_fit() and resize_to_pixel_perfect()

[lpkruger]

because the bug reproduces whether the window resize is caused by the user (by dragging the corner), but it also reproduces when it is resized by the system through a SDL_SetWindowSize call - and it is not picked up by the event in the event loop either. Regardless of the rest of the patch I think this small refactor is good on its own, it puts all the calls to SDL_SetWindowSize in a single code path.

[rom1v]

but it also reproduces when it is resized by the system through a SDL_SetWindowSize call - and it is not picked up by the event in the event loop either.

I don't understand well.

The problem does not occur only when moving the window from one monitor to another (which generates a SDL_WINDOWEVENT_SIZE_CHANGED event)?
If the user calls SDL_SetWindowSize, then the window should not be moved to another monitor?

[rom1v]

Regardless of the rest of the patch I think this small refactor is good on its own, it puts all the calls to SDL_SetWindowSize in a single code path.

The purpose of set_window_size() is to be able to change the windowed size even when fullscreen is enabled.

We don't want to resize the windowed size by a shortcut while in fullscreen (it's confusing).

[rom1v]

In order to be able to draw things above the video with coordinates relatives to the window (I have in mind OSD and "display local touches"), I will rework the screen to not use the "logical size" (so I will draw the video frame at the right position and compute mouse coordinates manually).

This will impact the way coordinates are handled, and will either automatically fix the issue, or will need to be adapted to handle the hidpi scale.

[rom1v]

I just pushed a quick&dirty PoC which implements the "logical size" manually: logical_size.2. Not everything is correctly handled, but mouse clicks should work.

Could you tell me if it works correctly (correctly rendered and clicks at the right position) when you move the window to your secondary screen, please?

[rom1v]

@lpkruger Could you please test logical_size.3? All sizes and location are computed manually, so maybe it makes the hidpi issue obsolete.

[lpkruger]

@rom1v I tested logical_size.3 - the behavior is kind of bizarre, especially when dragging from one monitor to the other.

dragging from hidpi to lowdpi monitor - the entire android screen appears in the upper left 1/4 of the computer window. The remaining 3/4 is black.
resize window on lowdpi monitor - the screen display becomes OK and the clicks are registered in the correct location.

dragging from lowdpi to hidpi - the android screen is "doubled" and only the top left 1/4 is visible in the computer window
resizing on hidpi monitor - the display becomes OK, but mouse events are registered at the wrong location (1/2 way to the upper left corner)

[rom1v]

Thank you for the feedbacks.

IIUC, moving from one monitor to another does not trigger the "window resized" event? (since resizing afterwards cause a different behavior)

[rom1v]

@lpkruger I re-read your feedbacks attentively, they are very precise and helpful 👍

Just a few questions:

resizing on hidpi monitor - the display becomes OK, but mouse events are registered at the wrong location (1/2 way to the upper left corner)

Is the behavior the same if you start directly on the hidpi monitor (on logical_size.3)?

the screen display becomes OK

Can you confirm that the window event SDL_WINDOWEVENT_SIZE_CHANGED is correctly triggered when you move from one monitor to another, please?

diff --git a/app/src/scrcpy.c b/app/src/scrcpy.c
index c2ed8bb..3abd379 100644
--- a/app/src/scrcpy.c
+++ b/app/src/scrcpy.c
@@ -145,9 +145,11 @@ handle_event(SDL_Event *event, bool control) {
         case SDL_WINDOWEVENT:
             switch (event->window.event) {
                 case SDL_WINDOWEVENT_EXPOSED:
+                    LOGD("EXPOSED");
                     screen_render(&screen);
                     break;
                 case SDL_WINDOWEVENT_SIZE_CHANGED:
+                    LOGD("SIZE_CHANGED");
                     screen_window_resized(&screen);
                     break;
             }

---

but mouse events are registered at the wrong location (1/2 way to the upper left corner)

This suggests that coordinates in mouse events are in screen coordinates (in contrast to pixel coordinates). But if this is really the case, since coordinates are integers, that would mean it is not possible to click on all pixels (if scaling is 2 in both directions, we would only be able to click on squares of 4 pixels). :(

[rom1v]

I will rework the screen to not use the "logical size"

For now, I let it aside. I will re-review the PR before v1.11, I'd like this problem to be fixed.

Just in case, in SDL, there is this commit: https://hg.libsdl.org/SDL/rev/46b094f7d20e

Does it solve the problem upstream? (EDIT: in fact, I think this is unrelated)

[rom1v]

I rebased (and squashed) your commits on dev, and applied the changes I suggested (after some preliminary changes on dev branch).

It calls screen_init_renderer_and_texture() on window resizing (which is supposed to be called when you move the window between non-hidpi and hidpi screens).

It's maybe not sufficient (you called it in other cases too, but it's not clear to me why), but I want to keep it minimal (and since I maintain it, I want to understand why every line is there 😉).

It's on fixhidpi.4. I'm waiting for your help and feedbacks :)

Thank you

[rom1v]

What do you think of the idea of calling either and storing the one that does not fail?
SDL_GL_GetDrawableSize() may not fail (it returns void).

[rom1v]

Regardless of the rest of the patch I think this small refactor is good on its own, it puts all the calls to SDL_SetWindowSize in a single code path.

The purpose of set_window_size() is to be able to change the windowed size even when fullscreen is enabled.

We don't want to resize the windowed size by a shortcut while in fullscreen (it's confusing).

[carstenhag]

I'm not super sure if I did everything right, but I checked out the fixhidpi.4 branch and built scrcpy according to https://github.com/Genymobile/scrcpy/blob/master/BUILD.md#common-steps.

I then ran scrcpy. With the original size everything worked fine, but after resizing it on my secondary monitor it is still broken like on the latest release.

So yeah, on the macbook display it works, but on the second monitor it's broken.

What I noticed was that, after moving it from the mac display to the second one, the touches work fine again.

Here's a screencap: https://drive.google.com/file/d/1vMxWxzTib2IlwUp5RdyHOm8BJGYs3vsz/view?usp=sharing

[rom1v]

Please test #1408, it should fix the issue.

[rom1v]

Fixed by #1408.