Emulation dialog in debugger minimizes window

[thrust26]

When you open the Option from inside the debugger. enter the Emulation dialog and press OK, Stella's size is minimized to ~100% zoom. Pressing escape returns to the (tiny) debugger window. Pressing escape again returns to the game (normal sized). When entering the debugger again, the window size is OK again.

https://atariage.com/forums/topic/307760-stella-62-released/?do=findComment&comment=4565986

[sa666666]

Also, entering the "Video & Audio" dialog from that same sequence causes Stella to segfault, at least in Linux. I think we haven't tested those dialogs from the debugger that much. TBH, I've never used the Options from the debugger.

EDIT: This is noted in the AtariAge report. It is a complete crash. I guess it's a good thing we delayed the 6.2.1 release.

[thrust26]

Fixed the "Audio & Video" crash with 0816f25ee.

[thrust26]

The problem is that FrameBuffer::setAvailableVidModes() fails when called from within the debugger. And then no zoom modes are created so that the display falls back to 1x zoom.

The call happens due to calling initializeVideo() in EmulationDialog::saveConfig(), which is required for setting VSYNC. Which is in this dialog, because it is directly related to emulation speed. Else it would be in the "Video & Audio" dialog.

[sa666666]

Hmm, it shouldn't matter, since setAvailableVidModes() should be checking the mode currently active and setting the correct size. So if the method is called from the debugger, it should be detecting that and doing the right thing. But obviously there's a bug there.

[sa666666]

Confirmed. If I put the following code at the end of that method:

cerr << "windowed:\n" << myWindowedModeList << endl;
cerr << "fullscreen:\n" << myFullscreenModeLists[0] << endl;

This is the following output:

windowed:
-----
image=0x0,320x240  screen=320x240  stretch=none  desc=  zoom=1  fsIndex= -1
-----

fullscreen:
-----
image=1760x960,320x240  screen=3840x2160  stretch=none  desc=  zoom=1  fsIndex= 0

So it's not properly detecting that the debugger is active, and hence not using the correct size. This isn't entirely surprising I guess, since that code wasn't originally designed to be called from the debugger. So some reworking will be needed here.

[sa666666]

Furthermore, the base width and height being passed in are 320x240, so this indicates the problem is also above this method. We need to trace backwards to see how it's called.

[sa666666]

Sorry for the flurry of emails. It's from EmulationDialog::saveConfig(), as you say:

  if(instance().hasConsole())
  {
    // update speed
    instance().console().initializeAudio();
    // update VSync
    instance().console().initializeVideo();

    instance().frameBuffer().tiaSurface().ntsc().enableThreading(myUseThreads->getState());
  }

Testing for the console being active is not enough. This is fine when you're actually in a menued mode (ie, where the TIA is underneath the menus). But in debugger mode, the console is available but we are not in the TIA mode. Hence the discrepancy.

[sa666666]

First, I would probably change the OSystem::hasConsole() method to only return true if a console actually exists (ie, the pointer is non-null), and also that EventHandler::state() is one that has the TIA as its base framebuffer (ie, neither ROM launcher nor debugger mode).

Second, in the code above, if hasConsole() returns false (which it will with the changes from in the previous paragraph), only changes the settings for the items in question, but doesn't actually re-initialize the audio and video. When the debugger exits, it will re-create the TIA window anyway, during which the settings will be picked up and used. And that makes sense, since changing any audio settings or vsync should never actually do anything in the debugger anyway (in the former, sound is already disabled, and in the latter it is irrelevant).

OK, off to bed now.

[thrust26]

I compared the code with similar code in VideoAudioDialog.cxx. It turned out that adding one line was sufficient to fix the bug.

Or would you prefer a more detailed fix? Using EventHandler::state() doesn't really help, I already explored the potential solutions there.

[sa666666]

I suppose if it works, then it's fine. But the underlying problem is that as we're not in TIA mode (or one closely related to it), we shouldn't be re-initializing at all, but simply turning on/off a setting, and letting it actually be applied later.

With the new code, it still creates a framebuffer/window for the Console, then creates another framebuffer/window for the debugger right after. It happens so fast that you don't see it, but I would argue that the window shouldn't be re-created at all in the debugger. That should only happen when you exit the debugger and let the console re-create the framebuffer.

[thrust26]

Agreed.

The whole video mode code is very convoluted. Maybe it is time to refactor it. And then we should think about separate surfaces too.

[sa666666]

Well, if you want to dive down that rabbit hole, I am willing to do it as well. The video code is some of the oldest parts of Stella, and is converted from software rendering, as was present before any UI code was added. Maybe something for 6.3 or 6.4?

[sa666666]

I guess we can close this one, and create another general issue for the video mode refactoring, if you think we should attempt it.

[thrust26]

Agreed.