Skip to content

Commit

Permalink
Fix Bug #23908 - Crash on resize with SDL2.
Browse files Browse the repository at this point in the history
SDL2 uses shared memory to communicate with the graphics system when
using a software renderer. A resize event will cause SDL2 to
invalidate the SDL_surface that relies on shared memory and any
subsequent calls to will be SDL_GetWindowSurface will cause the shared
memory do be unmapped as the old surface is released.  This is
problematic when it occurs during a rendering cycle, because the
software rendered may also invoke SDL_GetWindowSurface, making any
references to that surface that the game contains become stale. This
will in turn cause a segmentation fault during blitting.
  • Loading branch information
aginor committed Oct 12, 2015
1 parent 0c27eb9 commit 449aed0
Show file tree
Hide file tree
Showing 7 changed files with 65 additions and 3 deletions.
20 changes: 18 additions & 2 deletions src/events.cpp
Expand Up @@ -21,6 +21,7 @@
#include "sound.hpp"
#include "quit_confirmation.hpp"
#include "video.hpp"
#include "display.hpp"
#if defined _WIN32
#include "desktop/windows_tray_notification.hpp"
#endif
Expand Down Expand Up @@ -278,7 +279,9 @@ bool has_focus(const sdl_handler* hand, const SDL_Event* event)
void pump()
{
SDL_PumpEvents();

#if SDL_VERSION_ATLEAST(2,0,0)
peek_for_resize();
#endif
pump_info info;

//used to keep track of double click events
Expand Down Expand Up @@ -526,7 +529,7 @@ int pump_info::ticks(unsigned *refresh_counter, unsigned refresh_rate) {
#if SDL_VERSION_ATLEAST(2,0,0)

/* The constants for the minimum and maximum are picked from the headers. */
#define INPUT_MIN 0x200
#define INPUT_MIN 0x300
#define INPUT_MAX 0x8FF

bool is_input(const SDL_Event& event)
Expand All @@ -539,6 +542,19 @@ void discard_input()
SDL_FlushEvents(INPUT_MIN, INPUT_MAX);
}

void peek_for_resize()
{
SDL_Event events[100];
int num = SDL_PeepEvents(events, 100, SDL_PEEKEVENT, SDL_WINDOWEVENT, SDL_WINDOWEVENT);
for (int i = 0; i < num; i++) {
if (events[i].type == SDL_WINDOWEVENT &&
events[i].window.event == SDL_WINDOWEVENT_RESIZED) {
display::get_singleton()->video().update_framebuffer();

}
}
}

#else

#define INPUT_MASK (SDL_EVENTMASK(SDL_KEYDOWN)|\
Expand Down
5 changes: 5 additions & 0 deletions src/events.hpp
Expand Up @@ -93,6 +93,11 @@ struct event_context
//causes events to be dispatched to all handler objects.
void pump();

#if SDL_VERSION_ATLEAST(2,0,0)
//look for resize events and update references to the screen area
void peek_for_resize();
#endif

struct pump_info {
pump_info() : resize_dimensions(), ticks_(0) {}
std::pair<int,int> resize_dimensions;
Expand Down
10 changes: 10 additions & 0 deletions src/gui/widgets/grid.cpp
Expand Up @@ -21,6 +21,9 @@
#include "gui/auxiliary/layout_exception.hpp"
#include "gui/widgets/control.hpp"
#include "utils/foreach.tpp"
#if SDL_VERSION_ATLEAST(2,0,0)
#include "events.hpp"
#endif

#include <numeric>

Expand Down Expand Up @@ -918,6 +921,10 @@ void tgrid::impl_draw_children(surface& frame_buffer)
* the area not used for resizing the screen, this call `fixes' that.
*/
SDL_PumpEvents();
#if SDL_VERSION_ATLEAST(2,0,0)
events::peek_for_resize();
#endif


assert(get_visible() == twidget::tvisible::visible);
set_is_dirty(false);
Expand Down Expand Up @@ -956,6 +963,9 @@ tgrid::impl_draw_children(surface& frame_buffer, int x_offset, int y_offset)
* the area not used for resizing the screen, this call `fixes' that.
*/
SDL_PumpEvents();
#if SDL_VERSION_ATLEAST(2,0,0)
events::peek_for_resize();
#endif

assert(get_visible() == twidget::tvisible::visible);
set_is_dirty(false);
Expand Down
6 changes: 6 additions & 0 deletions src/hotkey/hotkey_preferences_display.cpp
Expand Up @@ -598,6 +598,9 @@ void hotkey_preferences_dialog::show_binding_dialog(
&& (event.type != SDL_MOUSEBUTTONDOWN )
)
SDL_PollEvent(&event);
#if SDL_VERSION_ATLEAST(2,0,0)
events::peek_for_resize();
#endif

do {
switch (event.type) {
Expand All @@ -610,6 +613,9 @@ void hotkey_preferences_dialog::show_binding_dialog(


SDL_PollEvent(&event);
#if SDL_VERSION_ATLEAST(2,0,0)
events::peek_for_resize();
#endif
disp_.flip();
disp_.delay(10);
} while (event.type != SDL_KEYUP && event.type != SDL_JOYBUTTONUP
Expand Down
7 changes: 7 additions & 0 deletions src/loadscreen.cpp
Expand Up @@ -27,6 +27,9 @@
#include "video.hpp"
#include "image.hpp"
#include "text.hpp"
#if SDL_VERSION_ATLEAST(2,0,0)
#include "display.hpp"
#endif

#include <SDL_events.h>
#include <SDL_image.h>
Expand Down Expand Up @@ -206,6 +209,10 @@ void loadscreen::draw_screen(const std::string &text)
SDL_Event ev;
while(SDL_PollEvent(&ev)) {
#if SDL_VERSION_ATLEAST(2,0,0)
if (ev.type == SDL_WINDOWEVENT &&
ev.window.type == SDL_WINDOWEVENT_RESIZED) {
display::get_singleton()->video().update_framebuffer();
}
if (ev.type == SDL_WINDOWEVENT &&
(ev.window.type == SDL_WINDOWEVENT_RESIZED ||
ev.window.type == SDL_WINDOWEVENT_EXPOSED))
Expand Down
15 changes: 14 additions & 1 deletion src/video.cpp
Expand Up @@ -532,6 +532,19 @@ int CVideo::modePossible( int x, int y, int bits_per_pixel, int flags, bool curr
}

#if SDL_VERSION_ATLEAST(2, 0, 0)

void CVideo::update_framebuffer()
{
if (!window)
return;

surface fb = SDL_GetWindowSurface(*window);
if (!frameBuffer)
frameBuffer = fb;
else
frameBuffer.assign(fb);
}

int CVideo::setMode( int x, int y, int bits_per_pixel, int flags )
{
update_rects.clear();
Expand All @@ -554,7 +567,7 @@ int CVideo::setMode( int x, int y, int bits_per_pixel, int flags )
}
}

frameBuffer = SDL_GetWindowSurface(*window);
update_framebuffer();

if(frameBuffer != NULL) {
image::set_pixel_format(frameBuffer->format);
Expand Down
5 changes: 5 additions & 0 deletions src/video.hpp
Expand Up @@ -75,6 +75,11 @@ class CVideo : private boost::noncopyable {
int modePossible( int x, int y, int bits_per_pixel, int flags, bool current_screen_optimal=false);
int setMode( int x, int y, int bits_per_pixel, int flags );

#if SDL_VERSION_ATLEAST(2, 0, 0)
//this needs to be invoked immediately after a resize event or the game will crash.
void update_framebuffer();
#endif

//did the mode change, since the last call to the modeChanged() method?
bool modeChanged();

Expand Down

2 comments on commit 449aed0

@Wedge009
Copy link
Member

Choose a reason for hiding this comment

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

If I understand this and the previous commit correctly, this is working around the unusual way in which Wesnoth updates the window surface and handles/process events. At a high level I think I understand the intention here, even though I probably don't have a full appreciation for all the effort that went into reaching this point.

I'll test this on returning from work. Thanks for getting this far!

@aginor
Copy link
Contributor Author

@aginor aginor commented on 449aed0 Oct 12, 2015

Choose a reason for hiding this comment

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

Thanks. You will probably encounter https://gna.org/bugs/index.php?23934 when you test, but it should not be crashing any longer.

Please sign in to comment.