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.

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

[games] Fix crash in PCSX ReARMed with BIOS #14924

Merged
merged 1 commit into from Jan 28, 2019

Conversation

@KOPRajs
Copy link
Contributor

commented Nov 23, 2018

Description

Moves Streams().Initialize() back to InitializeGameplay() to solve the crash in PCSX ReARMed libretro core when run with BIOS. This is partial revert of commit 2bbd26f.

Motivation and Context

The issue was introduced as part of the PR #14146.

It is discussed here:
kodi-game/game.libretro.pcsx-rearmed#13

This needs more testing and approval from @garbear.

How Has This Been Tested?

LibreELEC on Amlogic MX2. So far 2 days without crash.

Screenshots (if appropriate):

Types of change

  • Bug fix (non-breaking change which fixes an issue)
  • Clean up (non-breaking change which removes non-working, unmaintained functionality)
  • Improvement (non-breaking change which improves existing functionality)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that will cause existing functionality to change)
  • Cosmetic change (non-breaking change that doesn't touch code)
  • None of the above (please explain below)

Checklist:

  • My code follows the Code Guidelines of this project
  • My change requires a change to the documentation, either Doxygen or wiki
  • I have updated the documentation accordingly
  • I have read the Contributing document
  • I have added tests to cover my change
  • All new and existing tests passed
@MartijnKaijser MartijnKaijser requested a review from garbear Nov 23, 2018
@garbear

This comment has been minimized.

Copy link
Member

commented Nov 24, 2018

Thanks for opening a PR. I'm a bit short on time but I'll dig into the code when I get a chance.

@garbear

This comment has been minimized.

Copy link
Member

commented Nov 25, 2018

Thanks for your patience. I think I found the root cause. PCSX enables hardware rendering during retro_init, which causes game.libretro to immediately open a stream: https://github.com/kodi-game/game.libretro/blob/master/src/video/VideoStream.cpp#L71

By not initializing Streams, we prevent a hardware stream from opening, so PCSX probably falls back into SW mode, which prevents crashing but has a performance hit. So this solution is not optimal.

Instead, when hardware framebuffer rendering is enabled, we should defer stream open until after the game is loaded. game.libretro can set a flag and open the stream after game load. Does that make sense?

@KOPRajs

This comment has been minimized.

Copy link
Contributor Author

commented Nov 26, 2018

How is this being affected by the presence of BIOS? It only crashes when there is a BIOS installed in the system folder.

Is there a way how to check if it is using software or hardware framebuffer? I'm testing this on Amlogic MX2 (old 32-bit dual core ARM) and I don't see any performance drop. More demanding titles are not running at full speed (e.g. NHL 99, Tekken 3, ...) but there is no visible difference between patched and not patched version.

@garbear

This comment has been minimized.

Copy link
Member

commented Nov 26, 2018

How is this being affected by the presence of BIOS? It only crashes when there is a BIOS installed in the system folder.

Can you step through PCSX in a debugger? This is how I catch bugs in libretro cores.

Is there a way how to check if it is using software or hardware framebuffer?

This is something that should be logged. Can you add a log line for this? That would at least rule out my theory.

If the crash is occurring in PCSX, not Kodi, it's because PCSX doesn't handle the order of libretro calls correctly. I've encountered several cores that make assumptions not present in the libretro API. The ideal solution would be to fix PCSX. The next best step would be to change game.libretro to guard against PCSX's bug (which is what I normally do). IMO modifying Kodi to work around a libretro core bug is the wrong approach.

I've identified a race condition that this PR could introduce. Streams must be initialized before load game takes place, because a core could deliver its first frame from inside load game (or spawn a thread from load game that races streams initialization). Load game shouldn't be called from Kodi with uninitialized streams.

What if we modify game.libretro to not open streams until load game is entered?

@KOPRajs

This comment has been minimized.

Copy link
Contributor Author

commented Nov 26, 2018

Lets get as many info as possible first. I'll try to debug and/or add more logs.

@KOPRajs

This comment has been minimized.

Copy link
Contributor Author

commented Dec 4, 2018

Long story short... I've added more logs and it seems that CVideoStream::EnableHardwareRendering is not being called at all in my case, sorry.

For reference:
Here is the patch: https://pastebin.com/tM3iKJSp
The crash log: https://pastebin.com/uhccHgwy
And the log with this PR applied: https://pastebin.com/bzHrZk4e

There are only "CVideoStream::GetSwFramebuffer" calls in both logs, no "CVideoStream::GetHwFramebuffer" or "Enabling hardware rendering".

P.S. I've just noticed this part in the log which is there every time it crashes:
DEBUG: RetroPlayer[AUDIO]: Initializing audio
ERROR: RetroPlayer[AUDIO]: Invalid samplerate: 0.000000
ERROR: GAME: Failed to open audio stream

It looks like the problem is in the initialization of audio stream not video.

@KOPRajs

This comment has been minimized.

Copy link
Contributor Author

commented Dec 4, 2018

Here is the same part of the log when you run the game with BIOS but after running it without the BIOS first so it doesn't crash: https://pastebin.com/r6niAYvW

@garbear

This comment has been minimized.

Copy link
Member

commented Dec 9, 2018

It looks like the problem is in the initialization of audio stream not video.

Interesting. Can you see why the audio initialization would be causing errors?

@KOPRajs

This comment has been minimized.

Copy link
Contributor Author

commented Jan 28, 2019

Anything new on this? I'm using my custom builds with this PR included for about two months now and playing NES, SNES, Genesis and PSX without a single crash so I've almost forgotten that it is not solved upstream. Today I've installed the official LibreELEC build on a new Rockchip device and again it crashes when playing with PCSX.

@KOPRajs

This comment has been minimized.

Copy link
Contributor Author

commented Jan 28, 2019

P.S. I've just noticed that it crashes even without the BIOS if you press "Reset" in the game OSD menu.

@garbear

This comment has been minimized.

Copy link
Member

commented Jan 28, 2019

It fixes the crash so this is good to go for v18. I'll look into this more closely for our next release.

@garbear garbear merged commit 943394b into xbmc:master Jan 28, 2019
1 check passed
1 check passed
default You're awesome. Have a cookie
Details
@MartijnKaijser MartijnKaijser added this to the Leia 18.1-rc1 milestone Jan 29, 2019
@Rechi Rechi added the v18 Leia label Jan 29, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.