Add Surface.premul_alpha_ip()

[itzpr3d4t0r]

Converting CPU surfaces is crucial to achieve optimal performance at blit time, but it comes at a cost, which generally means longer loading times in games.

Given that premultiplying implies having an alpha surface, we don't need to build a whole new Surface to premultiply that and discard the old one. We could just premultiply the alpha surface we have in place.

This PR implements just that by adding the premul_alpha_ip method. As expected this improves performance a lot (which in turn means shorter load times in games):

Basically now istead of doing this:

surf = pygame.image.load("mysurf.png").convert_alpha().premul_alpha()
                                            ^               ^
                                         new surf        new surf

You would do this:

surf = pygame.image.load("mysurf.png").convert_alpha().premul_alpha_ip()

I'm still unsure on returning the calling surface on the premul_alpha_ip method so we can chain methods or return just return None, so share your thoughts.

[MyreMylar]

OK LGTM 👍

[Starbuck5]

Is this consistent with anything else in the pygame-ce API?

I don't know that any surface permutation method right now has anything like this.

What about premul_alpha_ip instead? That way it's consistent with the Rect and Vector APIs at least.

Then what about convert and convert_alpha, do they need an in place parameter to be consistent with premul or would they also get an _ip variant?

Do you think that these methods should edit the surface itself by default? It seems more OOP to do so. Is there a migration path we could do for pygame-ce 3 that changes the semantics while keeping most old programs running perfectly?

[itzpr3d4t0r]

I don't know that any surface permutation method right now has anything like this.

Surface.convert has extra arguments that controls the return value (and behaviour).

What about premul_alpha_ip instead? That way it's consistent with the Rect and Vector APIs at least.

Didn't know about this, maybe it's best to keep new api to a minimum, and here an extra function seemed a bit too much.

Then what about convert and convert_alpha, do they need an in place parameter to be consistent with premul or would they also get an _ip variant?

convert alpha just uses an SDL function to convert, here we need to apply an operation to each pixel so the two are different

Do you think that these methods should edit the surface itself by default? It seems more OOP to do so. Is there a migration path we could do for pygame-ce 3 that changes the semantics while keeping most old programs running perfectly?

Nope I don't, that seems way out.

[oddbookworm]

I don't have a problem with this, so I'm approving it, but I'm soft requiring @Starbuck5 approval because he seems to have reservations about this API

[Starbuck5]

I do have a problem with this, I believe this doesn't fit into the design of the pygame-ce API, and would like to see some consideration to that end.

convert alpha just uses an SDL function to convert, here we need to apply an operation to each pixel so the two are different

We can make whatever semantics we want work internally.

[itzpr3d4t0r]

I do have a problem with this, I believe this doesn't fit into the design of the pygame-ce API, and would like to see some consideration to that end.

convert alpha just uses an SDL function to convert, here we need to apply an operation to each pixel so the two are different

We can make whatever semantics we want work internally.

Ok so about possibly making the other conversion api support an "in_place" argument too, I think it would only be possible if:

• The surface and format to convert to support the same number of channels (meaning the two surfaces have the same Bpp)
• What else possibly?

I've checked the SDL's implementation of the CPU conversion function and it accesses information that we can't have access to, meaning even if we make a custom conversion function we won't cover 100% of the conversion, probably more like 65-70% (like it's probably easy to convert the pixel format since it's just shuffling the pixels around but not so easy for everything else since we'd need to access private api and attributes).

So i think adding this functionality to all APIs isn't really viable. making an in_place version of the premul_alpha() function is more viable though.

[Starbuck5]

Ok so about possibly making the other conversion api support an "in_place" argument too, I think it would only be possible if:

But we can just change out the SDL surface that the pygame Surface is pointing to, right? That's how the display surface works. This is what I meant about doing anything internally.

One thing I've been thinking about is how un-object-oriented some of the pygame-ce objects are. Like to grayscale a surface, for example, you can't do Surface.grayscale(), you have to go to a whole separate module, call transform.grayscale, and get a new surface in return.

So then you come in with this PR, and I'm like maybe all the "surface permuting methods" in the actual Surface class should be in-place by default? The old semantic could be easily emulated with Surface.copy().convert | premul_alpha | convert_alpha (). I hear you that there's not much performance gain potential there, this is more about API ergonomics for me.

But maybe it just doesn't matter at all because people will call them chained on load and won't even notice whether it's a copy or an in place operation.

[Starbuck5]

Here's what I think would be a good path forward:

Put this in as premul_alpha_ip.
In pygame-ce 3, premul_alpha is removed, premul_alpha_ip is renamed to premul_alpha and replaces it

Advantages

• A function doesn't have different return semantics based on arg
• No performance loss based on having an arg
• Opt-in performance gain now
• Pygame-ce 3 API gets performance gains by default

Disadvantages:

• More functions
• premul_alpha_ip would be inconsistent with every single other surface manipulation I can think of

[ankith26]

My 2c, I actually prefer the kwarg approach (what was originally implemented) as opposed to the new method addition.

Why? premul_alpha is a pygame-ce function: we have no upstream compat restrictions here. Also, due to this fact, this function probably doesn't have a widespread usage like the classic API.

I think we need not worry too much about "similarity with vector and rect API", as long as we stay consistent within the same class. In the future if we can implement in place variants of convert and convert_alpha, we should take the same approach that we take for this case.

Here's what I propose. For now:

Surface.premul_alpha() raises a deprecation warning that says something along the lines of "Pass the keyword argument explicitly. For now the default value is in_place=False but in the future it is going to become in_place=True"

Surface.premul_alpha(in_place=True) and Surface.premul_alpha(in_place=False) will keep working now, and in the future. By suggesting the user to explicitly pass the keyword argument we are ensuring the user understands the difference, and makes the code more readable IMO

OFC, this introduces a tiny performance overhead of keyword argument handling

[Starbuck5]

Why? premul_alpha is a pygame-ce function: we have no upstream compat restrictions here. Also, due to this fact, this function probably doesn't have a widespread usage like the classic API.

I'm not worried about upstream compat restrictions with this.

I think we need not worry too much about "similarity with vector and rect API", as long as we stay consistent within the same class. In the future if we can implement in place variants of convert and convert_alpha, we should take the same approach that we take for this case.

Consistency within the same class is exactly what I'm worried about. In the future if we want in place variants of convert and convert_alpha they can use the same strategy as this.

@ankith26 Your proposal is very similar to what I was initially thinking, and what I wanted itzpr to think about. The reason I changed my mind is because I want our functions to have clear semantics, and having the entire semantic of a return differ based on an arg seems unnecessary to me.

Currently the 1 line description of premul_alpha is "returns a copy of the surface with the RGB channels pre-multiplied by the alpha channel."

In your proposal, how would that be written?

"returns a Surface with the RGB channels pre-multiplied by the channel and also might modify the original surface too have a good day" ?

Is "returns a Surface with the RGB channels pre-multiplied by the alpha channel" clear enough?

If there are two methods with their own clearly delineated semantics, I feel like that's better from a documentation standpoint.

* By semantics I mean like "use/functionality/side effects"

[itzpr3d4t0r]

In your proposal, how would that be written?

"returns a Surface with the RGB channels pre-multiplied by the channel and also might modify the original surface too have a good day" ?

Simply this:
If in_place is false (default), it returns a Surface with pre-multiplied RGB channels. If true, it pre-multiplies the RGB channels in place.

Btw blits has a dynamic return semantic already based on the doreturn argument as another example.

[MyreMylar]

I think this is fine as an new method with the _ip extension that we plan to deprecate in favour of a compat breaking .premul_alpha() in the future. In the end it doesn't matter hugely for a function that is unlikely to be widely used (it is relatively new and niche on top of that).

I guess it is possible that other transform or surface functions might benefit from a similar optimisation which might make it more important to figure out a permanent strategy - but I think it is likely that there are a lot larger gains possible there with SIMD techniques compared to this sort of thing.

[ankith26]

After Myre's review, the steering council could achieve a majority in support of premul_alpha_ip.

From my side, I'm happy to have reached a conclusion here and this PR in it's current state gets my approval now. Thanks again for the PR and bearing with our back and forth 😉

[Starbuck5]

Thanks for working on this @itzpr3d4t0r.