Commit 6187d3b5 authored by Stephen Anthony's avatar Stephen Anthony
Browse files

Attempt to fix continuous creation of dialog surfaces without cleanup, causing crashes on R77.

Remove cache of surfaces from FrameBuffer, into each dialog that owns it.
Make surfaces be unique_ptr instead of shared_ptr, so we can be sure cleanup occurs.
parent 0fa0a339
Pipeline #10435 passed with stages
in 4 minutes and 34 seconds
......@@ -71,6 +71,8 @@ FBBackendSDL2::~FBBackendSDL2()
myWindow = nullptr;
}
SDL_QuitSubSystem(SDL_INIT_VIDEO | SDL_INIT_TIMER);
cerr << "~FBBackendSDL2()" << endl;
}
// - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -
......
......@@ -40,6 +40,8 @@ namespace {
}
}
static int REF_COUNT = 0;
// - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -
FBSurfaceSDL2::FBSurfaceSDL2(FBBackendSDL2& backend,
uInt32 width, uInt32 height,
......@@ -48,6 +50,7 @@ FBSurfaceSDL2::FBSurfaceSDL2(FBBackendSDL2& backend,
: myBackend{backend},
myInterpolationMode{inter}
{
REF_COUNT++;
createSurface(width, height, staticData);
}
......@@ -58,6 +61,8 @@ FBSurfaceSDL2::~FBSurfaceSDL2()
if(mySurface)
{
REF_COUNT--;
cerr << " ~FBSurfaceSDL2(): " << this << " " << REF_COUNT << endl;
SDL_FreeSurface(mySurface);
mySurface = nullptr;
}
......@@ -200,17 +205,10 @@ void FBSurfaceSDL2::invalidateRect(uInt32 x, uInt32 y, uInt32 w, uInt32 h)
SDL_FillRect(mySurface, &tmp, 0);
}
// - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -
void FBSurfaceSDL2::free()
{
myBlitter.reset();
}
// - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -
void FBSurfaceSDL2::reload()
{
free();
reinitializeBlitter();
reinitializeBlitter(true);
}
// - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -
......@@ -221,8 +219,6 @@ void FBSurfaceSDL2::resize(uInt32 width, uInt32 height)
if(mySurface)
SDL_FreeSurface(mySurface);
free();
// NOTE: Currently, a resize changes a 'static' surface to 'streaming'
// No code currently does this, but we should at least check for it
if(myIsStatic)
......@@ -260,12 +256,15 @@ void FBSurfaceSDL2::createSurface(uInt32 width, uInt32 height,
if(myIsStatic)
SDL_memcpy(mySurface->pixels, data, mySurface->w * mySurface->h * 4);
reinitializeBlitter();
reload();
}
// - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -
void FBSurfaceSDL2::reinitializeBlitter()
void FBSurfaceSDL2::reinitializeBlitter(bool force)
{
if (force)
myBlitter.reset();
if (!myBlitter && myBackend.isInitialized())
myBlitter = BlitterFactory::createBlitter(
myBackend, scalingAlgorithm(myInterpolationMode));
......
......@@ -60,7 +60,6 @@ class FBSurfaceSDL2 : public FBSurface
void invalidate() override;
void invalidateRect(uInt32 x, uInt32 y, uInt32 w, uInt32 h) override;
void free() override;
void reload() override;
void resize(uInt32 width, uInt32 height) override;
......@@ -109,7 +108,7 @@ class FBSurfaceSDL2 : public FBSurface
void createSurface(uInt32 width, uInt32 height, const uInt32* data);
void reinitializeBlitter();
void reinitializeBlitter(bool force = false);
// Following constructors and assignment operators not supported
FBSurfaceSDL2() = delete;
......
......@@ -350,16 +350,8 @@ class FBSurface
*/
virtual void invalidateRect(uInt32 x, uInt32 y, uInt32 w, uInt32 h) = 0;
/**
This method should be called to free any resources being used by
the surface.
*/
virtual void free() = 0;
/**
This method should be called to reload the surface data/state.
It will normally be called after free().
*/
virtual void reload() = 0;
......
......@@ -71,7 +71,7 @@ FrameBuffer::~FrameBuffer()
// Most platforms are fine with doing this in either order, but it seems
// that OpenBSD in particular crashes when attempting to destroy textures
// *after* the renderer is already destroyed
freeSurfaces();
cerr << "~FrameBuffer()" << endl << endl;
}
// - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -
......@@ -551,6 +551,7 @@ void FrameBuffer::updateInEmulationMode(float framesPerSecond)
}
#ifdef GUI_SUPPORT
// - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -
void FrameBuffer::createMessage(const string& message, MessagePosition position, bool force)
{
// Only show messages if they've been enabled
......@@ -864,42 +865,61 @@ void FrameBuffer::setPauseDelay()
}
// - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -
shared_ptr<FBSurface> FrameBuffer::allocateSurface(
int w, int h, ScalingInterpolation inter, const uInt32* data
)
unique_ptr<FBSurface> FrameBuffer::allocateSurface(
int w, int h, ScalingInterpolation inter, const uInt32* data)
{
// Add new surface to the list
mySurfaceList.push_back(myBackend->createSurface(w, h, inter, data));
// And return a pointer to it (pointer should be treated read-only)
return mySurfaceList.at(mySurfaceList.size() - 1);
return myBackend->createSurface(w, h, inter, data);
}
// - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -
void FrameBuffer::freeSurfaces()
void FrameBuffer::resetSurfaces()
{
for(auto& s: mySurfaceList)
s->free();
}
switch(myOSystem.eventHandler().state())
{
case EventHandlerState::NONE:
case EventHandlerState::EMULATION:
case EventHandlerState::PAUSE:
case EventHandlerState::PLAYBACK:
myMsg.surface->reload();
myStatsMsg.surface->reload();
myTIASurface->resetSurfaces();
break;
// - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -
void FrameBuffer::reloadSurfaces()
{
for(auto& s: mySurfaceList)
s->reload();
}
#ifdef GUI_SUPPORT
case EventHandlerState::OPTIONSMENU:
myOSystem.menu().resetSurfaces();
break;
// - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -
void FrameBuffer::resetSurfaces()
{
// Free all resources for each surface, then reload them
// Due to possible timing and/or synchronization issues, all free()'s
// are done first, then all reload()'s
// Any derived FrameBuffer classes that call this method should be
// aware of these restrictions, and act accordingly
freeSurfaces();
reloadSurfaces();
case EventHandlerState::CMDMENU:
myOSystem.commandMenu().resetSurfaces();
break;
case EventHandlerState::HIGHSCORESMENU:
myOSystem.highscoresMenu().resetSurfaces();
break;
case EventHandlerState::MESSAGEMENU:
myOSystem.messageMenu().resetSurfaces();
break;
case EventHandlerState::TIMEMACHINE:
myOSystem.timeMachine().resetSurfaces();
break;
case EventHandlerState::LAUNCHER:
myOSystem.launcher().resetSurfaces();
break;
#endif
#ifdef DEBUGGER_SUPPORT
case EventHandlerState::DEBUGGER:
myOSystem.debugger().resetSurfaces();
break;
#endif
default:
break;
}
update(UpdateMode::REDRAW); // force full update
}
......
......@@ -158,7 +158,7 @@ class FrameBuffer
@return A pointer to a valid surface object, or nullptr
*/
shared_ptr<FBSurface> allocateSurface(
unique_ptr<FBSurface> allocateSurface(
int w,
int h,
ScalingInterpolation inter = ScalingInterpolation::none,
......@@ -384,16 +384,6 @@ class FrameBuffer
string getDisplayKey() const;
void saveCurrentWindowPosition() const;
/**
Calls 'free()' on all surfaces that the framebuffer knows about.
*/
void freeSurfaces();
/**
Calls 'reload()' on all surfaces that the framebuffer knows about.
*/
void reloadSurfaces();
/**
Frees and reloads all surfaces that the framebuffer knows about.
*/
......@@ -520,7 +510,7 @@ class FrameBuffer
int x{0}, y{0}, w{0}, h{0};
MessagePosition position{MessagePosition::BottomCenter};
ColorId color{kNone};
shared_ptr<FBSurface> surface;
unique_ptr<FBSurface> surface;
bool enabled{false};
bool dirty{false};
bool showGauge{false};
......@@ -541,9 +531,6 @@ class FrameBuffer
// Maximum TIA zoom level that can be used for this framebuffer
float myTIAMaxZoom{1.F};
// Holds a reference to all the surfaces that have been created
vector<shared_ptr<FBSurface>> mySurfaceList;
// Maximum message width [chars]
static constexpr int MESSAGE_WIDTH = 56;
// Maximum gauge bar width [chars]
......
......@@ -541,3 +541,12 @@ bool TIASurface::correctAspect() const
{
return myOSystem.settings().getBool("tia.correct_aspect");
}
// - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -
void TIASurface::resetSurfaces()
{
myTiaSurface->reload();
mySLineSurface->reload();
myBaseTiaSurface->reload();
myShadeSurface->reload();
}
......@@ -183,6 +183,11 @@ class TIASurface
*/
void updateSurfaceSettings();
/**
Issue a 'reload' to each surface.
*/
void resetSurfaces();
private:
/**
Average current calculated buffer's pixel with previous calculated buffer's pixel (50:50).
......@@ -208,7 +213,7 @@ class TIASurface
FrameBuffer& myFB;
TIA* myTIA{nullptr};
shared_ptr<FBSurface> myTiaSurface, mySLineSurface, myBaseTiaSurface, myShadeSurface;
unique_ptr<FBSurface> myTiaSurface, mySLineSurface, myBaseTiaSurface, myShadeSurface;
// NTSC object to use in TIA rendering mode
NTSCFilter myNTSCFilter;
......
......@@ -583,7 +583,6 @@ void ContextMenu::setArrows()
// - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -
void ContextMenu::drawDialog()
{
// Normally we add widgets and let Dialog::draw() take care of this
// logic. But for some reason, this Dialog was written differently
// by the ScummVM guys, so I'm not going to mess with it.
......
......@@ -87,6 +87,8 @@ Dialog::~Dialog()
_firstWidget = nullptr;
_buttonGroup.clear();
cerr << "\n~Dialog(): " << this << endl;
}
// - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -
......@@ -252,7 +254,7 @@ void Dialog::render()
// Extra surfaces must be rendered afterwards, so they are drawn on top
if(_surface->render())
{
mySurfaceStack.applyAll([](shared_ptr<FBSurface>& surface) {
mySurfaceStack.applyAll([](unique_ptr<FBSurface>& surface) {
surface->render();
});
}
......@@ -418,9 +420,10 @@ void Dialog::buildCurrentFocusList(int tabID)
}
// - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -
void Dialog::addSurface(const shared_ptr<FBSurface>& surface)
void Dialog::addSurface(const unique_ptr<FBSurface>& surface)
{
mySurfaceStack.push(surface);
// FIXME : add this to the stack somehow
// mySurfaceStack.push(surface);
}
// - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -
......
......@@ -90,7 +90,7 @@ class Dialog : public GuiObject
the surface render() call will always occur in such a case, the
surface should call setVisible() to enable/disable its output.
*/
void addSurface(const shared_ptr<FBSurface>& surface);
void addSurface(const unique_ptr<FBSurface>& surface);
void setTitle(const string& title);
bool hasTitle() { return !_title.empty(); }
......@@ -220,8 +220,6 @@ class Dialog : public GuiObject
int _layer{0};
unique_ptr<ToolTip> _toolTip;
Common::FixedStack<shared_ptr<FBSurface>> mySurfaceStack;
private:
struct Focus {
Widget* widget{nullptr};
......@@ -248,13 +246,15 @@ class Dialog : public GuiObject
TabFocusList _myTabList; // focus for each tab (if any)
WidgetArray _buttonGroup;
shared_ptr<FBSurface> _surface;
shared_ptr<FBSurface> _shadeSurface;
unique_ptr<FBSurface> _surface;
unique_ptr<FBSurface> _shadeSurface;
int _tabID{0};
uInt32 _max_w{0}; // maximum wanted width
uInt32 _max_h{0}; // maximum wanted height
Common::FixedStack<unique_ptr<FBSurface>> mySurfaceStack;
private:
// Following constructors and assignment operators not supported
Dialog() = delete;
......
......@@ -199,6 +199,14 @@ void DialogContainer::reStack()
reset();
}
// - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -
void DialogContainer::resetSurfaces()
{
myDialogStack.applyAll([&](Dialog*& d) {
d->surface().reload();
});
}
// - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -
void DialogContainer::handleTextEvent(char text)
{
......
......@@ -150,6 +150,13 @@ class DialogContainer
*/
void reStack();
/**
Issue a 'reload' event to each dialog surface in the stack. This
is typically used when interpolation or attributes for a dialog
have changed.
*/
void resetSurfaces();
/**
Inform the container that it should resize according to the current
screen dimensions. We make this virtual, since the container may or
......
......@@ -50,7 +50,7 @@ class RomInfoWidget : public Widget
private:
// Surface pointer holding the PNG image
shared_ptr<FBSurface> mySurface;
unique_ptr<FBSurface> mySurface;
// Whether the surface should be redrawn by drawWidget()
bool mySurfaceIsValid{false};
......
......@@ -47,15 +47,13 @@ void ToolTip::setFont(const GUI::Font& font)
myWidth = fontWidth * MAX_COLUMNS + myTextXOfs * 2;
myHeight = fontHeight * MAX_ROWS + myTextYOfs * 2;
// unallocate
if(mySurface != nullptr)
{
// TODO: unallocate
mySurface = nullptr;
}
mySurface.reset();
}
// - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -
shared_ptr<FBSurface> ToolTip::surface()
const unique_ptr<FBSurface>& ToolTip::surface()
{
if(mySurface == nullptr)
mySurface = myDialog.instance().frameBuffer().allocateSurface(myWidth, myHeight);
......
......@@ -76,7 +76,7 @@ class ToolTip
/**
Allocate surface if required and return it
*/
shared_ptr<FBSurface> surface();
const unique_ptr<FBSurface>& surface();
void show(const string& tip);
......@@ -100,7 +100,7 @@ class ToolTip
uInt32 myTextYOfs{0};
bool myTipShown{false};
uInt32 myScale{1};
shared_ptr<FBSurface> mySurface;
unique_ptr<FBSurface> mySurface;
};
#endif
......@@ -55,7 +55,6 @@ class FBSurfaceLIBRETRO : public FBSurface
bool render() override { return true; }
void invalidate() override { }
void invalidateRect(uInt32, uInt32, uInt32, uInt32) override { }
void free() override { }
void reload() override { }
void resize(uInt32 width, uInt32 height) override { }
void setScalingInterpolation(ScalingInterpolation) override { }
......
Markdown is supported
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment