Replace sparkle with glow in components/canvas#33918
Merged
sagudev merged 2 commits intoservo:mainfrom Nov 20, 2024
Merged
Conversation
sagudev
force-pushed
the
bye-sparkle-shared-canvas
branch
from
49d93c3 to
2d82205
Compare
Member
Author
|
I started working on components/canvas (it's the most complex and the last one) in this PR, but it will be landed as separate PR in the end. There is some more stuff missing in glow a lot is covered by grovesNL/glow#317. |
sagudev
force-pushed
the
bye-sparkle-shared-canvas
branch
from
2d82205 to
8da87f2
Compare
Member
Author
|
There are two types of todos:
|
|
🔨 Triggering try run (#11532818911) for Linux WPT |
|
Test results for linux-wpt-layout-2020 from try job (#11532818911): Flaky unexpected result (14)
Stable unexpected results that are known to be intermittent (12)
Stable unexpected results (5)
|
|
|
|
🔨 Triggering try run (#11534551078) for Linux WPT |
|
Test results for linux-wpt-layout-2020 from try job (#11534551078): Flaky unexpected result (13)
Stable unexpected results that are known to be intermittent (5)
Stable unexpected results (2)
|
|
|
sagudev
added
T-linux-wpt
github-actions
bot
removed
T-linux-wpt
|
🔨 Triggering try run (#11542244209) for Linux WPT, Windows |
|
Test results for linux-wpt-layout-2020 from try job (#11542244209): Flaky unexpected result (13)
Stable unexpected results that are known to be intermittent (9)
Stable unexpected results (2)
|
|
|
Member
Author
|
Interestingly we also failed this test with sparkle but differently (hence WPT failure): OK /_webgl/conformance2/renderbuffers/readbuffer.html I am not sure what causes difference tho. |
mrobinson
requested changes
Oct 29, 2024
Member
mrobinson
left a comment
mrobinson
left a comment
There was a problem hiding this comment.
Great work! I know this is waiting on some things, but here is an initial review.
| if err != 0 { | ||
| warn!("Error enabling GL program point size: {}", err); | ||
| unsafe { | ||
| // XXX: Do we even need to this? |
Member
There was a problem hiding this comment.
What's the minimum version of OpenGL that we support?
Member
Author
There was a problem hiding this comment.
IDK, I also asked this on zulip: https://servo.zulipchat.com/#narrow/channel/263398-general/topic/GL.2FGLES.20versions.20supported, as this would allow us to remove some old GLES/GL handling.
mrobinson
changed the title
sparkle with glow in components/shared/canvas
Member
Author
Actually this PR currently also contains changes to component/canvas that will be landed as separate PR (only first two commits will remain here). |
sagudev
force-pushed
the
bye-sparkle-shared-canvas
branch
from
5427702 to
0e3db54
Compare
Member
Author
diff --git a/components/canvas/webgl_thread.rs b/components/canvas/webgl_thread.rs
index e45567d8b4..990f7b0225 100644
--- a/components/canvas/webgl_thread.rs
+++ b/components/canvas/webgl_thread.rs
@@ -1206,7 +1206,11 @@ impl WebGLImpl {
let len = bytes_per_type(pixel_type) *
components_per_format(format) *
rect.size.area() as usize;
- let mut pixels = vec![0; len];
+ let mut pixels: Vec<u8> = Vec::new();
+ pixels.reserve(len);
+ unsafe {
+ pixels.set_len(len);
+ }
unsafe {
// We don't want any alignment padding on pixel rows.
gl.pixel_store_i32(glow::PACK_ALIGNMENT, 1);makes test pass like in sparkle, so new behavior is more correct (0,0,255,255 values were coming from previous allocation of pixels). This command also triggers Last GL operation failed warning (hence no data is written) but this should be solved separately. |
|
🔨 Triggering try run (#11710691640) for Linux WPT |
|
Test results for linux-wpt-layout-2020 from try job (#11710691640): Flaky unexpected result (19)
Stable unexpected results that are known to be intermittent (14)
Stable unexpected results (35)
|
|
|
|
🔨 Triggering try run (#11711514821) for Linux WPT |
|
Test results for linux-wpt-layout-2020 from try job (#11711514821): Flaky unexpected result (19)
Stable unexpected results that are known to be intermittent (10)
|
|
✨ Try run (#11711514821) succeeded. |
3 tasks
sagudev
changed the title
sparkle with glow in components/shared/canvassparkle with glow in components/canvas
sagudev
removed
the
S-blocked-on-external
Signed-off-by: sagudev <[email protected]>
sagudev
force-pushed
the
bye-sparkle-shared-canvas
branch
from
8d7a3bc to
d8a5610
Compare
Signed-off-by: sagudev <[email protected]>
|
🔨 Triggering try run (#11927810662) for Linux WPT |
|
Test results for linux-wpt-layout-2020 from try job (#11927810662): Flaky unexpected result (18)
Stable unexpected results that are known to be intermittent (10)
|
|
✨ Try run (#11927810662) succeeded. |
mrobinson
approved these changes
Nov 20, 2024
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Removes last usage of sparkle 🎊.
This is part of #33539.
./mach build -ddoes not report any errors./mach test-tidydoes not report any errors