Merge #763

763: Remove raw pointers from the render pipelines API r=kvark a=GabrielMajeri

**Connections**
Rust-ification of API, as part of #689

**Description**
The objective is to get rid of raw pointers in the render pipeline descriptor and associated structures.

**Testing**
Checked with `player` and the `trace` feature, and with `wgpu-rs` with the changes in https://github.com/gfx-rs/wgpu-rs/pull/425

Co-authored-by: Gabriel Majeri <gabriel.majeri6@gmail.com>

player/src/main.rs

@@ -55,25 +55,6 @@ impl Label {
     }
 }
 
-struct OwnedProgrammableStage {
-    desc: wgc::pipeline::ProgrammableStageDescriptor,
-    #[allow(dead_code)]
-    entry_point: CString,
-}
-
-impl From<trace::ProgrammableStageDescriptor> for OwnedProgrammableStage {
-    fn from(stage: trace::ProgrammableStageDescriptor) -> Self {
-        let entry_point = CString::new(stage.entry_point.as_str()).unwrap();
-        OwnedProgrammableStage {
-            desc: wgc::pipeline::ProgrammableStageDescriptor {
-                module: stage.module,
-                entry_point: entry_point.as_ptr(),
-            },
-            entry_point,
-        }
-    }
-}
-
 #[derive(Debug)]
 struct IdentityPassThrough<I>(PhantomData<I>);
 
@@ -311,13 +292,13 @@ impl GlobalExt for wgc::hub::Global<IdentityPassThroughFactory> {
                 self.shader_module_destroy::<B>(id);
             }
             A::CreateComputePipeline { id, desc } => {
-                let cs_stage = OwnedProgrammableStage::from(desc.compute_stage);
+                let compute_stage = desc.compute_stage.to_core();
                 self.device_maintain_ids::<B>(device);
                 self.device_create_compute_pipeline::<B>(
                     device,
                     &wgc::pipeline::ComputePipelineDescriptor {
                         layout: desc.layout,
-                        compute_stage: cs_stage.desc,
+                        compute_stage,
                     },
                     id,
                 )
@@ -327,17 +308,16 @@ impl GlobalExt for wgc::hub::Global<IdentityPassThroughFactory> {
                 self.compute_pipeline_destroy::<B>(id);
             }
             A::CreateRenderPipeline { id, desc } => {
-                let vs_stage = OwnedProgrammableStage::from(desc.vertex_stage);
+                let vertex_stage = desc.vertex_stage.to_core();
-                let fs_stage = desc.fragment_stage.map(OwnedProgrammableStage::from);
+                let fragment_stage = desc.fragment_stage.as_ref().map(|fs| fs.to_core());
                 let vertex_buffers = desc
                     .vertex_state
                     .vertex_buffers
                     .iter()
-                    .map(|vb| wgc::pipeline::VertexBufferLayoutDescriptor {
+                    .map(|vb| wgt::VertexBufferDescriptor {
-                        array_stride: vb.array_stride,
+                        stride: vb.stride,
                         step_mode: vb.step_mode,
-                        attributes: vb.attributes.as_ptr(),
+                        attributes: &vb.attributes,
-                        attributes_length: vb.attributes.len(),
                     })
                     .collect::<Vec<_>>();
                 self.device_maintain_ids::<B>(device);
@@ -345,23 +325,15 @@ impl GlobalExt for wgc::hub::Global<IdentityPassThroughFactory> {
                     device,
                     &wgc::pipeline::RenderPipelineDescriptor {
                         layout: desc.layout,
-                        vertex_stage: vs_stage.desc,
+                        vertex_stage,
-                        fragment_stage: fs_stage.as_ref().map_or(ptr::null(), |s| &s.desc),
+                        fragment_stage,
                         primitive_topology: desc.primitive_topology,
-                        rasterization_state: desc
+                        rasterization_state: desc.rasterization_state,
-                            .rasterization_state
+                        color_states: &desc.color_states,
-                            .as_ref()
+                        depth_stencil_state: desc.depth_stencil_state,
-                            .map_or(ptr::null(), |rs| rs),
+                        vertex_state: wgt::VertexStateDescriptor {
-                        color_states: desc.color_states.as_ptr(),
-                        color_states_length: desc.color_states.len(),
-                        depth_stencil_state: desc
-                            .depth_stencil_state
-                            .as_ref()
-                            .map_or(ptr::null(), |ds| ds),
-                        vertex_state: wgc::pipeline::VertexStateDescriptor {
                             index_format: desc.vertex_state.index_format,
-                            vertex_buffers: vertex_buffers.as_ptr(),
+                            vertex_buffers: &vertex_buffers,
-                            vertex_buffers_length: vertex_buffers.len(),
                         },
                         sample_count: desc.sample_count,
                         sample_mask: desc.sample_mask,

wgpu-core/src/device/mod.rs

@@ -2062,44 +2062,37 @@ impl<G: GlobalIdentityHandlerFactory> Global<G> {
             sc as u8
         };
 
-        let color_states =
+        let color_states = desc.color_states;
-            unsafe { slice::from_raw_parts(desc.color_states, desc.color_states_length) };
+        let depth_stencil_state = desc.depth_stencil_state.as_ref();
-        let depth_stencil_state = unsafe { desc.depth_stencil_state.as_ref() };
 
-        let rasterization_state = unsafe { desc.rasterization_state.as_ref() }.cloned();
+        let rasterization_state = desc.rasterization_state.as_ref();
         let rasterizer = conv::map_rasterization_state_descriptor(
-            &rasterization_state.clone().unwrap_or_default(),
+            &rasterization_state.cloned().unwrap_or_default(),
         );
 
         let mut interface = validation::StageInterface::default();
         let mut validated_stages = wgt::ShaderStage::empty();
 
-        let desc_vbs = unsafe {
+        let desc_vbs = desc.vertex_state.vertex_buffers;
-            slice::from_raw_parts(
-                desc.vertex_state.vertex_buffers,
-                desc.vertex_state.vertex_buffers_length,
-            )
-        };
         let mut vertex_strides = Vec::with_capacity(desc_vbs.len());
         let mut vertex_buffers = Vec::with_capacity(desc_vbs.len());
         let mut attributes = Vec::new();
         for (i, vb_state) in desc_vbs.iter().enumerate() {
             vertex_strides
                 .alloc()
-                .init((vb_state.array_stride, vb_state.step_mode));
+                .init((vb_state.stride, vb_state.step_mode));
-            if vb_state.attributes_length == 0 {
+            if vb_state.attributes.is_empty() {
                 continue;
             }
             vertex_buffers.alloc().init(hal::pso::VertexBufferDesc {
                 binding: i as u32,
-                stride: vb_state.array_stride as u32,
+                stride: vb_state.stride as u32,
                 rate: match vb_state.step_mode {
                     InputStepMode::Vertex => hal::pso::VertexInputRate::Vertex,
                     InputStepMode::Instance => hal::pso::VertexInputRate::Instance(1),
                 },
             });
-            let desc_atts =
+            let desc_atts = vb_state.attributes;
-                unsafe { slice::from_raw_parts(vb_state.attributes, vb_state.attributes_length) };
             for attribute in desc_atts {
                 if attribute.offset >= 0x10000000 {
                     return Err(
@@ -2213,11 +2206,7 @@ impl<G: GlobalIdentityHandlerFactory> Global<G> {
             };
 
             let vertex = {
-                let entry_point_name =
+                let entry_point_name = desc.vertex_stage.entry_point;
-                    unsafe { ffi::CStr::from_ptr(desc.vertex_stage.entry_point) }
-                        .to_str()
-                        .to_owned()
-                        .unwrap();
 
                 let shader_module = &shader_module_guard[desc.vertex_stage.module];
 
@@ -2241,12 +2230,9 @@ impl<G: GlobalIdentityHandlerFactory> Global<G> {
                 }
             };
 
-            let fragment = match unsafe { desc.fragment_stage.as_ref() } {
+            let fragment = match &desc.fragment_stage {
                 Some(stage) => {
-                    let entry_point_name = unsafe { ffi::CStr::from_ptr(stage.entry_point) }
+                    let entry_point_name = stage.entry_point;
-                        .to_str()
-                        .to_owned()
-                        .unwrap();
 
                     let shader_module = &shader_module_guard[stage.module];
 
@@ -2344,7 +2330,9 @@ impl<G: GlobalIdentityHandlerFactory> Global<G> {
             attachments: AttachmentData {
                 colors: color_states.iter().map(|state| state.format).collect(),
                 resolves: ArrayVec::new(),
-                depth_stencil: depth_stencil_state.map(|state| state.format),
+                depth_stencil: depth_stencil_state
+                    .as_ref()
+                    .map(|state| state.format.clone()),
             },
             sample_count: samples,
         };
@@ -2355,7 +2343,7 @@ impl<G: GlobalIdentityHandlerFactory> Global<G> {
                 flags |= pipeline::PipelineFlags::BLEND_COLOR;
             }
         }
-        if let Some(ds) = depth_stencil_state {
+        if let Some(ds) = depth_stencil_state.as_ref() {
             if ds.needs_stencil_reference() {
                 flags |= pipeline::PipelineFlags::STENCIL_REFERENCE;
             }
@@ -2392,25 +2380,22 @@ impl<G: GlobalIdentityHandlerFactory> Global<G> {
                 desc: trace::RenderPipelineDescriptor {
                     layout: desc.layout,
                     vertex_stage: trace::ProgrammableStageDescriptor::new(&desc.vertex_stage),
-                    fragment_stage: unsafe { desc.fragment_stage.as_ref() }
+                    fragment_stage: desc
+                        .fragment_stage
+                        .as_ref()
                         .map(trace::ProgrammableStageDescriptor::new),
                     primitive_topology: desc.primitive_topology,
-                    rasterization_state,
+                    rasterization_state: rasterization_state.cloned(),
                     color_states: color_states.to_vec(),
                     depth_stencil_state: depth_stencil_state.cloned(),
                     vertex_state: trace::VertexStateDescriptor {
                         index_format: desc.vertex_state.index_format,
                         vertex_buffers: desc_vbs
                             .iter()
-                            .map(|vbl| trace::VertexBufferLayoutDescriptor {
+                            .map(|vbl| trace::VertexBufferDescriptor {
-                                array_stride: vbl.array_stride,
+                                stride: vbl.stride,
                                 step_mode: vbl.step_mode,
-                                attributes: unsafe {
+                                attributes: vbl.attributes.to_owned(),
-                                    slice::from_raw_parts(vbl.attributes, vbl.attributes_length)
-                                }
-                                .iter()
-                                .cloned()
-                                .collect(),
                             })
                             .collect(),
                     },
@@ -2475,10 +2460,7 @@ impl<G: GlobalIdentityHandlerFactory> Global<G> {
             let pipeline_stage = &desc.compute_stage;
             let (shader_module_guard, _) = hub.shader_modules.read(&mut token);
 
-            let entry_point_name = unsafe { ffi::CStr::from_ptr(pipeline_stage.entry_point) }
+            let entry_point_name = pipeline_stage.entry_point;
-                .to_str()
-                .to_owned()
-                .unwrap();
 
             let shader_module = &shader_module_guard[pipeline_stage.module];
 

wgpu-core/src/device/trace.rs

@@ -40,9 +40,17 @@ impl ProgrammableStageDescriptor {
     pub fn new(desc: &crate::pipeline::ProgrammableStageDescriptor) -> Self {
         ProgrammableStageDescriptor {
             module: desc.module,
-            entry_point: unsafe { std::ffi::CStr::from_ptr(desc.entry_point) }
+            entry_point: desc.entry_point.to_string(),
-                .to_string_lossy()
+        }
-                .to_string(),
+    }
+}
+
+#[cfg(feature = "replay")]
+impl ProgrammableStageDescriptor {
+    pub fn to_core(&self) -> crate::pipeline::ProgrammableStageDescriptor {
+        crate::pipeline::ProgrammableStageDescriptor {
+            module: self.module,
+            entry_point: &self.entry_point,
         }
     }
 }
@@ -58,8 +66,8 @@ pub struct ComputePipelineDescriptor {
 #[derive(Debug)]
 #[cfg_attr(feature = "trace", derive(serde::Serialize))]
 #[cfg_attr(feature = "replay", derive(serde::Deserialize))]
-pub struct VertexBufferLayoutDescriptor {
+pub struct VertexBufferDescriptor {
-    pub array_stride: wgt::BufferAddress,
+    pub stride: wgt::BufferAddress,
     pub step_mode: wgt::InputStepMode,
     pub attributes: Vec<wgt::VertexAttributeDescriptor>,
 }
@@ -69,7 +77,7 @@ pub struct VertexBufferLayoutDescriptor {
 #[cfg_attr(feature = "replay", derive(serde::Deserialize))]
 pub struct VertexStateDescriptor {
     pub index_format: wgt::IndexFormat,
-    pub vertex_buffers: Vec<VertexBufferLayoutDescriptor>,
+    pub vertex_buffers: Vec<VertexBufferDescriptor>,
 }
 
 #[derive(Debug)]

wgpu-core/src/pipeline.rs

@@ -6,31 +6,14 @@ use crate::{
     device::RenderPassContext,
     id::{DeviceId, PipelineLayoutId, ShaderModuleId},
     validation::StageError,
-    LifeGuard, RawString, RefCount, Stored,
+    LifeGuard, RefCount, Stored,
 };
 use std::borrow::Borrow;
 use wgt::{
     BufferAddress, ColorStateDescriptor, DepthStencilStateDescriptor, IndexFormat, InputStepMode,
-    PrimitiveTopology, RasterizationStateDescriptor, VertexAttributeDescriptor,
+    PrimitiveTopology, RasterizationStateDescriptor, VertexStateDescriptor,
 };
 
-#[repr(C)]
-#[derive(Debug)]
-pub struct VertexBufferLayoutDescriptor {
-    pub array_stride: BufferAddress,
-    pub step_mode: InputStepMode,
-    pub attributes: *const VertexAttributeDescriptor,
-    pub attributes_length: usize,
-}
-
-#[repr(C)]
-#[derive(Debug)]
-pub struct VertexStateDescriptor {
-    pub index_format: IndexFormat,
-    pub vertex_buffers: *const VertexBufferLayoutDescriptor,
-    pub vertex_buffers_length: usize,
-}
-
 #[repr(C)]
 #[derive(Debug)]
 pub enum ShaderModuleSource<'a> {
@@ -46,18 +29,17 @@ pub struct ShaderModule<B: hal::Backend> {
     pub(crate) module: Option<naga::Module>,
 }
 
-#[repr(C)]
 #[derive(Debug)]
-pub struct ProgrammableStageDescriptor {
+pub struct ProgrammableStageDescriptor<'a> {
     pub module: ShaderModuleId,
-    pub entry_point: RawString,
+    pub entry_point: &'a str,
 }
 
 #[repr(C)]
 #[derive(Debug)]
-pub struct ComputePipelineDescriptor {
+pub struct ComputePipelineDescriptor<'a> {
     pub layout: PipelineLayoutId,
-    pub compute_stage: ProgrammableStageDescriptor,
+    pub compute_stage: ProgrammableStageDescriptor<'a>,
 }
 
 #[derive(Clone, Debug)]
@@ -79,18 +61,16 @@ impl<B: hal::Backend> Borrow<RefCount> for ComputePipeline<B> {
     }
 }
 
-#[repr(C)]
 #[derive(Debug)]
-pub struct RenderPipelineDescriptor {
+pub struct RenderPipelineDescriptor<'a> {
     pub layout: PipelineLayoutId,
-    pub vertex_stage: ProgrammableStageDescriptor,
+    pub vertex_stage: ProgrammableStageDescriptor<'a>,
-    pub fragment_stage: *const ProgrammableStageDescriptor,
+    pub fragment_stage: Option<ProgrammableStageDescriptor<'a>>,
     pub primitive_topology: PrimitiveTopology,
-    pub rasterization_state: *const RasterizationStateDescriptor,
+    pub rasterization_state: Option<RasterizationStateDescriptor>,
-    pub color_states: *const ColorStateDescriptor,
+    pub color_states: &'a [ColorStateDescriptor],
-    pub color_states_length: usize,
+    pub depth_stencil_state: Option<DepthStencilStateDescriptor>,
-    pub depth_stencil_state: *const DepthStencilStateDescriptor,
+    pub vertex_state: VertexStateDescriptor<'a>,
-    pub vertex_state: VertexStateDescriptor,
     pub sample_count: u32,
     pub sample_mask: u32,
     pub alpha_to_coverage_enabled: bool,

wgpu-types/src/lib.rs

@@ -813,6 +813,26 @@ pub struct VertexAttributeDescriptor {
     pub shader_location: ShaderLocation,
 }
 
+/// Describes how the vertex buffer is interpreted.
+#[derive(Copy, Clone, Debug, Hash, Eq, PartialEq)]
+pub struct VertexBufferDescriptor<'a> {
+    /// The stride, in bytes, between elements of this buffer.
+    pub stride: BufferAddress,
+    /// How often this vertex buffer is "stepped" forward.
+    pub step_mode: InputStepMode,
+    /// The list of attributes which comprise a single vertex.
+    pub attributes: &'a [VertexAttributeDescriptor],
+}
+
+/// Describes vertex input state for a render pipeline.
+#[derive(Copy, Clone, Debug, Hash, Eq, PartialEq)]
+pub struct VertexStateDescriptor<'a> {
+    /// The format of any index buffers used with this pipeline.
+    pub index_format: IndexFormat,
+    /// The format of any vertex buffers used with this pipeline.
+    pub vertex_buffers: &'a [VertexBufferDescriptor<'a>],
+}
+
 /// Vertex Format for a Vertex Attribute (input).
 #[repr(C)]
 #[derive(Copy, Clone, Debug, Hash, Eq, PartialEq)]