Introduce snapshot concept of canvas#36119
Conversation
sagudev
added
A-gfx/uncategorized
A-content/webgl
sagudev
changed the title
sagudev
force-pushed
the
canvas-snapshot
branch
from
647b54e to
8f366e4
Compare
sagudev
force-pushed
the
canvas-snapshot
branch
2 times, most recently
from
441e7c8 to
c8f619e
Compare
|
I analyzed all stable unexpected failures https://github.com/sagudev/servo/actions/runs/14377247275/attempts/1#summary-40315242040: New PASSes are because we now properly handle RGBA<->BGRA and pre/un multiplying when uploading/downloading pixels. New FAILures were actually just false positives, this can be verified with "near" tests that also fail. |
sagudev
force-pushed
the
canvas-snapshot
branch
4 times, most recently
from
8a126f5 to
99418a4
Compare
|
I also experimented with typing inner alphamode and pixelformat, but decides it was not worth it as we must use untyped version most of the time anyway: sagudev@61d20f3 |
sagudev
force-pushed
the
canvas-snapshot
branch
from
99418a4 to
ecbe924
Compare
There was a problem hiding this comment.
LGTM overall, although I know very little about image formats so this may warrant the involvement of another reviewer?
Do I understand it correctly that the snapshot does not introduce any kind of concurrent logic, and is only used to delay running the conversion logic(on data that does not change or lose validity at some point) at the point of use?
I'm asking because to me the name "snapshot" seems to imply some sort of concurrent logic around the snapshot becoming out of date at some point in time, but that seems to be a wrong assumption on my part and the logic seems straight forward.
Hm, @jdm?
Yes, mayor goal is to actually avoid running conversion os much as possible.
I took the name from webgpu spec: https://gpuweb.github.io/gpuweb/#abstract-opdef-get-a-copy-of-the-image-contents-of-a-context and it represents snapshot of canvas image bitmap. I will write more docs. |
Signed-off-by: sagudev <[email protected]>
Signed-off-by: sagudev <[email protected]>
Signed-off-by: sagudev <[email protected]>
Signed-off-by: sagudev <[email protected]>
Signed-off-by: sagudev <[email protected]>
sagudev
force-pushed
the
canvas-snapshot
branch
from
1f34641 to
c3ee879
Compare
Signed-off-by: sagudev <[email protected]>
jdm
left a comment
jdm
left a comment
There was a problem hiding this comment.
Pretty sensible! Just a couple questions.
| }) | ||
| Snapshot::new( | ||
| canvas_size, | ||
| snapshot::PixelFormat::BGRA, |
There was a problem hiding this comment.
It's a bit weird to see the bgra8 format here when we call a function named rgba8_get_rect just above this.
There was a problem hiding this comment.
rgba8_get_rect just extracts subimage (pixels) of image regardless of pixel inner structure (pixel just needs to be 4B), if input image is BGRA it's result will also be BGRA and that's whats happening here.
Signed-off-by: sagudev <[email protected]>
sagudev
force-pushed
the
canvas-snapshot
branch
from
ca1256f to
3582997
Compare
Signed-off-by: sagudev <[email protected]>
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. --------- Signed-off-by: sagudev <[email protected]> Co-authored-by: Martin Robinson <[email protected]>
3 participants
Each canvas context returns snapshot instead of just raw bytes. This allows as to hold off conversions (BGRA <-> RGBA, (un)premultiply) to when/if they are actually needed. For example when loading snapshot into webgl we can load both RGBA and BGRA so no conversion is really needed.
Currently whole thing is designed to be able to be extend on servo/ipc-channel#356, to make less copies. Hence some commented out code.
Fixes #35759
There are tests for these changes in WPT