Merge gfx-rs#342

342: Fallback to D32 when D24 isn't available r=kvark a=grovesNL

The rationale is that D32 seems to be supported on more devices. Using D24 could be a future memory/performance optimization instead.

Otherwise if it's fairly trivial I could add the fallback path for D32 here instead, but I'm not sure how we intend for values in the `D24Unorm` depth range to work in general (i.e. compared to `D32Sfloat`). Maybe I missed some discussion about this, but I don't see it in the minutes.

cc @Yatekii 

Co-authored-by: Joshua Groves <josh@joshgroves.com>

wgpu-native/src/command/allocator.rs

@@ -1,5 +1,5 @@
 use super::CommandBuffer;
-use crate::{hub::GfxBackend, track::TrackerSet, DeviceId, LifeGuard, Stored, SubmissionIndex};
+use crate::{hub::GfxBackend, track::TrackerSet, DeviceId, Features, LifeGuard, Stored, SubmissionIndex};
 
 use hal::{command::CommandBuffer as _, device::Device as _, pool::CommandPool as _};
 use log::trace;
@@ -63,6 +63,7 @@ impl<B: GfxBackend> CommandAllocator<B> {
         &self,
         device_id: Stored<DeviceId>,
         device: &B::Device,
+        features: Features,
     ) -> CommandBuffer<B> {
         //debug_assert_eq!(device_id.backend(), B::VARIANT);
         let thread_id = thread::current().id();
@@ -88,6 +89,7 @@ impl<B: GfxBackend> CommandAllocator<B> {
             life_guard: LifeGuard::new(),
             trackers: TrackerSet::new(B::VARIANT),
             used_swap_chain: None,
+            features,
         }
     }
 

wgpu-native/src/command/mod.rs

@@ -30,6 +30,7 @@ use crate::{
     CommandEncoderId,
     ComputePassId,
     DeviceId,
+    Features,
     LifeGuard,
     RenderPassId,
     Stored,
@@ -119,6 +120,7 @@ pub struct CommandBuffer<B: hal::Backend> {
     pub(crate) life_guard: LifeGuard,
     pub(crate) trackers: TrackerSet,
     pub(crate) used_swap_chain: Option<(Stored<TextureViewId>, B::Framebuffer)>,
+    pub(crate) features: Features,
 }
 
 impl<B: GfxBackend> CommandBuffer<B> {
@@ -324,7 +326,7 @@ pub fn command_encoder_begin_render_pass<B: GfxBackend>(
                     }
                 };
                 hal::pass::Attachment {
-                    format: Some(conv::map_texture_format(view.format)),
+                    format: Some(conv::map_texture_format(view.format, device.features)),
                     samples: view.samples,
                     ops: conv::map_load_store_ops(at.depth_load_op, at.depth_store_op),
                     stencil_ops: conv::map_load_store_ops(at.stencil_load_op, at.stencil_store_op),
@@ -400,7 +402,7 @@ pub fn command_encoder_begin_render_pass<B: GfxBackend>(
                 };
 
                 colors.push(hal::pass::Attachment {
-                    format: Some(conv::map_texture_format(view.format)),
+                    format: Some(conv::map_texture_format(view.format, device.features)),
                     samples: view.samples,
                     ops: conv::map_load_store_ops(at.load_op, at.store_op),
                     stencil_ops: hal::pass::AttachmentOps::DONT_CARE,
@@ -472,7 +474,7 @@ pub fn command_encoder_begin_render_pass<B: GfxBackend>(
                 };
 
                 resolves.push(hal::pass::Attachment {
-                    format: Some(conv::map_texture_format(view.format)),
+                    format: Some(conv::map_texture_format(view.format, device.features)),
                     samples: view.samples,
                     ops: hal::pass::AttachmentOps::new(
                         hal::pass::AttachmentLoadOp::DontCare,

wgpu-native/src/command/transfer.rs

@@ -145,7 +145,6 @@ pub fn command_encoder_copy_buffer_to_texture<B: GfxBackend>(
 ) {
     let hub = B::hub();
     let mut token = Token::root();
-
     let (mut cmb_guard, mut token) = hub.command_buffers.write(&mut token);
     let cmb = &mut cmb_guard[command_encoder_id];
     let (buffer_guard, mut token) = hub.buffers.read(&mut token);
@@ -181,7 +180,7 @@ pub fn command_encoder_copy_buffer_to_texture<B: GfxBackend>(
     });
 
     let aspects = dst_texture.full_range.aspects;
-    let bytes_per_texel = conv::map_texture_format(dst_texture.format)
+    let bytes_per_texel = conv::map_texture_format(dst_texture.format, cmb.features)
         .surface_desc()
         .bits as u32
         / BITS_PER_BYTE;
@@ -271,7 +270,7 @@ pub fn command_encoder_copy_texture_to_buffer<B: GfxBackend>(
     });
 
     let aspects = src_texture.full_range.aspects;
-    let bytes_per_texel = conv::map_texture_format(src_texture.format)
+    let bytes_per_texel = conv::map_texture_format(src_texture.format, cmb.features)
         .surface_desc()
         .bits as u32
         / BITS_PER_BYTE;

wgpu-native/src/conv.rs

@@ -1,4 +1,4 @@
-use crate::{binding_model, command, pipeline, resource, Color, Extent3d, Origin3d};
+use crate::{binding_model, command, pipeline, resource, Color, Extent3d, Features, Origin3d};
 
 pub fn map_buffer_usage(
     usage: resource::BufferUsage,
@@ -307,7 +307,7 @@ fn map_stencil_operation(stencil_operation: pipeline::StencilOperation) -> hal::
     }
 }
 
-pub fn map_texture_format(texture_format: resource::TextureFormat) -> hal::format::Format {
+pub(crate) fn map_texture_format(texture_format: resource::TextureFormat, features: Features) -> hal::format::Format {
     use crate::resource::TextureFormat as Tf;
     use hal::format::Format as H;
     match texture_format {
@@ -367,8 +367,16 @@ pub fn map_texture_format(texture_format: resource::TextureFormat) -> hal::forma
 
         // Depth and stencil formats
         Tf::Depth32Float => H::D32Sfloat,
-        Tf::Depth24Plus => H::D24UnormS8Uint, //TODO: substitute
-        Tf::Depth24PlusStencil8 => H::D24UnormS8Uint, //TODO: substitute
+        Tf::Depth24Plus => if features.supports_texture_d24_s8 {
+            H::D24UnormS8Uint
+        } else {
+            H::D32Sfloat
+        }
+        Tf::Depth24PlusStencil8 => if features.supports_texture_d24_s8 {
+            H::D24UnormS8Uint
+        } else {
+            H::D32SfloatS8Uint
+        },
     }
 }
 

wgpu-native/src/device.rs

@@ -24,6 +24,7 @@ use crate::{
     CommandEncoderId,
     ComputePipelineId,
     DeviceId,
+    Features,
     LifeGuard,
     PipelineLayoutId,
     QueueId,
@@ -36,7 +37,9 @@ use crate::{
     SurfaceId,
     SwapChainId,
     TextureDimension,
+    TextureFormat,
     TextureId,
+    TextureUsage,
     TextureViewId,
 };
 
@@ -496,6 +499,7 @@ pub struct Device<B: hal::Backend> {
     pub(crate) render_passes: Mutex<FastHashMap<RenderPassKey, B::RenderPass>>,
     pub(crate) framebuffers: Mutex<FastHashMap<FramebufferKey, B::Framebuffer>>,
     pending: Mutex<PendingResources<B>>,
+    pub(crate) features: Features,
 }
 
 impl<B: GfxBackend> Device<B> {
@@ -504,6 +508,7 @@ impl<B: GfxBackend> Device<B> {
         adapter_id: AdapterId,
         queue_group: hal::queue::QueueGroup<B>,
         mem_props: hal::adapter::MemoryProperties,
+        supports_texture_d24_s8: bool,
     ) -> Self {
         // don't start submission index at zero
         let life_guard = LifeGuard::new();
@@ -549,6 +554,9 @@ impl<B: GfxBackend> Device<B> {
                 free: Vec::new(),
                 ready_to_map: Vec::new(),
             }),
+            features: Features {
+                supports_texture_d24_s8,
+            }
         }
     }
 
@@ -660,13 +668,22 @@ impl<B: GfxBackend> Device<B> {
         desc: &resource::TextureDescriptor,
     ) -> resource::Texture<B> {
         debug_assert_eq!(self_id.backend(), B::VARIANT);
+
+        // Ensure `D24Plus` textures cannot be copied
+        match desc.format {
+            TextureFormat::Depth24Plus | TextureFormat::Depth24PlusStencil8 => {
+                assert!(!desc.usage.intersects(TextureUsage::COPY_SRC | TextureUsage::COPY_DST));
+            }
+            _ => {}
+        }
+
         let kind = conv::map_texture_dimension_size(
             desc.dimension,
             desc.size,
             desc.array_layer_count,
             desc.sample_count,
         );
-        let format = conv::map_texture_format(desc.format);
+        let format = conv::map_texture_format(desc.format, self.features);
         let aspects = format.surface_desc().aspects;
         let usage = conv::map_texture_usage(desc.usage, aspects);
 
@@ -933,7 +950,7 @@ pub fn texture_create_view<B: GfxBackend>(
             .create_image_view(
                 &texture.raw,
                 view_kind,
-                conv::map_texture_format(format),
+                conv::map_texture_format(format, device.features),
                 hal::format::Swizzle::NO,
                 range.clone(),
             )
@@ -1425,7 +1442,7 @@ pub fn device_create_command_encoder<B: GfxBackend>(
         value: device_id,
         ref_count: device.life_guard.ref_count.clone(),
     };
-    let mut comb = device.com_allocator.allocate(dev_stored, &device.raw);
+    let mut comb = device.com_allocator.allocate(dev_stored, &device.raw, device.features);
     unsafe {
         comb.raw.last_mut().unwrap().begin(
             hal::command::CommandBufferFlags::ONE_TIME_SUBMIT,
@@ -1727,7 +1744,7 @@ pub fn device_create_render_pipeline<B: GfxBackend>(
             colors: color_states
                 .iter()
                 .map(|at| hal::pass::Attachment {
-                    format: Some(conv::map_texture_format(at.format)),
+                    format: Some(conv::map_texture_format(at.format, device.features)),
                     samples: sc,
                     ops: hal::pass::AttachmentOps::PRESERVE,
                     stencil_ops: hal::pass::AttachmentOps::DONT_CARE,
@@ -1740,7 +1757,7 @@ pub fn device_create_render_pipeline<B: GfxBackend>(
             // or depth/stencil resolve modes but satisfy the other compatibility conditions.
             resolves: ArrayVec::new(),
             depth_stencil: depth_stencil_state.map(|at| hal::pass::Attachment {
-                format: Some(conv::map_texture_format(at.format)),
+                format: Some(conv::map_texture_format(at.format, device.features)),
                 samples: sc,
                 ops: hal::pass::AttachmentOps::PRESERVE,
                 stencil_ops: hal::pass::AttachmentOps::PRESERVE,
@@ -1970,7 +1987,7 @@ pub fn device_create_swap_chain<B: GfxBackend>(
     let num_frames = swap_chain::DESIRED_NUM_FRAMES
         .max(*caps.image_count.start())
         .min(*caps.image_count.end());
-    let config = desc.to_hal(num_frames);
+    let config = desc.to_hal(num_frames, &device.features);
 
     if let Some(formats) = formats {
         assert!(

wgpu-native/src/instance.rs

@@ -75,7 +75,6 @@ pub struct Adapter<B: hal::Backend> {
     pub(crate) raw: hal::adapter::Adapter<B>,
 }
 
-
 #[repr(C)]
 #[derive(Copy, Clone, Debug, Hash, Eq, PartialEq)]
 #[cfg_attr(feature = "remote", derive(Serialize, Deserialize))]
@@ -459,11 +458,19 @@ pub fn adapter_request_device<B: GfxBackend>(
         );
 
         let mem_props = adapter.physical_device.memory_properties();
+
+        let supports_texture_d24_s8 = adapter
+            .physical_device
+            .format_properties(Some(hal::format::Format::D24UnormS8Uint))
+            .optimal_tiling
+            .contains(hal::format::ImageFeature::DEPTH_STENCIL_ATTACHMENT);
+
         Device::new(
             gpu.device,
             adapter_id,
             gpu.queue_groups.swap_remove(0),
             mem_props,
+            supports_texture_d24_s8,
         )
     };
 

wgpu-native/src/lib.rs

@@ -216,3 +216,8 @@ macro_rules! gfx_select {
         }
     };
 }
+
+#[derive(Clone, Copy, Debug)]
+pub(crate) struct Features {
+    pub supports_texture_d24_s8: bool,
+}

wgpu-native/src/swap_chain.rs

@@ -35,6 +35,7 @@ use crate::{
     resource,
     DeviceId,
     Extent3d,
+    Features,
     Input,
     LifeGuard,
     Stored,
@@ -83,11 +84,11 @@ pub struct SwapChainDescriptor {
 }
 
 impl SwapChainDescriptor {
-    pub(crate) fn to_hal(&self, num_frames: u32) -> hal::window::SwapchainConfig {
+    pub(crate) fn to_hal(&self, num_frames: u32, features: &Features) -> hal::window::SwapchainConfig {
         let mut config = hal::window::SwapchainConfig::new(
             self.width,
             self.height,
-            conv::map_texture_format(self.format),
+            conv::map_texture_format(self.format, *features),
             num_frames,
         );
         //TODO: check for supported
@@ -146,7 +147,7 @@ pub fn swap_chain_get_next_texture<B: GfxBackend>(
             }
             Err(e) => {
                 log::warn!("acquire_image() failed ({:?}), reconfiguring swapchain", e);
-                let desc = sc.desc.to_hal(sc.num_frames);
+                let desc = sc.desc.to_hal(sc.num_frames, &device.features);
                 unsafe {
                     suf
                         .configure_swapchain(&device.raw, desc)