-
-
Notifications
You must be signed in to change notification settings - Fork 157
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
Add SIMD versions of the greyscale transform (attempt #2) #2432
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
@MyreMylar Now that the prereq PR is this ready to leave draft status? |
I believe I still wanted to address some reviews that went up in the
previous attempt PR.
Will look that over tomorrow and get back to you.
…On Sat, 9 Sept 2023, 06:52 Charlie Hayden, ***@***.***> wrote:
@MyreMylar <https://github.com/MyreMylar> Now that the prereq PR is this
ready to leave draft status?
—
Reply to this email directly, view it on GitHub
<#2432 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ADGDGGUDCORIGNN32AD44ADXZP7Z3ANCNFSM6AAAAAA4HTW2NQ>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
OK, I've tried to address everything that was brought up in previous reviews now, so hopefully this is ready for fresh reviews if it passes on the CI tests. Thanks for the patience with this, |
In my testing: SSE2/NEON old one-pixel method: 6 times faster than non-SIMD greyscale. Used this test program: from sys import stdout
from pstats import Stats
from cProfile import Profile
import pygame
pygame.init()
def grayscale_test():
dimensions = [
(1, 1),
(3, 3),
(65, 65),
(126, 127),
(254, 255),
(1024, 1024),
(2047, 2048),
]
for iterations in range(0, 1000):
for w, h in dimensions:
src_surf = pygame.Surface((w, h), pygame.SRCALPHA, 32)
src_surf.fill((100, 255, 128, 200))
result = pygame.transform.grayscale(src_surf)
# res_color = result.get_at((0, 0))
# if res_color == pygame.Color(195, 195, 195, 200):
# pass
if __name__ == "__main__":
print("Grayscale")
profiler = Profile()
profiler.runcall(grayscale_test)
stats = Stats(profiler, stream=stdout)
stats.strip_dirs()
stats.sort_stats("cumulative")
stats.print_stats() |
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.
Pygame-ce w/ the SIMD be like 🚀
This looks good!
# Conflicts: # src_c/simd_transform.h # src_c/simd_transform_sse2.c
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.
LGTM!
Original PR was here: #2214
The last attempt ended up a bit of a mess after it got split into four different PRs, then there was an attempt to rebase and git seemed to get completely bamboozled.
This PR requires #2421 to function, and serves as a demonstration, or proof of concept for that PR. The intent being that we could add SIMD versions of many more of the transform functions making some of them more practical for 'real-time' (i.e. once per frame) full-screen usage in a game.
I'm thinking here of having a live desaturation effect to indicate health loss (or some other game state) across the whole screen by multiply blending a grey scale version of the current full-display frame with a greyscale version of it. Lots of fun post process type effects become possible without having to delve into setting up shaders if you can make the transform functions fast enough for this kind of real-time usage.
So the other obvious question is, does this work? I think last time I gave it a test it was about 12x faster for the AVX2 version on a medium sized image but I may try out exactly the sort of example I talk about above with a test program to see what sort of practical frame-rate difference we end up with.
I think there was some feedback last time on the SIMD code that I still need to respond to before it all got messed up, so I'll try and get to that as well. This will also remain a draft until #2421 gets itself merged.