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

Fix crash in zero-copy games if dimensions are changed #14194

Closed
wants to merge 1 commit into from

Conversation

garbear
Copy link
Member

@garbear garbear commented Jul 18, 2018

Buffers can only be released on the rendering thread because they access graphics resources.

Motivation and Context

Reported here: kodi-game/game.libretro.pcsx-rearmed#13

How Has This Been Tested?

Tested on OSX with PCSX.

Types of change

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

@garbear
Copy link
Member Author

garbear commented Jul 18, 2018

@MilhouseVH can you include this in your next build?

@MilhouseVH
Copy link
Contributor

Will do.

renderBuffer->SetHeader(header);
m_free.erase(it);

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

@GTechAlpha
Copy link
Contributor

GTechAlpha commented Jul 19, 2018

FYI - This is required for zero-copy to work on pcsx-rearmed.

EDIT: Also, I do not think this PR is necessary. Only free buffers which are no longer valid are released, so there should be no conflict with rendering. If you leave them in, the pool is "contaminated" with invalid buffers.

EDIT2: Links.

@garbear
Copy link
Member Author

garbear commented Jul 19, 2018

"contaminated" buffers are dropped on the next flush, so these shouldn't be a problem sitting around for a bit

@GTechAlpha
Copy link
Contributor

buffers are dropped on the next flush, so these shouldn't be a problem sitting around for a bit

Right. But we also miss out on the cool log message.

@garbear
Copy link
Member Author

garbear commented Jul 19, 2018

The problem is the free buffers are released on the game thread, which uses resources only available on the rendering.

Width and Height are passed on each frame, so instead of tearing everything down and reinitializing, what if we passed dimensions with the video data and stretched in real-time? Width and height would no longer be a state of the renderer, but of the buffer.

@GTechAlpha
Copy link
Contributor

Are we talking about fixing this issue or a redesign? Because this issue (kodi-game/game.libretro.pcsx-rearmed#13) is resolved by this: libretro/pcsx_rearmed#178

@garbear
Copy link
Member Author

garbear commented Jul 20, 2018

I never observed the crash fixed by libretro/pcsx_rearmed#178, probably because I recompiled the core from master within the last week. This PR fixes a crash I observed when dimensions change.

@GTechAlpha
Copy link
Contributor

This could be related: #14203

I have not experienced a crash since ^ + emu fix.

@garbear
Copy link
Member Author

garbear commented Jul 20, 2018

@GTechAlpha what about, for this PR, if we move free buffers of a different size to a "discard" vector that gets flushed on the rendering thread?

@GTechAlpha
Copy link
Contributor

@garbear

Invalid buffers should really never be requested in the first place. When a dimension change occurs, a buffer should not be requested until the flush and reconfigure have completed. #14203 should ensure this.

So between #14203 and this PR (as it is now), there should be no need for a special discard vector, IMO.

I was wondering before how a free buffer being released under lock protection could cause a crash. I now see that free buffers are not always free.

@garbear garbear added Type: Fix non-breaking change which fixes an issue v18 Leia Component: Games labels Jul 20, 2018
@garbear
Copy link
Member Author

garbear commented Jul 20, 2018

Closing in favor of #14203

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component: Games Type: Fix non-breaking change which fixes an issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants