From b265540f4b1fbdfa05ee69b61781afd90b957648 Mon Sep 17 00:00:00 2001 From: Kevin Reid Date: Mon, 26 Sep 2022 08:42:10 -0700 Subject: [PATCH 1/2] Low-level error handling in canvas context creation (for WebGPU and WebGL 2). MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Part of fixing #3017. This commit changes the handling of `web_sys` errors and nulls from `expect()` to returning `Err`, but it doesn't actually affect the public API — that will be done in the next commit. --- wgpu-hal/src/gles/web.rs | 61 +++++++++++++++++++++++++++------------- wgpu/src/backend/web.rs | 48 +++++++++++++++++++++++++------ 2 files changed, 80 insertions(+), 29 deletions(-) diff --git a/wgpu-hal/src/gles/web.rs b/wgpu-hal/src/gles/web.rs index b9e7302182..091c494ddc 100644 --- a/wgpu-hal/src/gles/web.rs +++ b/wgpu-hal/src/gles/web.rs @@ -33,32 +33,53 @@ impl Instance { &self, canvas: &web_sys::HtmlCanvasElement, ) -> Result { - let webgl2_context = canvas - .get_context_with_context_options("webgl2", &Self::create_context_options()) - .expect("Cannot create WebGL2 context") - .and_then(|context| context.dyn_into::().ok()) - .expect("Cannot convert into WebGL2 context"); - - *self.webgl2_context.lock() = Some(webgl2_context.clone()); - - Ok(Surface { - webgl2_context, - srgb_present_program: None, - swapchain: None, - texture: None, - presentable: true, - }) + self.create_surface_from_context( + canvas.get_context_with_context_options("webgl2", &Self::create_context_options()), + ) } pub fn create_surface_from_offscreen_canvas( &self, canvas: &web_sys::OffscreenCanvas, ) -> Result { - let webgl2_context = canvas - .get_context_with_context_options("webgl2", &Self::create_context_options()) - .expect("Cannot create WebGL2 context") - .and_then(|context| context.dyn_into::().ok()) - .expect("Cannot convert into WebGL2 context"); + self.create_surface_from_context( + canvas.get_context_with_context_options("webgl2", &Self::create_context_options()), + ) + } + + /// Common portion of public `create_surface_from_*` functions. + /// + /// Note: Analogous code also exists in the WebGPU backend at + /// `wgpu::backend::web::Context`. + fn create_surface_from_context( + &self, + context_result: Result, wasm_bindgen::JsValue>, + ) -> Result { + let context_object: js_sys::Object = match context_result { + Ok(Some(context)) => context, + Ok(None) => { + // + // A getContext() call “returns null if contextId is not supported, or if the + // canvas has already been initialized with another context type”. Additionally, + // “not supported” could include “insufficient GPU resources” or “the GPU process + // previously crashed”. So, we must return it as an `Err` since it could occur + // for circumstances outside the application author's control. + return Err(crate::InstanceError); + } + Err(js_error) => { + // + // A thrown exception indicates misuse of the canvas state. Ideally we wouldn't + // panic in this case, but for now, `InstanceError` conveys no detail, so it + // is more informative to panic with a specific message. + panic!("canvas.getContext() threw {js_error:?}") + } + }; + + // Not returning this error because it is a type error that shouldn't happen unless + // the browser, JS builtin objects, or wasm bindings are misbehaving somehow. + let webgl2_context: web_sys::WebGl2RenderingContext = context_object + .dyn_into() + .expect("canvas context is not a WebGl2RenderingContext"); *self.webgl2_context.lock() = Some(webgl2_context.clone()); diff --git a/wgpu/src/backend/web.rs b/wgpu/src/backend/web.rs index 6e4e6d4496..4d1ee7a10e 100644 --- a/wgpu/src/backend/web.rs +++ b/wgpu/src/backend/web.rs @@ -1027,22 +1027,52 @@ impl Context { &self, canvas: &web_sys::HtmlCanvasElement, ) -> ::SurfaceId { - let context: wasm_bindgen::JsValue = match canvas.get_context("webgpu") { - Ok(Some(ctx)) => ctx.into(), - _ => panic!("expected to get context from canvas"), - }; - create_identified(context.into()) + self.create_surface_from_context(canvas.get_context("webgpu")) + .unwrap() } pub fn instance_create_surface_from_offscreen_canvas( &self, canvas: &web_sys::OffscreenCanvas, ) -> ::SurfaceId { - let context: wasm_bindgen::JsValue = match canvas.get_context("webgpu") { - Ok(Some(ctx)) => ctx.into(), - _ => panic!("expected to get context from canvas"), + self.create_surface_from_context(canvas.get_context("webgpu")) + .unwrap() + } + + /// Common portion of public `instance_create_surface_from_*` functions. + /// + /// Note: Analogous code also exists in the WebGL 2 backend at + /// `wgpu_hal::gles::web::Instance`. + fn create_surface_from_context( + &self, + context_result: Result, wasm_bindgen::JsValue>, + ) -> Result<::SurfaceId, ()> { + let context: js_sys::Object = match context_result { + Ok(Some(context)) => context, + Ok(None) => { + // + // A getContext() call “returns null if contextId is not supported, or if the + // canvas has already been initialized with another context type”. Additionally, + // “not supported” could include “insufficient GPU resources” or “the GPU process + // previously crashed”. So, we must return it as an `Err` since it could occur + // for circumstances outside the application author's control. + return Err(()); + } + Err(js_error) => { + // + // A thrown exception indicates misuse of the canvas state. Ideally we wouldn't + // panic in this case ... TODO + panic!("canvas.getContext() threw {js_error:?}") + } }; - create_identified(context.into()) + + // Not returning this error because it is a type error that shouldn't happen unless + // the browser, JS builtin objects, or wasm bindings are misbehaving somehow. + let context: web_sys::GpuCanvasContext = context + .dyn_into() + .expect("canvas context is not a GPUCanvasContext"); + + Ok(create_identified(context)) } pub fn queue_copy_external_image_to_texture( From c28eab20457960a16c19dd8a55d78da20cc6be14 Mon Sep 17 00:00:00 2001 From: Kevin Reid Date: Mon, 26 Sep 2022 19:35:52 -0700 Subject: [PATCH 2/2] Breaking: Change type of `create_surface()` functions to expose canvas errors. Part of fixing #3017. --- CHANGELOG.md | 4 ++ wgpu-core/src/instance.rs | 40 +++++++++------- wgpu/Cargo.toml | 8 +++- wgpu/examples/framework.rs | 5 +- wgpu/examples/hello-triangle/main.rs | 2 +- wgpu/examples/hello-windows/main.rs | 2 +- wgpu/src/backend/direct.rs | 28 ++++++----- wgpu/src/backend/web.rs | 14 +++--- wgpu/src/lib.rs | 72 +++++++++++++++++++++------- 9 files changed, 118 insertions(+), 57 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 030960c49c..fb07c09bcd 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -63,6 +63,10 @@ Additionally `Surface::get_default_config` now returns an Option and returns Non + let config = surface.get_default_config(&adapter).expect("Surface unsupported by adapter"); ``` +#### Fallible surface creation + +`Instance::create_surface()` now returns `Result` instead of `Surface`. This allows an error to be returned instead of panicking if the given window is a HTML canvas and obtaining a WebGPU or WebGL 2 context fails. (No other platforms currently report any errors through this path.) By @kpreid in [#3052](https://github.com/gfx-rs/wgpu/pull/3052/) + ### Changes #### General diff --git a/wgpu-core/src/instance.rs b/wgpu-core/src/instance.rs index 82468881e2..f1419c78d0 100644 --- a/wgpu-core/src/instance.rs +++ b/wgpu-core/src/instance.rs @@ -502,22 +502,26 @@ impl Global { &self, canvas: &web_sys::HtmlCanvasElement, id_in: Input, - ) -> SurfaceId { + ) -> Result { profiling::scope!("Instance::create_surface_webgl_canvas"); let surface = Surface { presentation: None, - gl: self.instance.gl.as_ref().map(|inst| HalSurface { - raw: { - inst.create_surface_from_canvas(canvas) - .expect("Create surface from canvas") - }, - }), + gl: self + .instance + .gl + .as_ref() + .map(|inst| { + Ok(HalSurface { + raw: inst.create_surface_from_canvas(canvas)?, + }) + }) + .transpose()?, }; let mut token = Token::root(); let id = self.surfaces.prepare(id_in).assign(surface, &mut token); - id.0 + Ok(id.0) } #[cfg(all(target_arch = "wasm32", not(target_os = "emscripten")))] @@ -525,22 +529,26 @@ impl Global { &self, canvas: &web_sys::OffscreenCanvas, id_in: Input, - ) -> SurfaceId { + ) -> Result { profiling::scope!("Instance::create_surface_webgl_offscreen_canvas"); let surface = Surface { presentation: None, - gl: self.instance.gl.as_ref().map(|inst| HalSurface { - raw: { - inst.create_surface_from_offscreen_canvas(canvas) - .expect("Create surface from offscreen canvas") - }, - }), + gl: self + .instance + .gl + .as_ref() + .map(|inst| { + Ok(HalSurface { + raw: inst.create_surface_from_offscreen_canvas(canvas)?, + }) + }) + .transpose()?, }; let mut token = Token::root(); let id = self.surfaces.prepare(id_in).assign(surface, &mut token); - id.0 + Ok(id.0) } #[cfg(dx12)] diff --git a/wgpu/Cargo.toml b/wgpu/Cargo.toml index 36272c0fda..57bafc566a 100644 --- a/wgpu/Cargo.toml +++ b/wgpu/Cargo.toml @@ -83,7 +83,7 @@ wgsl = ["wgc?/wgsl"] trace = ["serde", "wgc/trace"] replay = ["serde", "wgc/replay"] angle = ["wgc/angle"] -webgl = ["wgc"] +webgl = ["hal", "wgc"] emscripten = ["webgl"] vulkan-portability = ["wgc/vulkan-portability"] expose-ids = [] @@ -100,9 +100,13 @@ optional = true [dependencies.wgt] workspace = true -[target.'cfg(any(not(target_arch = "wasm32"), target_os = "emscripten"))'.dependencies.hal] +[target.'cfg(not(target_arch = "wasm32"))'.dependencies.hal] workspace = true +[target.'cfg(target_arch = "wasm32")'.dependencies.hal] +workspace = true +optional = true + [dependencies] arrayvec.workspace = true log.workspace = true diff --git a/wgpu/examples/framework.rs b/wgpu/examples/framework.rs index 122c88170d..9af044bf66 100644 --- a/wgpu/examples/framework.rs +++ b/wgpu/examples/framework.rs @@ -164,7 +164,7 @@ async fn setup(title: &str) -> Setup { let size = window.inner_size(); #[cfg(any(not(target_arch = "wasm32"), target_os = "emscripten"))] - let surface = instance.create_surface(&window); + let surface = instance.create_surface(&window).unwrap(); #[cfg(all(target_arch = "wasm32", not(target_os = "emscripten")))] let surface = { if let Some(offscreen_canvas_setup) = &offscreen_canvas_setup { @@ -174,7 +174,8 @@ async fn setup(title: &str) -> Setup { } else { instance.create_surface(&window) } - }; + } + .unwrap(); (size, surface) }; diff --git a/wgpu/examples/hello-triangle/main.rs b/wgpu/examples/hello-triangle/main.rs index 4c19babf3c..58e7ba1a90 100644 --- a/wgpu/examples/hello-triangle/main.rs +++ b/wgpu/examples/hello-triangle/main.rs @@ -8,7 +8,7 @@ use winit::{ async fn run(event_loop: EventLoop<()>, window: Window) { let size = window.inner_size(); let instance = wgpu::Instance::new(wgpu::Backends::all()); - let surface = unsafe { instance.create_surface(&window) }; + let surface = unsafe { instance.create_surface(&window) }.unwrap(); let adapter = instance .request_adapter(&wgpu::RequestAdapterOptions { power_preference: wgpu::PowerPreference::default(), diff --git a/wgpu/examples/hello-windows/main.rs b/wgpu/examples/hello-windows/main.rs index 0dcc87daac..4a93cf6b7f 100644 --- a/wgpu/examples/hello-windows/main.rs +++ b/wgpu/examples/hello-windows/main.rs @@ -20,7 +20,7 @@ struct Viewport { impl ViewportDesc { fn new(window: Window, background: wgpu::Color, instance: &wgpu::Instance) -> Self { - let surface = unsafe { instance.create_surface(&window) }; + let surface = unsafe { instance.create_surface(&window) }.unwrap(); Self { window, background, diff --git a/wgpu/src/backend/direct.rs b/wgpu/src/backend/direct.rs index 7eb0d4eba1..6187bedb06 100644 --- a/wgpu/src/backend/direct.rs +++ b/wgpu/src/backend/direct.rs @@ -219,24 +219,30 @@ impl Context { pub fn instance_create_surface_from_canvas( self: &Arc, canvas: &web_sys::HtmlCanvasElement, - ) -> Surface { - let id = self.0.create_surface_webgl_canvas(canvas, ()); - Surface { + ) -> Result { + let id = self + .0 + .create_surface_webgl_canvas(canvas, ()) + .map_err(|hal::InstanceError| crate::CreateSurfaceError {})?; + Ok(Surface { id, configured_device: Mutex::default(), - } + }) } #[cfg(all(target_arch = "wasm32", feature = "webgl", not(feature = "emscripten")))] pub fn instance_create_surface_from_offscreen_canvas( self: &Arc, canvas: &web_sys::OffscreenCanvas, - ) -> Surface { - let id = self.0.create_surface_webgl_offscreen_canvas(canvas, ()); - Surface { + ) -> Result { + let id = self + .0 + .create_surface_webgl_offscreen_canvas(canvas, ()) + .map_err(|hal::InstanceError| crate::CreateSurfaceError {})?; + Ok(Surface { id, configured_device: Mutex::default(), - } + }) } #[cfg(target_os = "windows")] @@ -943,13 +949,13 @@ impl crate::Context for Context { &self, display_handle: raw_window_handle::RawDisplayHandle, window_handle: raw_window_handle::RawWindowHandle, - ) -> Self::SurfaceId { - Surface { + ) -> Result { + Ok(Surface { id: self .0 .instance_create_surface(display_handle, window_handle, ()), configured_device: Mutex::new(None), - } + }) } fn instance_request_adapter( diff --git a/wgpu/src/backend/web.rs b/wgpu/src/backend/web.rs index 4d1ee7a10e..a8fc828815 100644 --- a/wgpu/src/backend/web.rs +++ b/wgpu/src/backend/web.rs @@ -1026,27 +1026,25 @@ impl Context { pub fn instance_create_surface_from_canvas( &self, canvas: &web_sys::HtmlCanvasElement, - ) -> ::SurfaceId { + ) -> Result<::SurfaceId, crate::CreateSurfaceError> { self.create_surface_from_context(canvas.get_context("webgpu")) - .unwrap() } pub fn instance_create_surface_from_offscreen_canvas( &self, canvas: &web_sys::OffscreenCanvas, - ) -> ::SurfaceId { + ) -> Result<::SurfaceId, crate::CreateSurfaceError> { self.create_surface_from_context(canvas.get_context("webgpu")) - .unwrap() } /// Common portion of public `instance_create_surface_from_*` functions. /// - /// Note: Analogous code also exists in the WebGL 2 backend at + /// Note: Analogous code also exists in the WebGL2 backend at /// `wgpu_hal::gles::web::Instance`. fn create_surface_from_context( &self, context_result: Result, wasm_bindgen::JsValue>, - ) -> Result<::SurfaceId, ()> { + ) -> Result<::SurfaceId, crate::CreateSurfaceError> { let context: js_sys::Object = match context_result { Ok(Some(context)) => context, Ok(None) => { @@ -1056,7 +1054,7 @@ impl Context { // “not supported” could include “insufficient GPU resources” or “the GPU process // previously crashed”. So, we must return it as an `Err` since it could occur // for circumstances outside the application author's control. - return Err(()); + return Err(crate::CreateSurfaceError {}); } Err(js_error) => { // @@ -1171,7 +1169,7 @@ impl crate::Context for Context { &self, _display_handle: raw_window_handle::RawDisplayHandle, window_handle: raw_window_handle::RawWindowHandle, - ) -> Self::SurfaceId { + ) -> Result { let canvas_attribute = match window_handle { raw_window_handle::RawWindowHandle::Web(web_handle) => web_handle.id, _ => panic!("expected valid handle for canvas"), diff --git a/wgpu/src/lib.rs b/wgpu/src/lib.rs index ff67365ec6..bb04d00ac3 100644 --- a/wgpu/src/lib.rs +++ b/wgpu/src/lib.rs @@ -204,7 +204,7 @@ trait Context: Debug + Send + Sized + Sync { &self, display_handle: raw_window_handle::RawDisplayHandle, window_handle: raw_window_handle::RawWindowHandle, - ) -> Self::SurfaceId; + ) -> Result; fn instance_request_adapter( &self, options: &RequestAdapterOptions<'_>, @@ -1816,23 +1816,34 @@ impl Instance { /// /// # Safety /// - /// - Raw Window Handle must be a valid object to create a surface upon and - /// must remain valid for the lifetime of the returned surface. - /// - If not called on the main thread, metal backend will panic. + /// - `raw_window_handle` must be a valid object to create a surface upon. + /// - `raw_window_handle` must remain valid until after the returned [`Surface`] is + /// dropped. + /// + /// # Errors + /// + /// - On WebGL2: Will return an error if the browser does not support WebGL2, + /// or declines to provide GPU access (such as due to a resource shortage). + /// + /// # Panics + /// + /// - On macOS/Metal: will panic if not called on the main thread. + /// - On web: will panic if the `raw_window_handle` does not properly refer to a + /// canvas element. pub unsafe fn create_surface< W: raw_window_handle::HasRawWindowHandle + raw_window_handle::HasRawDisplayHandle, >( &self, window: &W, - ) -> Surface { - Surface { + ) -> Result { + Ok(Surface { context: Arc::clone(&self.context), id: Context::instance_create_surface( &*self.context, raw_window_handle::HasRawDisplayHandle::raw_display_handle(window), raw_window_handle::HasRawWindowHandle::raw_window_handle(window), - ), - } + )?, + }) } /// Creates a surface from `CoreAnimationLayer`. @@ -1862,29 +1873,42 @@ impl Instance { /// /// The `canvas` argument must be a valid `` element to /// create a surface upon. + /// + /// # Errors + /// + /// - On WebGL2: Will return an error if the browser does not support WebGL2, + /// or declines to provide GPU access (such as due to a resource shortage). #[cfg(all(target_arch = "wasm32", not(feature = "emscripten")))] - pub fn create_surface_from_canvas(&self, canvas: &web_sys::HtmlCanvasElement) -> Surface { - Surface { + pub fn create_surface_from_canvas( + &self, + canvas: &web_sys::HtmlCanvasElement, + ) -> Result { + Ok(Surface { context: Arc::clone(&self.context), - id: self.context.instance_create_surface_from_canvas(canvas), - } + id: self.context.instance_create_surface_from_canvas(canvas)?, + }) } /// Creates a surface from a `web_sys::OffscreenCanvas`. /// /// The `canvas` argument must be a valid `OffscreenCanvas` object /// to create a surface upon. + /// + /// # Errors + /// + /// - On WebGL2: Will return an error if the browser does not support WebGL2, + /// or declines to provide GPU access (such as due to a resource shortage). #[cfg(all(target_arch = "wasm32", not(feature = "emscripten")))] pub fn create_surface_from_offscreen_canvas( &self, canvas: &web_sys::OffscreenCanvas, - ) -> Surface { - Surface { + ) -> Result { + Ok(Surface { context: Arc::clone(&self.context), id: self .context - .instance_create_surface_from_offscreen_canvas(canvas), - } + .instance_create_surface_from_offscreen_canvas(canvas)?, + }) } /// Polls all devices. @@ -2353,6 +2377,22 @@ impl Display for RequestDeviceError { impl error::Error for RequestDeviceError {} +/// [`Instance::create_surface()`] or a related function failed. +#[derive(Clone, PartialEq, Eq, Debug)] +#[non_exhaustive] +pub struct CreateSurfaceError { + // TODO: Report diagnostic clues +} +static_assertions::assert_impl_all!(CreateSurfaceError: Send, Sync); + +impl Display for CreateSurfaceError { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + write!(f, "Creating a surface failed") + } +} + +impl error::Error for CreateSurfaceError {} + /// Error occurred when trying to async map a buffer. #[derive(Clone, PartialEq, Eq, Debug)] pub struct BufferAsyncError;