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
Conversation
1f7a6a4
to
e627af8
Compare
There was a problem hiding this 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.
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
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 |
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 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 |
e627af8
to
3d45013
Compare
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.
3d45013
to
606a624
Compare
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:
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.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
_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.