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

Use HasColorKey instead of GetColorKey in pgSurface_Blit #2835

Merged

Conversation

Starbuck5
Copy link
Member

@Starbuck5 Starbuck5 commented May 3, 2024

This routine doesn't use the value of GetColorKey, it just wants to know whether the surface has one.

Internally, this was setting a bunch of SDL errors ("surf doesn't have colorkey"-esque), which became much more expensive in SDL 2.29.3 because of a new logging feature.

Therefore lets replace it with HasColorKey (a newer function, the code was originally written before this existed). This fixes the performance regression here and even makes the performance slightly better than it was before, at least in my test scenario.

Credit to @itzpr3d4t0r for narrowing this down to SDL_GetColorKey

Relevant to #2821

This routine doesn't use the value of GetColorKey, it just wants to know whether the surface has one.

Internally, this was setting a bunch of SDL errors ("surf doesn't have colorkey"-esque), which became much more expensive in SDL 2.29.3 because of a new logging feature.

Therefore lets replace it with HasColorKey (a newer function, the code was originally written before this existed). This fixes the performance regression here and even makes the performance slightly better than it was before, at least in my test scenario.

Credit to itzpr for narrowing this down to SDL_GetColorKey
@Starbuck5 Starbuck5 requested a review from a team as a code owner May 3, 2024 06:24
@Starbuck5 Starbuck5 added Performance Related to the speed or resource usage of the project Surface pygame.Surface labels May 3, 2024
@Starbuck5
Copy link
Member Author

Test script:

import random
import time

import pygame

random.seed(36)

width = 600
height = 400

screen = pygame.Surface((width, height))

def make_particle():
    p_surf = pygame.Surface((5,5))
    p_surf.fill((random.randint(0,255), random.randint(0,255), random.randint(0,255)))

    return (p_surf, (random.randint(0, width), random.randint(0, height)))

particles = [make_particle() for _ in range(1000000)]

start = time.time()
screen.fblits(particles)
print(time.time() - start)

Results:

  • main: 1.13 seconds
  • this PR: 0.17 seconds
  • 2.4.1 (from pip): 0.27 seconds

@Starbuck5
Copy link
Member Author

An example of how the issue ( #2821 ) isn't fully fixed by this PR--

If you take the previous example and swap out the line defining p_surf for p_surf = pygame.Surface((5,5), pygame.SRCALPHA), to make it an alpha blit, there is still a performance problem.

On 2.4.1 it's 0.30 seconds, on main it's 1.60 seconds, and on this PR it's 0.63 seconds. So an improvement, but the SDL_GetColorKey in SoftBlitPyGame also needs to be re-evaluated

@Starbuck5 Starbuck5 added this to the 2.5.0 milestone May 3, 2024
Copy link
Member

@itzpr3d4t0r itzpr3d4t0r left a comment

Choose a reason for hiding this comment

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

Great! Tested this branch and compared it to 2.4.1's performance with fblits drawing 10 surfaces of increasingly bigger size up to 1000x1000 and got the following results:
Figure_1
A very good improvement for small (up to 200x200) surfaces!

@itzpr3d4t0r
Copy link
Member

itzpr3d4t0r commented May 3, 2024

An example of how the issue ( #2821 ) isn't fully fixed by this PR--
On 2.4.1 it's 0.30 seconds, on main it's 1.60 seconds, and on this PR it's 0.63 seconds. So an improvement, but the SDL_GetColorKey in SoftBlitPyGame also needs to be re-evaluated

About this, if SDL can do something about SDL_GetColorkey's perf we're good, otherwise could we implement our own function for that? I know SDL_Surface.map is private and it wouldn't be the best practice but what woud be the alternatives?

Copy link
Contributor

@robertpfeiffer robertpfeiffer left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@ankith26 ankith26 left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for the PR! 🎉

@ankith26 ankith26 merged commit 9b251f5 into pygame-community:main May 6, 2024
39 checks passed
@Starbuck5 Starbuck5 deleted the use-hascolorkey-in-pgsurface-blit branch May 8, 2024 06:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Performance Related to the speed or resource usage of the project Surface pygame.Surface
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants