pygame-ce icon indicating copy to clipboard operation
pygame-ce copied to clipboard

Add test & fix for unusual pitch in `surface.premul_alpha()`

Open MyreMylar opened this issue 1 year ago โ€ข 1 comments
trafficstars

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.

MyreMylar avatar May 28 '24 18:05 MyreMylar

IG the same approach should probably be taken on all simd surface manipulations not just premul

ankith26 avatar Jun 28 '24 11:06 ankith26

๐Ÿ“ 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 path
src_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 premul
src_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 pitches
test/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 @coderabbitai in a new review comment at the desired location with your query.
  • PR comments: Tag @coderabbitai in 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 ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere 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.

coderabbitai[bot] avatar Aug 26 '25 15:08 coderabbitai[bot]