Skip to content

Commit

Permalink
Add details to InstanceError and CreateSurfaceError.
Browse files Browse the repository at this point in the history
This will allow developers, particularly those in communication with
users with hardware they don't own, to diagnose “why won't this work
on my machine” better, without needing to obtain logs in addition to
the error value.

Note that this is a semver-breaking change because `InstanceError` and
`CreateSurfaceError` no longer implement `PartialEq`. This could be
avoided by pointer comparison of the internal `Arc`, but I think it is
more typical Rust to not implement `PartialEq` for error types unless
there is a strong motivation to and their contents are tightly
controlled.
  • Loading branch information
kpreid committed Aug 16, 2023
1 parent f61ed50 commit cd0f99c
Show file tree
Hide file tree
Showing 14 changed files with 268 additions and 124 deletions.
3 changes: 3 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -70,9 +70,12 @@ By @Valaphee in [#3402](https://github.com/gfx-rs/wgpu/pull/3402)

### Changes

#### General

- Omit texture store bound checks since they are no-ops if out of bounds on all APIs. By @teoxoy in [#3975](https://github.com/gfx-rs/wgpu/pull/3975)
- Validate `DownlevelFlags::READ_ONLY_DEPTH_STENCIL`. By @teoxoy in [#4031](https://github.com/gfx-rs/wgpu/pull/4031)
- Add validation in accordance with WebGPU `setViewport` valid usage for `x`, `y` and `this.[[attachment_size]]`. By @James2022-rgb in [#4058](https://github.com/gfx-rs/wgpu/pull/4058)
- `wgpu::CreateSurfaceError` now gives details of the failure, but no longer implements `PartialEq`. By @kpreid in [#4066](https://github.com/gfx-rs/wgpu/pull/4066)

#### Vulkan

Expand Down
8 changes: 4 additions & 4 deletions wgpu-hal/examples/halmark/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,7 @@ struct Example<A: hal::Api> {
}

impl<A: hal::Api> Example<A> {
fn init(window: &winit::window::Window) -> Result<Self, hal::InstanceError> {
fn init(window: &winit::window::Window) -> Result<Self, Box<dyn std::error::Error>> {
let instance_desc = hal::InstanceDescriptor {
name: "example",
flags: if cfg!(debug_assertions) {
Expand All @@ -108,13 +108,13 @@ impl<A: hal::Api> Example<A> {
let (adapter, capabilities) = unsafe {
let mut adapters = instance.enumerate_adapters();
if adapters.is_empty() {
return Err(hal::InstanceError);
return Err("no adapters found".into());
}
let exposed = adapters.swap_remove(0);
(exposed.adapter, exposed.capabilities)
};
let surface_caps =
unsafe { adapter.surface_capabilities(&surface) }.ok_or(hal::InstanceError)?;
let surface_caps = unsafe { adapter.surface_capabilities(&surface) }
.ok_or("failed to get surface capabilities")?;
log::info!("Surface caps: {:#?}", surface_caps);

let hal::OpenDevice { device, mut queue } = unsafe {
Expand Down
42 changes: 28 additions & 14 deletions wgpu-hal/src/auxil/dxgi/factory.rs
Original file line number Diff line number Diff line change
Expand Up @@ -96,7 +96,9 @@ pub fn create_factory(
required_factory_type: DxgiFactoryType,
instance_flags: crate::InstanceFlags,
) -> Result<(d3d12::DxgiLib, d3d12::DxgiFactory), crate::InstanceError> {
let lib_dxgi = d3d12::DxgiLib::new().map_err(|_| crate::InstanceError)?;
let lib_dxgi = d3d12::DxgiLib::new().map_err(|e| {
crate::InstanceError::with_source(String::from("failed to load dxgi.dll"), e)
})?;

let mut factory_flags = d3d12::FactoryCreationFlags::empty();

Expand Down Expand Up @@ -128,18 +130,22 @@ pub fn create_factory(
Ok(factory) => Some(factory),
// We hard error here as we _should have_ been able to make a factory4 but couldn't.
Err(err) => {
log::error!("Failed to create IDXGIFactory4: {}", err);
return Err(crate::InstanceError);
return Err(crate::InstanceError::with_source(
String::from("failed to create IDXGIFactory4"),
err,
));
}
},
// If we require factory4, hard error.
Err(err) if required_factory_type == DxgiFactoryType::Factory4 => {
log::error!("IDXGIFactory1 creation function not found: {:?}", err);
return Err(crate::InstanceError);
return Err(crate::InstanceError::with_source(
String::from("IDXGIFactory1 creation function not found"),
err,
));
}
// If we don't print it to info as all win7 will hit this case.
Err(err) => {
log::info!("IDXGIFactory1 creation function not found: {:?}", err);
log::info!("IDXGIFactory1 creation function not found: {err:?}");
None
}
};
Expand All @@ -153,8 +159,10 @@ pub fn create_factory(
}
// If we require factory6, hard error.
Err(err) if required_factory_type == DxgiFactoryType::Factory6 => {
log::warn!("Failed to cast IDXGIFactory4 to IDXGIFactory6: {:?}", err);
return Err(crate::InstanceError);
return Err(crate::InstanceError::with_source(
format!("failed to cast IDXGIFactory4 to IDXGIFactory6"),
err,
));
}
// If we don't print it to info.
Err(err) => {
Expand All @@ -169,14 +177,18 @@ pub fn create_factory(
Ok(pair) => match pair.into_result() {
Ok(factory) => factory,
Err(err) => {
log::error!("Failed to create IDXGIFactory1: {}", err);
return Err(crate::InstanceError);
return Err(crate::InstanceError::with_source(
format!("failed to create IDXGIFactory1"),
err,
));
}
},
// We always require at least factory1, so hard error
Err(err) => {
log::error!("IDXGIFactory1 creation function not found: {:?}", err);
return Err(crate::InstanceError);
return Err(crate::InstanceError::with_source(
format!("IDXGIFactory1 creation function not found"),
err,
));
}
};

Expand All @@ -188,8 +200,10 @@ pub fn create_factory(
}
// If we require factory2, hard error.
Err(err) if required_factory_type == DxgiFactoryType::Factory2 => {
log::warn!("Failed to cast IDXGIFactory1 to IDXGIFactory2: {:?}", err);
return Err(crate::InstanceError);
return Err(crate::InstanceError::with_source(
format!("failed to cast IDXGIFactory1 to IDXGIFactory2"),
err,
));
}
// If we don't print it to info.
Err(err) => {
Expand Down
7 changes: 5 additions & 2 deletions wgpu-hal/src/dx11/instance.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,10 +8,13 @@ impl crate::Instance<super::Api> for super::Instance {
};

if !enable_dx11 {
return Err(crate::InstanceError);
return Err(crate::InstanceError::new(String::from(
"DX11 support is unstable; set WGPU_UNSTABLE_DX11_BACKEND=1 to enable anyway",
)));
}

let lib_d3d11 = super::library::D3D11Lib::new().ok_or(crate::InstanceError)?;
let lib_d3d11 = super::library::D3D11Lib::new()
.ok_or_else(|| crate::InstanceError::new(String::from("failed to load d3d11.dll")))?;

let (lib_dxgi, factory) = auxil::dxgi::factory::create_factory(
auxil::dxgi::factory::DxgiFactoryType::Factory1,
Expand Down
8 changes: 6 additions & 2 deletions wgpu-hal/src/dx12/instance.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,9 @@ impl Drop for super::Instance {

impl crate::Instance<super::Api> for super::Instance {
unsafe fn init(desc: &crate::InstanceDescriptor) -> Result<Self, crate::InstanceError> {
let lib_main = d3d12::D3D12Lib::new().map_err(|_| crate::InstanceError)?;
let lib_main = d3d12::D3D12Lib::new().map_err(|e| {
crate::InstanceError::with_source(String::from("failed to load d3d12.dll"), e)
})?;

if desc.flags.contains(crate::InstanceFlags::VALIDATION) {
// Enable debug layer
Expand Down Expand Up @@ -95,7 +97,9 @@ impl crate::Instance<super::Api> for super::Instance {
supports_allow_tearing: self.supports_allow_tearing,
swap_chain: None,
}),
_ => Err(crate::InstanceError),
_ => Err(crate::InstanceError::new(format!(
"window handle {window_handle:?} is not a Win32 handle"
))),
}
}
unsafe fn destroy_surface(&self, _surface: super::Surface) {
Expand Down
45 changes: 24 additions & 21 deletions wgpu-hal/src/gles/adapter.rs
Original file line number Diff line number Diff line change
Expand Up @@ -43,8 +43,9 @@ impl super::Adapter {
src = &src[pos + es_sig.len()..];
}
None => {
log::warn!("ES not found in '{}'", src);
return Err(crate::InstanceError);
return Err(crate::InstanceError::new(format!(
"OpenGL version {src:?} does not contain 'ES'"
)));
}
}
};
Expand Down Expand Up @@ -86,10 +87,9 @@ impl super::Adapter {
},
minor,
)),
_ => {
log::warn!("Unable to extract the version from '{}'", version);
Err(crate::InstanceError)
}
_ => Err(crate::InstanceError::new(format!(
"unable to extract OpenGL version from {version:?}"
))),
}
}

Expand Down Expand Up @@ -974,27 +974,30 @@ mod tests {

#[test]
fn test_version_parse() {
let error = Err(crate::InstanceError);
assert_eq!(Adapter::parse_version("1"), error);
assert_eq!(Adapter::parse_version("1."), error);
assert_eq!(Adapter::parse_version("1 h3l1o. W0rld"), error);
assert_eq!(Adapter::parse_version("1. h3l1o. W0rld"), error);
assert_eq!(Adapter::parse_version("1.2.3"), error);
assert_eq!(Adapter::parse_version("OpenGL ES 3.1"), Ok((3, 1)));
Adapter::parse_version("1").unwrap_err();
Adapter::parse_version("1.").unwrap_err();
Adapter::parse_version("1 h3l1o. W0rld").unwrap_err();
Adapter::parse_version("1. h3l1o. W0rld").unwrap_err();
Adapter::parse_version("1.2.3").unwrap_err();

assert_eq!(Adapter::parse_version("OpenGL ES 3.1").unwrap(), (3, 1));
assert_eq!(
Adapter::parse_version("OpenGL ES 2.0 Google Nexus").unwrap(),
(2, 0)
);
assert_eq!(Adapter::parse_version("GLSL ES 1.1").unwrap(), (1, 1));
assert_eq!(
Adapter::parse_version("OpenGL ES 2.0 Google Nexus"),
Ok((2, 0))
Adapter::parse_version("OpenGL ES GLSL ES 3.20").unwrap(),
(3, 2)
);
assert_eq!(Adapter::parse_version("GLSL ES 1.1"), Ok((1, 1)));
assert_eq!(Adapter::parse_version("OpenGL ES GLSL ES 3.20"), Ok((3, 2)));
assert_eq!(
// WebGL 2.0 should parse as OpenGL ES 3.0
Adapter::parse_version("WebGL 2.0 (OpenGL ES 3.0 Chromium)"),
Ok((3, 0))
Adapter::parse_version("WebGL 2.0 (OpenGL ES 3.0 Chromium)").unwrap(),
(3, 0)
);
assert_eq!(
Adapter::parse_version("WebGL GLSL ES 3.00 (OpenGL ES GLSL ES 3.0 Chromium)"),
Ok((3, 0))
Adapter::parse_version("WebGL GLSL ES 3.00 (OpenGL ES GLSL ES 3.0 Chromium)").unwrap(),
(3, 0)
);
}
}
43 changes: 29 additions & 14 deletions wgpu-hal/src/gles/egl.rs
Original file line number Diff line number Diff line change
Expand Up @@ -283,7 +283,10 @@ fn choose_config(
}
}

Err(crate::InstanceError)
// TODO: include diagnostic details that are currently logged
Err(crate::InstanceError::new(String::from(
"unable to find an acceptable EGL framebuffer configuration",
)))
}

fn gl_debug_message_callback(source: u32, gltype: u32, id: u32, severity: u32, message: &str) {
Expand Down Expand Up @@ -495,7 +498,12 @@ impl Inner {
display: khronos_egl::Display,
force_gles_minor_version: wgt::Gles3MinorVersion,
) -> Result<Self, crate::InstanceError> {
let version = egl.initialize(display).map_err(|_| crate::InstanceError)?;
let version = egl.initialize(display).map_err(|e| {
crate::InstanceError::with_source(
String::from("failed to initialize EGL display connection"),
e,
)
})?;
let vendor = egl
.query_string(Some(display), khronos_egl::VENDOR)
.unwrap();
Expand Down Expand Up @@ -599,8 +607,10 @@ impl Inner {
let context = match egl.create_context(display, config, None, &context_attributes) {
Ok(context) => context,
Err(e) => {
log::warn!("unable to create GLES 3.x context: {:?}", e);
return Err(crate::InstanceError);
return Err(crate::InstanceError::with_source(
String::from("unable to create GLES 3.x context"),
e,
));
}
};

Expand All @@ -623,8 +633,10 @@ impl Inner {
egl.create_pbuffer_surface(display, config, &attributes)
.map(Some)
.map_err(|e| {
log::warn!("Error in create_pbuffer_surface: {:?}", e);
crate::InstanceError
crate::InstanceError::with_source(
String::from("error in create_pbuffer_surface"),
e,
)
})?
};

Expand Down Expand Up @@ -734,8 +746,10 @@ impl crate::Instance<super::Api> for Instance {
let egl = match egl_result {
Ok(egl) => Arc::new(egl),
Err(e) => {
log::info!("Unable to open libEGL: {:?}", e);
return Err(crate::InstanceError);
return Err(crate::InstanceError::with_source(
String::from("unable to open libEGL"),
e,
));
}
};

Expand Down Expand Up @@ -899,8 +913,9 @@ impl crate::Instance<super::Api> for Instance {
};

if ret != 0 {
log::error!("Error returned from ANativeWindow_setBuffersGeometry");
return Err(crate::InstanceError);
return Err(crate::InstanceError::new(format(
"error {ret} returned from ANativeWindow_setBuffersGeometry",
)));
}
}
#[cfg(not(target_os = "emscripten"))]
Expand Down Expand Up @@ -938,8 +953,7 @@ impl crate::Instance<super::Api> for Instance {
Arc::clone(&inner.egl.instance),
display,
inner.force_gles_minor_version,
)
.map_err(|_| crate::InstanceError)?;
)?;

let old_inner = std::mem::replace(inner.deref_mut(), new_inner);
inner.wl_display = Some(display_handle.display);
Expand All @@ -950,8 +964,9 @@ impl crate::Instance<super::Api> for Instance {
#[cfg(target_os = "emscripten")]
(Rwh::Web(_), _) => {}
other => {
log::error!("Unsupported window: {:?}", other);
return Err(crate::InstanceError);
return Err(crate::InstanceError::new(format!(
"unsupported window: {other:?}"
)));
}
};

Expand Down
16 changes: 10 additions & 6 deletions wgpu-hal/src/gles/web.rs
Original file line number Diff line number Diff line change
Expand Up @@ -65,14 +65,16 @@ impl Instance {
// “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);
return Err(crate::InstanceError::new(String::from(
"canvas.getContext() returned null; webgl2 not available or canvas already in use"
)));
}
Err(js_error) => {
// <https://html.spec.whatwg.org/multipage/canvas.html#dom-canvas-getcontext>
// 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:?}")
// A thrown exception indicates misuse of the canvas state.
return Err(crate::InstanceError::new(format!(
"canvas.getContext() threw exception {js_error:?}",
)));
}
};

Expand Down Expand Up @@ -153,7 +155,9 @@ impl crate::Instance<super::Api> for Instance {

self.create_surface_from_canvas(canvas)
} else {
Err(crate::InstanceError)
Err(crate::InstanceError::new(format!(
"window handle {window_handle:?} is not a web handle"
)))
}
}

Expand Down
Loading

0 comments on commit cd0f99c

Please sign in to comment.