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

Change: prevent palette updates during copying to the video driver #9379

Merged
merged 2 commits into from Jun 26, 2021

Conversation

@TrueBrain
Copy link
Member

@TrueBrain TrueBrain commented Jun 17, 2021

Motivation / Problem

CLang's ThreadSanitizer complained a lot about our way of doing palettes. Turns out, depending on the video driver, it is also a bit of a mess how it is used.

Description

This PR does two things:

  1. first, it fixes the discrepancy between the video-drivers. Although not strictly needed to use _local_palette for the ones that weren't using it, as the _cur_palette is only used in that function, having everything the same does make the next commit a lot easier.

  2. set a mutex-guard on _cur_palette access, so only one thread is accessing it at any given time.

This is a bit of an extreme solution in the sense that it possibly could be implemented in a more lightweight variant. count_dirty is used as event to notify the draw-thread a new palette is available. This means that guarding this would be sufficient, but it is also less clear that this is the case. So I didn't go for this solution, but it is something we can talk about.

Limitations

  • There are other places accessing _cur_palette, but they seem to all be done from the game-thread, which is all perfectly fine ofc.

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')
  • This PR affects the save game format? (label 'savegame upgrade')
  • This PR affects the GS/AI API? (label 'needs review: Script API')
    • ai_changelog.hpp, gs_changelog.hpp need updating.
    • The compatibility wrappers (compat_*.nut) need updating.
  • This PR affects the NewGRF API? (label 'needs review: NewGRF')
Copy link
Contributor

@rubidium42 rubidium42 left a comment

Though functionally the behaviour is the same, you add a static Palette _local_palette to some drivers and some drivers have a class variable for it. Might it not make sense to make Palette local_palette a variable of the video driver class, making everything even more consistent? That way any new driver will have the local palette making it more likely that someone doesn't access _cur_palette.
You could consider making the CopyPalette a function of the video driver, so it becomes the "problem" of the (generic) video driver to fill the local palette in a safe manner.
Although, alternatively screenshot could just copy the palette as well, and just remove _cur_palette from gfx_func.h all together. That would make it harder for new video drivers to access it; they would simply fail to compile, instead of function but be thread unsafe.

Loading

src/gfx.cpp Outdated Show resolved Hide resolved
Loading
@TrueBrain
Copy link
Member Author

@TrueBrain TrueBrain commented Jun 19, 2021

You could consider making the CopyPalette a function of the video driver, so it becomes the "problem" of the (generic) video driver to fill the local palette in a safe manner.

I rather have the functions locally next to each other, so it is a lot easier to spot what functions are cooperating in the lock. Moving CoypPalette would be a lot of distance between those 2 functions, meaning another global mutex, etc etc. The code doesn't really get better because of it ;)
The problem in reality is more that Palette should be a class with a Copy function, etc etc, but that is a rather large code-refactor .. not for now :D

Although, alternatively screenshot could just copy the palette as well, and just remove _cur_palette from gfx_func.h all together. That would make it harder for new video drivers to access it; they would simply fail to compile, instead of function but be thread unsafe.

That would be future work, and has to be considered carefully. There is no real reason to do this, other than to avoid someone accessing _cur_palette from the video driver. But the biggest problem here is that 1 blitter also accesses the _cur_palette which would also need to be addressed in that case. So this would involve a bit more work :D

Loading

@TrueBrain
Copy link
Member Author

@TrueBrain TrueBrain commented Jun 19, 2021

Though functionally the behaviour is the same, you add a static Palette _local_palette to some drivers and some drivers have a class variable for it. Might it not make sense to make Palette local_palette a variable of the video driver class, making everything even more consistent? That way any new driver will have the local palette making it more likely that someone doesn't access _cur_palette.

After a bit of fiddling with this: this is not as simple as it looks on the surface. Although michi and I did clean up the win32, cocoa and sdl2 driver, we didn't do allegro and sdl1 (yet?). And guess what? THEY ARE UGLY :P For both I have to refactor the video driver quiet a bit. It starts with 1 function that is static while accessing the palette .. and that turns into changing a lot of other functions to static. But that also means some of the other things done in the other videodrivers should be done, otherwise future-us will hurt even more, and ... down the rabbit hole we go. So I pressed the panic button and resurfaced :P

So no, I am not going to do this for this PR if you don't mind, and leave it as it is. It is still a good idea, but requires more work than fits in this PR scope :D

Loading

@TrueBrain TrueBrain force-pushed the palette-thread-safe branch from e627af8 to 3d45013 Jun 19, 2021
TrueBrain added 2 commits Jun 26, 2021
It was a bit of a mixed bag. With this change, gfx.cpp is in
control who accesses _cur_palette from the video-drivers.
ThreadSanitizer rightfully notices that the game-thread could
update the palette while the draw-thread is copying it for local
use. The odds of this are very small, but nevertheless, it does
carry a very good point.

It wouldn't hurt the application in any way, but it might cause
visual glitches on the screen.
@TrueBrain TrueBrain force-pushed the palette-thread-safe branch from 3d45013 to 606a624 Jun 26, 2021
@TrueBrain TrueBrain merged commit 0013673 into OpenTTD:master Jun 26, 2021
15 checks passed
Loading
@TrueBrain TrueBrain deleted the palette-thread-safe branch Jun 26, 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
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants