Skip to content

Conversation

@waywardmonkeys
Copy link
Collaborator

  • Sanitize gradient stop positions with sanitize_stop_position to clamp NaNs, infinities, and out-of- range values into [0, 1].
  • Use sanitized positions in determine_lut_size and LUT ramp construction to ensure indices are finite, monotonic, and within bounds.
  • Keep the 4-wide write in GradientLut::new bounded by lut_size so very small LUTs cannot trigger end- index panics.
  • Add targeted tests to verify sanitization behavior and ensure GradientLut handles infinite and out-of-range stop offsets without panicking.

- Sanitize gradient stop positions with `sanitize_stop_position` to clamp NaNs, infinities, and out-of-
  range values into [0, 1].
- Use sanitized positions in `determine_lut_size` and LUT ramp construction to ensure indices are finite,
  monotonic, and within bounds.
- Keep the 4-wide write in `GradientLut::new` bounded by `lut_size` so very small LUTs cannot trigger end-
  index panics.
- Add targeted tests to verify sanitization behavior and ensure `GradientLut` handles infinite
  and out-of-range stop offsets without panicking.
@waywardmonkeys
Copy link
Collaborator Author

This should fix against the panic mentioned by @nicoburns in a comment in #1302.

Some notes / questions:

  • We should probably improve some handling of this with Peniko.
  • Callers should definitely do better.
  • We should probably document these invariants better?
  • Should I debug_assert instead of clamping / sanitizing to make callers fix their code? Or debug_assert, but still sanitize for non-debug-assertions builds?

Comment on lines +44 to +53
fn sanitize_stop_position(pos: f32) -> f32 {
if pos.is_nan() {
0.0
} else if pos.is_infinite() {
if pos.is_sign_negative() { 0.0 } else { 1.0 }
} else {
pos.clamp(0.0, 1.0)
}
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The following is a branchless way to do this.

Suggested change
fn sanitize_stop_position(pos: f32) -> f32 {
if pos.is_nan() {
0.0
} else if pos.is_infinite() {
if pos.is_sign_negative() { 0.0 } else { 1.0 }
} else {
pos.clamp(0.0, 1.0)
}
}
#[inline]
fn sanitize_stop_position(pos: f32) -> f32 {
pos.max(0.).min(1.)
}

f32:{max,min} don't propagate NaNs (so the NaN gets turned in to 0., if pos.max(0.) is first).

https://godbolt.org/z/nrzG75b1K

https://play.rust-lang.org/?version=stable&mode=debug&edition=2024&gist=3cd614c3df1900afdee1a4c481e5625a

Copy link
Contributor

@LaurenzV LaurenzV left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We already have a method that aims to validate the gradient before even encoding it:

fn validate(gradient: &Gradient) -> Result<(), Paint> {

So I feel like it might be better to just add this additional case here? Not much point in trying to fix a gradient that is fundamentally broken. 😄

@LaurenzV
Copy link
Contributor

LaurenzV commented Dec 1, 2025

I'm actually surprised this doesn't already work, because we already are checking that each stop is between 0 and 1...

@waywardmonkeys
Copy link
Collaborator Author

I will figure out why and fix it!

@waywardmonkeys
Copy link
Collaborator Author

(I already know why, just have to drive home)

@nicoburns nicoburns added the C-sparse-strips Applies to sparse strips variants of vello in general label Dec 3, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

C-sparse-strips Applies to sparse strips variants of vello in general

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants