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

Jump to bottom

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

Merged
merged 2 commits into from Jun 26, 2021
Merged

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

merged 2 commits into from Jun 26, 2021

Conversation

TrueBrain
Copy link
Member

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')

@TrueBrain TrueBrain mentioned this pull request Jun 17, 2021
Closed
rubidium42
rubidium42 reviewed Jun 19, 2021
View reviewed changes
Copy link
Contributor

@rubidium42 rubidium42 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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

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

@TrueBrain
Copy link
Member Author

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

@TrueBrain
Codechange: use _cur_palette the same in all the drivers
b900465 
It was a bit of a mixed bag. With this change, gfx.cpp is in
control who accesses _cur_palette from the video-drivers.
@TrueBrain
Change: prevent palette updates during copying to the video driver
606a624 
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 merged commit 0013673 into OpenTTD:master Jun 26, 2021
@TrueBrain TrueBrain deleted the palette-thread-safe branch June 26, 2021 18:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rubidium42 rubidium42 rubidium42 left review comments

@michicc michicc michicc approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@TrueBrain @michicc @rubidium42