vello_common: Store strip data in tile units

[LaurenzV]

Context

In the near future, we want to be able to support negative coordinates. This applies to two different areas:

• We want to support negative coordinates in strips, so that we can cache strips even if parts of it lie outside of the current viewport.
• We want to support negative coordinates in wide tiles during fine rasterization, so that pixels at the border with neighboring filters can sample pixels that are strictly outside.

In order to do this, we need to eventually migrate from using u16 to i16 in most places, as has been discussed in office hours.

Problem

Putting aside the complications that come from dealing with negative coordinates, a more immediate problem is that in the current implementation, this would reduce the maximum drawing area that can be used. Currently, we support a dimension of 65535x65535 (i.e. u16::MAX) in each direction, but after migrating to i16, this would be reduced to half of that, so around 32k pixels. While not dramatic, I do think it's unfortunate, especially because I think we can avoid this problem by doing some preparatory work. Therefore, before we start migrating to using i16, I would propose restructuring the current code in a way that will make the migration easier. This will also allow us to split it into multiple PRs that are easier to review.

Solution

Basically, in order to alleviate this problem, we can make use of the fact that our tiles always have a width/height of 4 (in the future, this might increase, but as long as we don't decrease the tile size this will not matter). Since this is true, inside of Strip, we can store the x/y coordinates in tile units instead of pixel coordinates, i.e. divided by 4. We actually already do this in Tile, but not in Strip yet. By doing so, even if we switch to using i16 later on, we can still store the maximum coordinates u16::MAX/4 inside of that, meaning that we can still represent the whole 65535x65535 pixel range (yay)!

PRs

Therefore, I am planning on opening the following PRs:

• Changing strips to store coordinates in tile units. <--- this PR
• Changing the strip clipping logic so that it won't overflow once we switch to i16.
• Changing all of the other methods in WideTile so that it won't overflow once we switch to i16.

While not strictly related, I noticed that we can do the same for alpha index: Instead of storing alpha_buf.len() as the offset, we can instead store alpha_buf.len() / (Tile::WIDTH * Tile::HEIGHT), since it's guaranteed that each tile always has 16 alpha values. So that could be a fourth PR.

Downside

The only downside that I can see is that if we go down this route, it will not be possible anymore to introduce the concept of footprints, since all of this is under the assumption that we are always properly aligned. But my understanding was that we weren't planning on re-introducing that, anyway.

[LaurenzV]

Thinking about it more closely, I think it would still be possible to support footprints. We could pack x and y into a single u32, but instead still store x in user units (to support footprints), but y coordinates in tile units. If we use 18 bits for x and only 14 bits for y, it still fits into a u32 but we can represent all the possible numbers for x and y!

So as far as I'm concerned, there shouldn't be any downsides to making this change.

[grebmeg]

Suggested change

	// TODO: Rewrite tests so we work with tile_x instead of x.
	// TODO: Rewrite tests so we work with tile_x instead of x.

[LaurenzV]

What's the suggestion here? 🤔 Or is it buggy on my screen?

[grebmeg]

Or it's just an empty line 😅

[grebmeg]

Suggested change

	// TODO: Ensure that all of the calculations below are compatible with i16.
	// TODO: Ensure that all of the calculations below are compatible with i16.

[LaurenzV]

Same as above.

[grebmeg]

This could overflow for very large tile_x. I think it’s highly unlikely in practice, but would it make sense to add an assertion when creating strip? and also use saturating_mul?

If we do this, we may be able to eliminate the branch? If I understand correctly, is_sentinel is used for clipping directly on strips.

pub fn x(&self) -> u16 {
    // Callers should check is_sentinel() before calling x() on sentinel strips
    debug_assert!(!self.is_sentinel(), "x() called on sentinel strip");
    self.tile_x * Tile::WIDTH
}

[LaurenzV]

I don't believe it should be possible to overflow, because we always do x / Tile::WIDTH, so a flooring division. But I agree using saturating makes sense here, just to be safe. I just thought having it explicitly might help us detect unexpected bugs.

I also had this debug assertion in the beginning, but it seems like some of the code relies on being able to call this for sentinel tiles, hence why I didn't add the debug assertion in the end.

[grebmeg]

I think it’s worth checking how much this conflicts with @b0nes164 ’s work on migrating to i16.

[b0nes164]

I think it’s worth checking how much this conflicts with @b0nes164 ’s work on migrating to i16.

We weren't able to get to it in discussion, but Laurenz has graciously taken the lead on the i16 changes. I'll still help where I can (starting with tile.rs, strip.rs, and strip_generator.rs), but to do that I need to get #1384, #1377, and #1368 done.

My existing changes on https://github.com/linebender/vello/tree/thomas/eyesixteen can be scrapped, as they are mostly exploratory. Notably, they also don't conform to the new spec Laurenz is using above, where the tile position is in "tile units."