This adds another copy of the input image, which for medium to large images is not neglegible. Running the script below I get a peak memory usage of ~450 MB, but with this PR I get a peak usage of ~580MB (as expected the difference is ~one copy of the input image, which is ~120MB). The performance of resizing the window is also much worse, I presume because it's making a copy of the array every time the window size changes. < ignore this

I guess the other way to fix this is to multiply in-place, then divide in place after the resample. That would avoid the extra memory, but if it's constantly happening during a window resize I could imagine it has performace implications, and perhaps floating point errors could accumulate pretty fast with all the multiplications and divisions?

I can't think of another option to solve this without going back to applying alpha in resampled space (ie undoing #29776). Perhaps @anntzer has some thoughts as the original PR author?

import matplotlib.pyplot as plt
import numpy as np

np.random.seed(42)
rgba = np.random.random((2000, 2000, 4))

fig, ax = plt.subplots()
ax.imshow(rgba)
fig.canvas.draw()

The fix looks correct to me, sorry for introducing the bug.

re: performance:

• It's clear to me that unpremultiplied filtering is wrong because agg explicitly clips the RGB values to no more than alpha after the premultiplication (Filter images in premultiplied alpha mode. #29776 (comment)), which only makes sense in premultiplied space; if we really don't want to premultiply we should then at least remove that clipping.
• In an initial version I wrote the premultiplication by upcasting uint8 just to uint16 e52c150 for int input, which should save a lot of memory, but following review (Filter images in premultiplied alpha mode. #29776 (review)) I moved everything to float for simplicity. I could bring back these narrower types to save memory.
• It may be possible to save memory by merging the premultiplication step into the C++ filtering code; it's probably doable but a quick check suggests it's still a bit of work.

Thoughts?

I guess we can add "input was uint8" to my list of reasons not to copy, since we already cast to float.

In my opinion #29776 should remain. The previous results were incorrect, and I think having correct results should trump efficiency.

We copy the user data in _normalize_image_array and set that to self._A. It seems to me the proper thing to do it recopy the user data, not make another version of our self._A. I'm not sure how inefficient that is, but doing that at the correct level would make it so we only ever have one extra copy.

👍 to doing the right thing at the expense of memory - at a glance I think there might be ways to save creating extra memory for the alpha channel, but for simplicity I think worth getting this in as is and then optimizing (if possible) later.

It may be possible to save memory by merging the premultiplication step into the C++ filtering code; it's probably doable but a quick check suggests it's still a bit of work.

Either #29453, or some not-yet-pushed version of it, does attempt to do some of this deduplication. I don't have time to look at that again until I finish the text stuff, but it may yet be a possibility to reduce the memory footprint, which can be done after this PR at least fixes the bug.

That's fine as long as we remember to remove the extra copy this introduces.

I thought #29776 removed a copy (from _rgb_to_rgba), so we should be net even with this PR? At least for now.

Sure? But we still will want to remove this copy?

I just want to clarify the issue is that we are modifying our copy of the array not the object the user passed to us.

I just want to clarify the issue is that we are modifying our copy of the array not the object the user passed to us.

Yes, this is correct