Skip to content

pixels: Ensure expected formats when accesing bytes of snapshot#37767

Merged
sagudev merged 7 commits intoservo:mainfrom
sagudev:snapshot2
Jul 3, 2025
Merged

pixels: Ensure expected formats when accesing bytes of snapshot#37767
sagudev merged 7 commits intoservo:mainfrom
sagudev:snapshot2

Conversation

@sagudev
Copy link
Copy Markdown
Member

@sagudev sagudev commented Jun 28, 2025

I introduced snapshot in #36119 to pack raw bytes and metadata together, now we take the next step and require for user to always specify what kind of byte data they want when calling as_bytes or to_vec (basically joining transform and data). There are also valid usages when one might require just one property of bytes (textures can generally handle both RGBA and BGRA). There are also valid usages of using just raw bytes (when cropping). This PR tries to make such usages more obvious.

This will make it easier to fix stuff around 2d canvas (we do not want to assume any bytes properties in abstraction).

Testing: Code is covered by WPT tests.

@sagudev sagudev force-pushed the snapshot2 branch 2 times, most recently from ca10e34 to 5f21c65 Compare June 29, 2025 06:20
@sagudev sagudev marked this pull request as ready for review June 29, 2025 06:23
@sagudev sagudev requested a review from gterzian as a code owner June 29, 2025 06:23
Copy link
Copy Markdown
Member

@mrobinson mrobinson left a comment

Choose a reason for hiding this comment

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

Just a few nits:

@sagudev sagudev requested a review from mrobinson June 30, 2025 09:00
Copy link
Copy Markdown
Member

@wusyong wusyong left a comment

Choose a reason for hiding this comment

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

LGTM, but I think it will also affect #37718. I'll report the result later.

@sagudev
Copy link
Copy Markdown
Member Author

sagudev commented Jul 2, 2025

LGTM, but I think it will also affect #37718. I'll report the result later.

This change should not change any behavior. Just make existing expectations more explicit.

@wusyong
Copy link
Copy Markdown
Member

wusyong commented Jul 2, 2025

Yeah, it seems it's unaffected. Feel free to merge it!

@sagudev sagudev enabled auto-merge July 2, 2025 06:30
@sagudev
Copy link
Copy Markdown
Member Author

sagudev commented Jul 2, 2025

gentle nudge @mrobinson

@sagudev
Copy link
Copy Markdown
Member Author

sagudev commented Jul 3, 2025

rebased

Copy link
Copy Markdown
Member

@mrobinson mrobinson left a comment

Choose a reason for hiding this comment

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

Looks good, with two changes:

@sagudev sagudev added this pull request to the merge queue Jul 3, 2025
@sagudev sagudev removed this pull request from the merge queue due to a manual request Jul 3, 2025
sagudev and others added 2 commits July 3, 2025 16:26
Co-authored-by: Martin Robinson <[email protected]>
Signed-off-by: sagudev <[email protected]>
Signed-off-by: sagudev <[email protected]>
@sagudev sagudev enabled auto-merge July 3, 2025 14:27
@sagudev sagudev added this pull request to the merge queue Jul 3, 2025
Merged via the queue into servo:main with commit a631b42 Jul 3, 2025
21 checks passed
@sagudev sagudev deleted the snapshot2 branch July 3, 2025 15:31
@sagudev sagudev self-assigned this Aug 11, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants