Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix for resizing HIDPI MacOS windows #848

Closed
wants to merge 2 commits into from
+97 −29
Closed

fix for resizing HIDPI MacOS windows #848

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6b64d92
- fix for resizing MacOS windows when both HIDPI and non-HIDPI monito…
lpkruger Oct 7, 2019
f4a7757
test if renderer re-initialization is really necessary
lpkruger Oct 15, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 8 additions & 1 deletion app/src/scrcpy.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -144,8 +144,15 @@ handle_event(SDL_Event *event, bool control) {
break;
case SDL_WINDOWEVENT:
switch (event->window.event) {
case SDL_WINDOWEVENT_EXPOSED:
case SDL_WINDOWEVENT_SIZE_CHANGED:
#ifdef HIDPI_SUPPORT
if (!screen_test_correct_hidpi_ratio(&screen)) {
LOGW("Reinitializing renderer due to incorrect hidpi ratio");
screen_init_renderer_and_texture(&screen);
}
#endif
// fall-through no break
case SDL_WINDOWEVENT_EXPOSED:
screen_render(&screen);
break;
}
Expand Down
94 changes: 66 additions & 28 deletions app/src/screen.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,12 @@ set_window_size(struct screen *screen, struct size new_size) {
screen->windowed_window_size = new_size;
} else {
SDL_SetWindowSize(screen->window, new_size.width, new_size.height);
#ifdef HIDPI_SUPPORT
if (!screen_test_correct_hidpi_ratio(screen)) {
LOGW("Reinitializing renderer due to incorrect hidpi ratio");
screen_init_renderer_and_texture(screen);
}
#endif
}
}

Expand Down Expand Up @@ -134,6 +140,50 @@ create_texture(SDL_Renderer *renderer, struct size frame_size) {
frame_size.width, frame_size.height);
}

// This may be called more than once to work around SDL bugs
bool
screen_init_renderer_and_texture(struct screen *screen) {
if (screen->texture != NULL) {
SDL_DestroyTexture(screen->texture);
screen->texture = NULL;
}
if (screen->renderer != NULL) {
SDL_DestroyRenderer(screen->renderer);
screen->renderer = NULL;
}

screen->renderer = SDL_CreateRenderer(screen->window, -1,
SDL_RENDERER_ACCELERATED);
if (!screen->renderer) {
LOGC("Could not create renderer: %s", SDL_GetError());
screen_destroy(screen);
return false;
}

if (SDL_RenderSetLogicalSize(screen->renderer, screen->frame_size.width,
screen->frame_size.height)) {
LOGE("Could not set renderer logical size: %s", SDL_GetError());
screen_destroy(screen);
return false;
}

#ifdef HIDPI_SUPPORT
int window_w, window_h, renderer_w, renderer_h;
SDL_GetWindowSize(screen->window, &window_w, &window_h);
SDL_GetRendererOutputSize(screen->renderer, &renderer_w, &renderer_h);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

screen->expected_hidpi_w_factor = renderer_w * 1000 / window_w;
screen->expected_hidpi_h_factor = renderer_h * 1000 / window_h;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

#endif

screen->texture = create_texture(screen->renderer, screen->frame_size);
if (!screen->texture) {
LOGC("Could not create texture: %s", SDL_GetError());
screen_destroy(screen);
return false;
}
return true;
}

bool
screen_init_rendering(struct screen *screen, const char *window_title,
struct size frame_size, bool always_on_top) {
Expand Down Expand Up @@ -162,21 +212,6 @@ screen_init_rendering(struct screen *screen, const char *window_title,
return false;
}

screen->renderer = SDL_CreateRenderer(screen->window, -1,
SDL_RENDERER_ACCELERATED);
if (!screen->renderer) {
LOGC("Could not create renderer: %s", SDL_GetError());
screen_destroy(screen);
return false;
}

if (SDL_RenderSetLogicalSize(screen->renderer, frame_size.width,
frame_size.height)) {
LOGE("Could not set renderer logical size: %s", SDL_GetError());
screen_destroy(screen);
return false;
}

SDL_Surface *icon = read_xpm(icon_xpm);
if (icon) {
SDL_SetWindowIcon(screen->window, icon);
Expand All @@ -187,15 +222,21 @@ screen_init_rendering(struct screen *screen, const char *window_title,

LOGI("Initial texture: %" PRIu16 "x%" PRIu16, frame_size.width,
frame_size.height);
screen->texture = create_texture(screen->renderer, frame_size);
if (!screen->texture) {
LOGC("Could not create texture: %s", SDL_GetError());
screen_destroy(screen);
return false;
}
return screen_init_renderer_and_texture(screen);
}

return true;
#ifdef HIDPI_SUPPORT
bool
screen_test_correct_hidpi_ratio(struct screen *screen) {
int window_w, window_h, renderer_w, renderer_h;
SDL_GetWindowSize(screen->window, &window_w, &window_h);
SDL_GetRendererOutputSize(screen->renderer, &renderer_w, &renderer_h);
int current_hidpi_w_factor = renderer_w * 1000 / window_w;
int current_hidpi_h_factor = renderer_h * 1000 / window_h;
return current_hidpi_w_factor == screen->expected_hidpi_w_factor &&
current_hidpi_h_factor == screen->expected_hidpi_h_factor;
}
#endif

void
screen_show_window(struct screen *screen) {
Expand Down Expand Up @@ -300,8 +341,7 @@ screen_switch_fullscreen(struct screen *screen) {
screen->fullscreen = !screen->fullscreen;
if (!screen->fullscreen) {
// fullscreen disabled, restore expected windowed window size
SDL_SetWindowSize(screen->window, screen->windowed_window_size.width,
screen->windowed_window_size.height);
set_window_size(screen, screen->windowed_window_size);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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()

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

}

LOGD("Switched to %s mode", screen->fullscreen ? "fullscreen" : "windowed");
Expand All @@ -313,17 +353,15 @@ screen_resize_to_fit(struct screen *screen) {
if (!screen->fullscreen) {
struct size optimal_size = get_optimal_window_size(screen,
screen->frame_size);
SDL_SetWindowSize(screen->window, optimal_size.width,
optimal_size.height);
set_window_size(screen, optimal_size);
LOGD("Resized to optimal size");
}
}

void
screen_resize_to_pixel_perfect(struct screen *screen) {
if (!screen->fullscreen) {
SDL_SetWindowSize(screen->window, screen->frame_size.width,
screen->frame_size.height);
set_window_size(screen, screen->frame_size);
LOGD("Resized to pixel-perfect");
}
}
23 changes: 23 additions & 0 deletions app/src/screen.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,15 @@ struct screen {
bool has_frame;
bool fullscreen;
bool no_window;
#ifdef HIDPI_SUPPORT
// these values store the ratio between renderer pixel size and window size
// in most configurations these ratios would be 1.0 (1000), but on MacOS with
// a Retina/HIDPI monitor connected they are 2.0 (2000) because that's how
// Apple chose to maintain compatibility with legacy apps, by pretending that
// that the screen has half of its real resolution.
int expected_hidpi_w_factor; // multiplied by 1000 to avoid float
int expected_hidpi_h_factor; // multiplied by 1000 to avoid float
#endif
};

#define SCREEN_INITIALIZER { \
Expand Down Expand Up @@ -48,6 +57,20 @@ bool
screen_init_rendering(struct screen *screen, const char *window_title,
struct size frame_size, bool always_on_top);

#ifdef HIDPI_SUPPORT
// test if the expected renderer to window ratio is correct
// used to work around SDL bugs
// returns true if correct.
// If it returns false the renderer state needs to be fixed
bool
screen_test_correct_hidpi_ratio(struct screen *screen);
#endif

// reinitialize the renderer (only used in some configurations
// if necessary to workaround SDL bugs)
bool
screen_init_renderer_and_texture(struct screen *screen);

// show the window
void
screen_show_window(struct screen *screen);
Expand Down