Skip to content

vello_hybrid: Read pixel pack buffer data into JS array first - #1814

Open
LaurenzV wants to merge 1 commit into
mainfrom
laurenz/readback
Open

vello_hybrid: Read pixel pack buffer data into JS array first#1814
LaurenzV wants to merge 1 commit into
mainfrom
laurenz/readback

Conversation

@LaurenzV

@LaurenzV LaurenzV commented Aug 12, 2026

Copy link
Copy Markdown
Collaborator

Safari 15 seems to have problems with the combination of PBO + WASM memory. Putting data into WASM memory directly works fine, copying memory from PBO to JS-allocated buffers also works fine, but copying from PBO to WASM-allocated memory results in a tab crash. This PR fixes this by first copying it into a JS-allocated array and then into the actual pixmap buffer.

out.mp4 
use std::{cell::RefCell, rc::Rc};
use wasm_bindgen::{JsCast, closure::Closure, prelude::wasm_bindgen};
use web_sys::{HtmlButtonElement, HtmlCanvasElement, WebGl2RenderingContext as Gl};

const N: i32 = 1;
const LEN: usize = 4;
type Callback = Rc<RefCell<Option<Closure<dyn FnMut()>>>>;

fn target() -> Gl {
    let document = web_sys::window().unwrap().document().unwrap();
    let canvas: HtmlCanvasElement = document
        .create_element("canvas")
        .unwrap()
        .dyn_into()
        .unwrap();
    canvas.set_width(N as u32);
    canvas.set_height(N as u32);
    let gl: Gl = canvas
        .get_context("webgl2")
        .unwrap()
        .unwrap()
        .dyn_into()
        .unwrap();
    gl.clear_color(0.0, 0.0, 1.0, 1.0);
    gl.clear(Gl::COLOR_BUFFER_BIT);
    gl
}

fn direct() {
    let gl = target();
    let mut bytes = vec![0; LEN];
    gl.read_pixels_with_opt_u8_array(0, 0, N, N, Gl::RGBA, Gl::UNSIGNED_BYTE, Some(&mut bytes))
        .unwrap();
    show(bytes[2] == 255);
}

fn pbo(js_memory: bool) {
    let gl = target();
    let buffer = gl.create_buffer().unwrap();
    gl.bind_buffer(Gl::PIXEL_PACK_BUFFER, Some(&buffer));
    gl.buffer_data_with_i32(Gl::PIXEL_PACK_BUFFER, LEN as i32, Gl::STREAM_READ);
    gl.read_pixels_with_i32(0, 0, N, N, Gl::RGBA, Gl::UNSIGNED_BYTE, 0)
        .unwrap();
    let sync = gl.fence_sync(Gl::SYNC_GPU_COMMANDS_COMPLETE, 0).unwrap();
    gl.flush();

    let callback: Callback = Rc::new(RefCell::new(None));
    let again = callback.clone();
    *callback.borrow_mut() = Some(Closure::new(move || {
        if gl.client_wait_sync_with_u32(&sync, 0, 0) == Gl::TIMEOUT_EXPIRED {
            frame(again.borrow().as_ref().unwrap());
            return;
        }
        gl.bind_buffer(Gl::PIXEL_PACK_BUFFER, Some(&buffer));
        if js_memory {
            let bytes = js_sys::Uint8Array::new_with_length(LEN as u32);
            gl.get_buffer_sub_data_with_i32_and_js_u8_array(Gl::PIXEL_PACK_BUFFER, 0, &bytes);
            show(bytes.get_index(2) == 255);
        } else {
            let mut bytes = vec![0; LEN];
            gl.get_buffer_sub_data_with_i32_and_u8_array(Gl::PIXEL_PACK_BUFFER, 0, &mut bytes);
            show(bytes[2] == 255);
        }
        again.borrow_mut().take();
    }));
    frame(callback.borrow().as_ref().unwrap());
}

fn frame(callback: &Closure<dyn FnMut()>) {
    web_sys::window()
        .unwrap()
        .request_animation_frame(callback.as_ref().unchecked_ref())
        .unwrap();
}

fn show(pass: bool) {
    web_sys::window()
        .unwrap()
        .document()
        .unwrap()
        .get_element_by_id("status")
        .unwrap()
        .set_text_content(Some(if pass { "PASS" } else { "FAIL" }));
}

fn button(id: &str, f: impl Fn() + 'static) {
    let button: HtmlButtonElement = web_sys::window()
        .unwrap()
        .document()
        .unwrap()
        .get_element_by_id(id)
        .unwrap()
        .dyn_into()
        .unwrap();
    let callback = Closure::<dyn FnMut()>::new(f);
    button.set_onclick(Some(callback.as_ref().unchecked_ref()));
    callback.forget();
}

#[wasm_bindgen(start)]
pub fn start() {
    button("direct", direct);
    button("js", || pbo(true));
    button("rust", || pbo(false));
}
<!doctype html>
<meta charset="utf-8">
<meta name="viewport" content="width=device-width,initial-scale=1">
<button id="direct">Direct → Rust/Wasm</button>
<button id="js">PBO → JS</button>
<button id="rust">PBO → Rust/Wasm</button>
<span id="status">READY</span>
<script type="module">import init from "./pkg/safari_wasm_memory_readback.js"; init();</script>

While it's a bit unfortunate to have two allocations now and another step of indirection, I haven't figured out a better approach to circumvent this. Just to be sure I also spot-checked some other devices to make sure nothing breaks, and they still seem to pass the probe normally. I also confirmed it fixes the probe in vello_bench2 while it crashes before this fix.

@LaurenzV
LaurenzV requested a review from grebmeg August 12, 2026 14:35
grebmeg
grebmeg approved these changes Aug 12, 2026
View reviewed changes
Comment thread sparse_strips/vello_hybrid/src/render/webgl/probe.rs
Comment on lines +244 to +248
// Safari 15 crashes the tab when attempting to read from a pixel pack buffer directly
// into WASM-allocated memory. Therefore, we first read it into a JS-allocated buffer and
// only then transfer it into the buffer backing the pixmap in WASM memory.
let readback =
js_sys::Uint8Array::new_with_length(u32::from(self.width) * u32::from(self.height) * 4);

@grebmeg grebmeg Aug 12, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This workaround makes sense. I found several related WebKit issues involving getBufferSubData on iOS 15, incorrect buffer offsets, and WebGL operations on WASM-backed views causing crashes. The performance impact should be negligible here, but this makes Safari 15’s limitation the common path for every browser. If a consumer already has a reliable browser-detection approach, would it be better to use this workaround only for affected Safari versions and retain the direct readback elsewhere?

@LaurenzV LaurenzV Aug 13, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

But how would you implement that? Letting the user pass the browser they are using? What if a user for example masks their browser? I agree it’s unfortunate :( but not sure if its worth special-casing.

@grebmeg grebmeg Aug 13, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

But how would you implement that? Letting the user pass the browser they are using? What if a user for example masks their browser? I agree it’s unfortunate :( but not sure if its worth special-casing.

I don't think passing the browser is a good idea. It would be better to simply enable/disable the feature and let consumers decide for themselves. That said, I don't think the current changes would have much negative impact either.

@LaurenzV LaurenzV Aug 13, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

So should I add a boolean flag? Or just leave it this way for now?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@grebmeg grebmeg grebmeg approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@LaurenzV @grebmeg