fix(plugins): enable WebGL2 in servo so animated pages actually animate - #671
Conversation
Servo's dom_webgl2_enabled preference defaults to false, so
getContext('webgl2') returned null and pages requiring a WebGL2 context
(e.g. webgl2fundamentals.org's animated background) bailed out of their
render loop entirely, producing a static capture. Enable the preference
at Servo construction and add an integration regression test asserting
that a WebGL2 rAF page produces changing frames.
Signed-off-by: streamkit-devin <devin@streamkit.dev>
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
![staging-devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/870844?s=60&v=4){kind=small}
| // WebGL2 is off by default in Servo; without it pages that | ||
| // require a webgl2 context (getContext returns null) render | ||
| // as static content with no animation. | ||
| dom_webgl2_enabled: true, |
There was a problem hiding this comment.
📝 Info: Preference is only applied when the first servo node constructs the engine
dom_webgl2_enabled is set inside servo.get_or_insert_with(...), so like the User-Agent it is fixed by whichever node creates the process-global Servo first. That is fine for a hardcoded constant (every node gets the same value), but it means the flag cannot be made per-node/configurable later without restructuring engine construction. Worth noting in case a enable_webgl2 config knob is desired.
Was this helpful? React with 👍 or 👎 to provide feedback.
Debug
There was a problem hiding this comment.
Right — like the User-Agent, it's fixed by whichever node constructs the process-global Servo first. Since it's a hardcoded constant every node gets the same value, so this is fine; a per-node enable_webgl2 knob would need engine-construction restructuring (same bucket as the per-instance isolation work tracked in #639/#666).
| for _ in 0..MAX_TICKS { | ||
| let frame = render(id, &rx); | ||
| // Pre-paint frames are fully transparent; only compare painted ones. | ||
| if frame.chunks_exact(4).any(|px| px[3] >= 128) { | ||
| painted_frames += 1; | ||
| if last.as_deref().is_some_and(|prev| prev != frame) { | ||
| changes += 1; | ||
| } | ||
| last = Some(frame); | ||
| } | ||
| if changes >= REQUIRED_CHANGES { | ||
| break; | ||
| } | ||
| std::thread::sleep(Duration::from_millis(33)); | ||
| } |
There was a problem hiding this comment.
🔍 Test may be flaky if WebGL2 canvas contents are not preserved across readback
The regression test relies on the WebGL2 drawing buffer's cleared colour reaching the composited surface. The canvas is created without preserveDrawingBuffer, and the readback happens asynchronously via handle_render on a separate tick from the rAF callback. If Servo's software WebGL2 path ever composites a stale/blank buffer, painted_frames could stay 0 and the test would fail for reasons unrelated to the pref. Consider drawing a DOM/CSS-independent fallback marker or asserting with a longer budget if CI flakes are observed.
Was this helpful? React with 👍 or 👎 to provide feedback.
Debug
There was a problem hiding this comment.
Noted. In practice Servo's software WebGL path composites the cleared buffer reliably — the test converges in ~12 painted frames locally, well under the 600-tick budget, and even if the canvas ever composited blank the page background itself still paints (so painted_frames wouldn't stay 0; only changes would stall, which is exactly the regression signal we want). If CI ever flakes here we can add a DOM-side fallback marker, but I'd rather not weaken the WebGL2 assertion preemptively.
…integration tests Signed-off-by: streamkit-devin <devin@streamkit.dev>
| // Servo's embedded engine has no clean global-shutdown path: at normal | ||
| // process exit, mozjs' C++ static destructors race the still-live | ||
| // renderer thread and abort with a SIGSEGV, which would fail this | ||
| // otherwise-passing test. This is the only test in this binary, so | ||
| // exiting here — after all assertions have held — is safe and bypasses | ||
| // that teardown entirely. | ||
| println!("animated_webgl2_page_frames_change: PASSED"); | ||
| let _ = std::io::stdout().flush(); | ||
| // SAFETY: `_exit` is async-signal-safe and simply terminates the | ||
| // process with status 0 without running atexit handlers or C++ static | ||
| // destructors (the source of the teardown SIGSEGV above). | ||
| unsafe { libc::_exit(0) }; |
There was a problem hiding this comment.
📝 Info: Regression test terminates the whole test process on success
animation_repro.rs ends with libc::_exit(0) after its assertions. This is safe only because the binary contains a single test; adding any second #[test] to this file would silently skip it (or cause nondeterministic skipping under the default multi-threaded harness) since the process is killed as soon as this one passes. The comment documents the constraint, but it is an easy trap for future additions.
Was this helpful? React with 👍 or 👎 to provide feedback.
Debug
There was a problem hiding this comment.
Acknowledged — this follows the same single-test-per-binary convention as cross_session_leak.rs (which has the same libc::_exit(0) teardown workaround), and the doc comment states the constraint. Any future Servo integration test should go in its own binary for the same reason.
| for _ in 0..MAX_TICKS { | ||
| let frame = render(id, &rx); | ||
| // Pre-paint frames are fully transparent; only compare painted ones. | ||
| if frame.chunks_exact(4).any(|px| px[3] >= 128) { | ||
| painted_frames += 1; | ||
| if last.as_deref().is_some_and(|prev| prev != frame) { | ||
| changes += 1; | ||
| } | ||
| last = Some(frame); | ||
| } | ||
| if changes >= REQUIRED_CHANGES { | ||
| break; | ||
| } | ||
| std::thread::sleep(Duration::from_millis(33)); | ||
| } |
There was a problem hiding this comment.
📝 Info: Test can pass with a static page if frames differ for other reasons
The frame-change detection compares full RGBA buffers, so any nondeterministic pixel noise (antialiasing, scrollbar/caret blink, compositor artifacts) would also count as a "change" and let the test pass even with WebGL2 disabled. Conversely with a 33 ms sleep and 600 ticks, the ~20 s worst-case runtime plus real-Servo boot makes this a heavy test to run in CI; the doc comment acknowledges it is not part of the workspace cargo test.
Was this helpful? React with 👍 or 👎 to provide feedback.
Debug
There was a problem hiding this comment.
On false-passes: with WebGL2 disabled I measured 0–2 changed frames over 600 ticks on this exact setup (the software renderer is deterministic — no AA noise, no caret/scrollbar on this page), so REQUIRED_CHANGES = 10 has comfortable margin. On runtime: the loop breaks as soon as 10 changes are seen, which is ~1–2 s after first paint in practice; 600 ticks is only the worst-case budget for a slow box before failing.
|
Tested at b0ec455 (plugin 0.2.5) via the web-capture gateway (plugin rebuilt + skit restarted). Results
Live cast before/after + regression evidence
|
|
Follow-up verification of the reported static time.is capture, tested end-to-end via the local web-capture gateway on this branch (plugin 0.2.5). Result: not static.
Live cast before/after
|
2 participants
Summary
dom_webgl2_enabledpreference defaults to false, socanvas.getContext('webgl2')returns null and the page's script bails out before ever starting its render loop (webgl2fundamentals' background does exactlyconst gl = canvas.getContext("webgl2", ...); if (!gl) return;), leaving a static first paint.dom_webgl2_enabledin the process-global Servo preferences at engine construction.tests/animation_repro.rs): boots real Servo against adata:page that animates a WebGL2 canvas via rAF — deliberately with no WebGL1 fallback — and asserts successive frames differ. It fails if WebGL2 is ever disabled again or if frame production freezes for any other reason (readback caching, event-loop stalls, etc.).Verified against the live pages:
https://webgl2fundamentals.organd its/webgl/background.htmlnow produce continuously changing frames (previously 0 changed frames after first paint).Review & Validation
cargo test --test animation_repro -- --nocaptureinplugins/native/servo(needs llvmpipe; no GPU/display)cargo test --test cross_session_leakstill passes (per-instance surface isolation unaffected)Notes
read_painted_frame/last_good_frame, stale surface readbacks, missing repaint scheduling, and Page Visibility throttling — instrumented pages showed rAF loops advancing and frames changing in all those paths.Link to Devin session: https://staging.itsdev.in/sessions/f29bf33bf0184c92a23f81e695642b4a
Requested by: @streamer45
Devin Review
1bb65bb