pygame-ce
pygame-ce copied to clipboard
Add test & fix for unusual pitch in `surface.premul_alpha()`
fixes #2750
From what I understand, these unusual, non-pixel aligned, surface pitches won't practically come up on any modern desktop systems (possibly no modern systems) so I prioritised fixing it for the least performance sensitive versions of this function - sse2 & the non-SIMD fallback. These are also the versions I originally wrote so I had a better idea of how they were supposed to work.
The basic fix is to add in the standard 'skip' value that is used in all the blitters to handle pitch issues between two different surfaces - usually these are pixel aligned. In the SSE2 case, to deal with the .5 of a pixel overlap in the pitch case we have to cast the skip value down to Uint8 to get to 'channel', or single byte, level of pointer incrementing as we only want to skip 2 channels worth of a pixel (2 bytes) rather than a whole pixel (4 bytes).
I think that makes sense anyway.
This probably needs feedback from @itzpr3d4t0r and @Starbuck5 to see if they think what I've changed makes sense and if we need to do anything else here.
IG the same approach should probably be taken on all simd surface manipulations not just premul
๐ Walkthrough
Walkthrough
Adds pitch-aware row advancement to premultiply-alpha routines and tight-packing guards for AVX2. Updates SSE2 and non-SIMD paths to compute per-row skips from pitch and advance pointers accordingly. Adds a test verifying premultiplication correctness on surfaces with non-standard pitch.
Changes
| Cohort / File(s) | Summary |
|---|---|
Core premul alpha pathsrc_c/alphablit.c |
AVX2 fast path now requires tight packing (pitch == width ร bpp). Non-SIMD path computes src/dst row skips from pitch and advances pointers after each row to respect padding. No API changes. |
SIMD SSE2 premulsrc_c/simd_blitters_sse2.c |
Introduces srcskip/dstskip based on pitch; advances srcp/dstp per row using byte-wise arithmetic. Per-pixel logic unchanged. |
Tests for odd pitchestest/surface_test.py |
Adds test creating surfaces from buffers with custom pitch (e.g., 10 bytes for 2px width) and asserts correct premul_alpha results across all pixels. |
Sequence Diagram(s)
sequenceDiagram
autonumber
participant Py as Python caller
participant Premul as premul_alpha()
participant AVX2 as AVX2 path
participant SSE2 as SSE2 path
participant Scalar as Non-SIMD path
Py->>Premul: request premultiply (src, dst)
Premul->>Premul: Check format, dims, pitch
alt Tight-packed and AVX2 available
Premul->>AVX2: process with AVX2
note right of AVX2: Guard: pitch == width*bpp
AVX2-->>Premul: done
else AVX2 not used
alt SSE2 available
Premul->>SSE2: process row by row
note right of SSE2: Compute srcskip/dstskip = pitch - width*bpp<br/>Advance pointers by skips each row
SSE2-->>Premul: done
else
Premul->>Scalar: process row by row
note right of Scalar: Compute srcskip/dstskip and advance each row
Scalar-->>Premul: done
end
end
Premul-->>Py: result surface
Estimated code review effort
๐ฏ 3 (Moderate) | โฑ๏ธ ~20 minutes
Assessment against linked issues
| Objective | Addressed | Explanation |
|---|---|---|
| Respect non-standard surface pitches in premul_alpha, including unaligned pitch (e.g., 10 bytes for 2px width) [#2750] | โ |
Assessment against linked issues: Out-of-scope changes
None found.
[!TIP]
๐ Remote MCP (Model Context Protocol) integration is now available!
Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats.
โจ Finishing Touches
- [ ] ๐ Generate Docstrings
๐งช Generate unit tests
- [ ] Create PR with unit tests
- [ ] Post copyable unit tests in a comment
- [ ] Commit unit tests in branch
fix-premul-alpha-bytewidth
๐ชง Tips
Chat
There are 3 ways to chat with CodeRabbit:
- Review comments: Directly reply to a review comment made by CodeRabbit. Example:
I pushed a fix in commit <commit_id>, please review it.Open a follow-up GitHub issue for this discussion.
- Files and specific lines of code (under the "Files changed" tab): Tag
@coderabbitaiin a new review comment at the desired location with your query. - PR comments: Tag
@coderabbitaiin a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:@coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.@coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
Support
Need help? Create a ticket on our support page for assistance with any issues or questions.
CodeRabbit Commands (Invoked using PR/Issue comments)
Type @coderabbitai help to get the list of available commands.
Other keywords and placeholders
- Add
@coderabbitai ignoreanywhere in the PR description to prevent this PR from being reviewed. - Add
@coderabbitai summaryto generate the high-level summary at a specific location in the PR description. - Add
@coderabbitaianywhere in the PR title to generate the title automatically.
Status, Documentation and Community
- Visit our Status Page to check the current availability of CodeRabbit.
- Visit our Documentation for detailed information on how to use CodeRabbit.
- Join our Discord Community to get help, request features, and share feedback.
- Follow us on X/Twitter for updates and announcements.