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: MakeScreenshot could crash when called from outside of VideoDriver::Tick, and acquire video buffer before taking game state lock to prevent erratic fast forward #9155

Closed
wants to merge 1 commit into from

Conversation

Milek7
Copy link
Contributor

@Milek7 Milek7 commented May 1, 2021

Closes #9140, closes #9147.

Motivation / Problem

While there was nothing wrong with #9140 itself, it was discovered that MakeScreenshot locking is broken anyway, as described in #9147.

Description

This attempts to fix the issue by adding yield mutex for draw thread, recursive mutex for video buffer and slightly awry locking sequence in MakeScreenshot. Because order of these locking operations interacts with #9140 this cannot be easily split into two commits, so it is already integrated with previous PR.

Limitations

I'm not too happy about the complexity of this, maybe it is just better to disallow screenshots from rcon when real video driver is loaded...

Checklist for review

Some things are not automated, and forgotten often. This list is a reminder for the reviewers.

  • The bug fix is important enough to be backported? (label: 'backport requested')

@Milek7 Milek7 force-pushed the locking-madness branch 3 times, most recently from 93fdb9f to f645d96 Compare May 1, 2021 00:33
…er::Tick, and acquire video buffer before taking game state lock to prevent erratic fast forward
@TrueBrain
Copy link
Member

I remember looking into screenshot stuff before, but I cannot remember if my conclusion was correct. Either way, I think we need to simplify the situation rather than adding more locks and mutexes. From what I remember and see, we have two screenshot flows:

  • Large viewports, world, heightmap, minimap, etc. They all do not need a lock on anything (needs confirmation!). They should just be rendering to a local buffer and writing that to a file.
  • "Current viewport" (small) screenshot: this dumps the current buffer to disk.

If my conclusion is correct, it might be a lot cleaner to send a signal to the draw thread that on next tick it should make a screenshot of what is in the buffer just after it finishes rendering. This would mean this whole VideoBufferLocker can be removed completely, making everything a simpler world.

This kinda begs for a queue in the draw thread so the game thread can inform it of doing some actions, but this is mostly the moment michi swoops in with a much cleaner solution to do just this.

Either way, food for thought.

@Milek7
Copy link
Contributor Author

Milek7 commented May 1, 2021

  • Large viewports, world, heightmap, minimap, etc. They all do not need a lock on anything (needs confirmation!). They should just be rendering to a local buffer and writing that to a file.

Not really, while they don't need lock on the real video buffer, they poke in the _screen struct to reconfigure wanted viewport.

@LordAro LordAro added the backport requested This PR should be backport to current release (RC / stable) label May 1, 2021
@michicc
Copy link
Member

michicc commented May 1, 2021

This might fix one problem but it does not fix the real underlying problem (which you'll only see if persistent buffer mapping is disabled): Calling OpenGL on the game thread is currently a no-go.

@Milek7
Copy link
Contributor Author

Milek7 commented May 1, 2021

Oops. Adding context switching is easy enough, but I guess we want another approach anyway.

@Milek7 Milek7 closed this May 1, 2021
@LordAro LordAro removed the backport requested This PR should be backport to current release (RC / stable) label May 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

MakeScreenshot locking is broken when called from outside of VideoDriver::Tick
4 participants