Relax strict pixel match tests in test_src_alpha_sdl2_blitter by allowing a small delta #3494
Conversation
ankith26
changed the title
|
Formatting should hopefully be correct finally |
|
The source of this problem is very mysterious. I think it's SIMD related, but why? SIMD doesn't do approximation, it just does acceleration. I don't think the solution is to just add a delta to the test. I would prefer it to test that the result is EXACTLY X or Y, rather than testing whether it's within 4 of X. |
|
I am of the view that as long as something blitting related does not change much visually, it shouldn't be considered a break. |
|
Blitting should be 100% deterministic. Input A goes to output B, every time, 100% accurate. The fact that it isn't (on the SDL2 blitter) is extremely unsettling. I believe our current situation is that Input A goes to output B or output C, based on SDL version / architecture. Therefore I'd like to test the output against B and C, and only fail if it is neither. I don't want it to drift again without our noticing, and that's what a delta does. |
|
Alright I'll look into changing the implementation to use fixed outputs as specified by the original error but I will need someone else who has those architectures to test that it resolves the issue. |
Blitting should be deterministic for a given SDL version? yes. The exact same on all platforms across all SDL versions of the past and future? Probably too much to ask, even if desirable. SDL can make algorithmic changes, and since the exact algorithm is not a documented API detail, I don't see why they shouldn't be making tiny changes to it if it helps with performance, even though the exact pixel colours change a tiny bit. I can understand if there's a big change that has visual affect, it could be considered a breaking change. If I had to guess, I'd say this PR changed things libsdl-org/SDL#5430 (I tested on windows 2.24.1) but I'm not very sure. Atleast I didn't find anything else blit related that changed.
I'm saying we shouldn't care of minor drifts. I can't tell the difference between I also don't want to add the maintenance burden of having to keep updating our tests everytime SDL does a minor change like now. pygame users who resort to using the SDL blitter are the same target group of people who care about performance and not exact match with the pygame blitter. |
|
Created an implementation that checks if its either the normal outputs or the ones provided in the original issue, just need to come to a decision on which version would be a better choice now. Can view the differences here in the meantime https://github.com/Temmie3754/pygame/tree/surface-test-fixes. |
There was a problem hiding this comment.
I mentioned this on the discord too but I am generally in favour of this change for the SDL2 alpha blitter tests. This isn't the first time, or even the second time that the SDL formula has changed slightly - in fact these changes are the whole reason we ended up creating the default pygame alpha blitter. Ultimately these tests failing is outside of our control as we can't stop SDL slightly tweaking their alpha blitting formula.
For most users, they will never use the SDL2 alpha blitter as you have to go and seek it out and specifically flag to use it, and for those that do so a tiny colour discrepancy (likely less than 0.5%) between SDL versions on some platforms when alpha blitting - that is not normally visually noticeable will be unlikely to cause issues.
I do think we should create a second issue to update the docs for the BLEND_ALPHA_SDL2 in 2.1.4 after all the performance changes around the alpha blitters that are floating around have made it in. I suspect the original description of being 'sometimes faster' will be increasingly inaccurate by that point - especially if we have full AVX2 acceleration on the pygame alpha blitters. Then at that point we can also add a note in the docs that the SDL2 blitting formula differs between different versions of SDL.
|
I'll merge it in now, it's a non-risky change and as said before should better future-proof this test. If the need arises in the future, we can consider a stricter but more-maintenance-ey test then. |
Resolves the small issue in a test raised by #3097
Allows for a delta of up to 2 which resolves all errors presented in the original problem.