Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Safe surface creation #4597

Merged
merged 5 commits into from
Nov 16, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,14 @@ Bottom level categories:
- Log vulkan validation layer messages during instance creation and destruction: By @exrook in [#4586](https://github.com/gfx-rs/wgpu/pull/4586)
- `TextureFormat::block_size` is deprecated, use `TextureFormat::block_copy_size` instead: By @wumpf in [#4647](https://github.com/gfx-rs/wgpu/pull/4647)

#### Safe `Surface` creation

It is now possible to safely create a `wgpu::Surface` with `Surface::create_surface()` by letting `Surface` hold a lifetime to `window`.

Passing an owned value `window` to `Surface` will return a `Surface<'static>`. Shared ownership over `window` can still be achieved with e.g. an `Arc`. Alternatively a reference could be passed, which will return a `Surface<'window>`.

`Surface::create_surface_from_raw()` can be used to continue producing a `Surface<'static>` without any lifetime requirements over `window`, which also remains `unsafe`.

#### Naga

- Introduce a new `Scalar` struct type for use in Naga's IR, and update all frontend, middle, and backend code appropriately. By @jimblandy in [#4673](https://github.com/gfx-rs/wgpu/pull/4673).
Expand Down
23 changes: 12 additions & 11 deletions examples/common/src/framework.rs
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
use std::sync::Arc;

use wgpu::{Instance, Surface, WasmNotSend, WasmNotSync};
use wgpu_test::GpuTestConfiguration;
use winit::{
Expand Down Expand Up @@ -90,15 +92,15 @@ fn init_logger() {

struct EventLoopWrapper {
event_loop: EventLoop<()>,
window: Window,
window: Arc<Window>,
}

impl EventLoopWrapper {
pub fn new(title: &str) -> Self {
let event_loop = EventLoop::new().unwrap();
let mut builder = winit::window::WindowBuilder::new();
builder = builder.with_title(title);
let window = builder.build(&event_loop).unwrap();
let window = Arc::new(builder.build(&event_loop).unwrap());

#[cfg(target_arch = "wasm32")]
{
Expand All @@ -121,7 +123,7 @@ impl EventLoopWrapper {
///
/// As surface usage varies per platform, wrapping this up cleans up the event loop code.
struct SurfaceWrapper {
surface: Option<wgpu::Surface>,
surface: Option<wgpu::Surface<'static>>,
config: Option<wgpu::SurfaceConfiguration>,
}

Expand All @@ -141,9 +143,9 @@ impl SurfaceWrapper {
///
/// We cannot unconditionally create a surface here, as Android requires
/// us to wait until we recieve the `Resumed` event to do so.
fn pre_adapter(&mut self, instance: &Instance, window: &Window) {
fn pre_adapter(&mut self, instance: &Instance, window: Arc<Window>) {
if cfg!(target_arch = "wasm32") {
self.surface = Some(unsafe { instance.create_surface(&window).unwrap() });
self.surface = Some(instance.create_surface(window).unwrap());
}
}

Expand All @@ -163,7 +165,7 @@ impl SurfaceWrapper {
/// On all native platforms, this is where we create the surface.
///
/// Additionally, we configure the surface based on the (now valid) window size.
fn resume(&mut self, context: &ExampleContext, window: &Window, srgb: bool) {
fn resume(&mut self, context: &ExampleContext, window: Arc<Window>, srgb: bool) {
// Window size is only actually valid after we enter the event loop.
let window_size = window.inner_size();
let width = window_size.width.max(1);
Expand All @@ -173,7 +175,7 @@ impl SurfaceWrapper {

// We didn't create the surface in pre_adapter, so we need to do so now.
if !cfg!(target_arch = "wasm32") {
self.surface = Some(unsafe { context.instance.create_surface(&window).unwrap() });
self.surface = Some(context.instance.create_surface(window).unwrap());
}

// From here on, self.surface should be Some.
Expand Down Expand Up @@ -252,7 +254,7 @@ struct ExampleContext {
}
impl ExampleContext {
/// Initializes the example context.
async fn init_async<E: Example>(surface: &mut SurfaceWrapper, window: &Window) -> Self {
async fn init_async<E: Example>(surface: &mut SurfaceWrapper, window: Arc<Window>) -> Self {
log::info!("Initializing wgpu...");

let backends = wgpu::util::backend_bits_from_env().unwrap_or_default();
Expand Down Expand Up @@ -357,8 +359,7 @@ async fn start<E: Example>(title: &str) {
init_logger();
let window_loop = EventLoopWrapper::new(title);
let mut surface = SurfaceWrapper::new();
let context = ExampleContext::init_async::<E>(&mut surface, &window_loop.window).await;

let context = ExampleContext::init_async::<E>(&mut surface, window_loop.window.clone()).await;
let mut frame_counter = FrameCounter::new();

// We wait to create the example until we have a valid surface.
Expand All @@ -384,7 +385,7 @@ async fn start<E: Example>(title: &str) {

match event {
ref e if SurfaceWrapper::start_condition(e) => {
surface.resume(&context, &window_loop.window, E::SRGB);
surface.resume(&context, window_loop.window.clone(), E::SRGB);

// If we haven't created the example yet, do so now.
if example.is_none() {
Expand Down
3 changes: 2 additions & 1 deletion examples/hello-triangle/src/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ async fn run(event_loop: EventLoop<()>, window: Window) {

let instance = wgpu::Instance::default();

let surface = unsafe { instance.create_surface(&window) }.unwrap();
let surface = instance.create_surface(&window).unwrap();
let adapter = instance
.request_adapter(&wgpu::RequestAdapterOptions {
power_preference: wgpu::PowerPreference::default(),
Expand Down Expand Up @@ -84,6 +84,7 @@ async fn run(event_loop: EventLoop<()>, window: Window) {

surface.configure(&device, &config);

let window = &window;
event_loop
.run(move |event, target| {
// Have the closure take ownership of the resources.
Expand Down
13 changes: 7 additions & 6 deletions examples/hello-windows/src/main.rs
Original file line number Diff line number Diff line change
@@ -1,16 +1,16 @@
#![cfg_attr(target_arch = "wasm32", allow(dead_code))]

use std::collections::HashMap;
use std::{collections::HashMap, sync::Arc};
use winit::{
event::{Event, WindowEvent},
event_loop::EventLoop,
window::{Window, WindowId},
};

struct ViewportDesc {
window: Window,
window: Arc<Window>,
background: wgpu::Color,
surface: wgpu::Surface,
surface: wgpu::Surface<'static>,
}

struct Viewport {
Expand All @@ -19,8 +19,8 @@ struct Viewport {
}

impl ViewportDesc {
fn new(window: Window, background: wgpu::Color, instance: &wgpu::Instance) -> Self {
let surface = unsafe { instance.create_surface(&window) }.unwrap();
fn new(window: Arc<Window>, background: wgpu::Color, instance: &wgpu::Instance) -> Self {
let surface = instance.create_surface(window.clone()).unwrap();
Self {
window,
background,
Expand Down Expand Up @@ -62,7 +62,7 @@ impl Viewport {
}
}

async fn run(event_loop: EventLoop<()>, viewports: Vec<(Window, wgpu::Color)>) {
async fn run(event_loop: EventLoop<()>, viewports: Vec<(Arc<Window>, wgpu::Color)>) {
let instance = wgpu::Instance::default();
let viewports: Vec<_> = viewports
.into_iter()
Expand Down Expand Up @@ -180,6 +180,7 @@ fn main() {
.with_inner_size(winit::dpi::PhysicalSize::new(WINDOW_SIZE, WINDOW_SIZE))
.build(&event_loop)
.unwrap();
let window = Arc::new(window);
window.set_outer_position(winit::dpi::PhysicalPosition::new(
WINDOW_PADDING + column * WINDOW_OFFSET,
WINDOW_PADDING + row * (WINDOW_OFFSET + WINDOW_TITLEBAR),
Expand Down
12 changes: 7 additions & 5 deletions examples/uniform-values/src/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
//! The usage of the uniform buffer within the shader itself is pretty self-explanatory given
//! some understanding of WGSL.

use std::sync::Arc;
// We won't bring StorageBuffer into scope as that might be too easy to confuse
// with actual GPU-allocated WGPU storage buffers.
use encase::ShaderType;
Expand Down Expand Up @@ -84,8 +85,8 @@ impl Default for AppState {
}

struct WgpuContext {
pub window: Window,
pub surface: wgpu::Surface,
pub window: Arc<Window>,
pub surface: wgpu::Surface<'static>,
pub surface_config: wgpu::SurfaceConfiguration,
pub device: wgpu::Device,
pub queue: wgpu::Queue,
Expand All @@ -95,11 +96,11 @@ struct WgpuContext {
}

impl WgpuContext {
async fn new(window: Window) -> WgpuContext {
async fn new(window: Arc<Window>) -> WgpuContext {
let size = window.inner_size();

let instance = wgpu::Instance::default();
let surface = unsafe { instance.create_surface(&window) }.unwrap();
let surface = instance.create_surface(window.clone()).unwrap();
let adapter = instance
.request_adapter(&wgpu::RequestAdapterOptions {
power_preference: wgpu::PowerPreference::HighPerformance,
Expand Down Expand Up @@ -223,7 +224,7 @@ impl WgpuContext {
}
}

async fn run(event_loop: EventLoop<()>, window: Window) {
async fn run(event_loop: EventLoop<()>, window: Arc<Window>) {
let mut wgpu_context = Some(WgpuContext::new(window).await);
// (6)
let mut state = Some(AppState::default());
Expand Down Expand Up @@ -351,6 +352,7 @@ fn main() {
.with_inner_size(winit::dpi::LogicalSize::new(900, 900))
.build(&event_loop)
.unwrap();
let window = Arc::new(window);
#[cfg(not(target_arch = "wasm32"))]
{
env_logger::builder().format_timestamp_nanos().init();
Expand Down
2 changes: 1 addition & 1 deletion wgpu/src/backend/web.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1109,7 +1109,7 @@ impl crate::context::Context for Context {

fn instance_request_adapter(
&self,
options: &crate::RequestAdapterOptions<'_>,
options: &crate::RequestAdapterOptions<'_, '_>,
) -> Self::RequestAdapterFuture {
//TODO: support this check, return `None` if the flag is not set.
// It's not trivial, since we need the Future logic to have this check,
Expand Down
6 changes: 3 additions & 3 deletions wgpu/src/context.rs
Original file line number Diff line number Diff line change
Expand Up @@ -103,7 +103,7 @@ pub trait Context: Debug + WasmNotSend + WasmNotSync + Sized {
) -> Result<(Self::SurfaceId, Self::SurfaceData), crate::CreateSurfaceError>;
fn instance_request_adapter(
&self,
options: &RequestAdapterOptions<'_>,
options: &RequestAdapterOptions<'_, '_>,
) -> Self::RequestAdapterFuture;
fn adapter_request_device(
&self,
Expand Down Expand Up @@ -1239,7 +1239,7 @@ pub(crate) trait DynContext: Debug + WasmNotSend + WasmNotSync {
#[allow(clippy::type_complexity)]
fn instance_request_adapter(
&self,
options: &RequestAdapterOptions<'_>,
options: &RequestAdapterOptions<'_, '_>,
) -> Pin<InstanceRequestAdapterFuture>;
fn adapter_request_device(
&self,
Expand Down Expand Up @@ -2103,7 +2103,7 @@ where

fn instance_request_adapter(
&self,
options: &RequestAdapterOptions<'_>,
options: &RequestAdapterOptions<'_, '_>,
) -> Pin<InstanceRequestAdapterFuture> {
let future: T::RequestAdapterFuture = Context::instance_request_adapter(self, options);
Box::pin(async move {
Expand Down
Loading
Loading