Screenshots on OpenGL draw the frame where it can be read back

OpenGL swapchain images offer RENDER_ATTACHMENT only, so the surface is
configured without COPY_SRC, but the screenshot path copied the swapchain
image into the readback buffer anyway: wgpu's validation aborted the game
("Usage flags RENDER_ATTACHMENT ... do not contain required usage flags
COPY_SRC") on every screenshot under the GL backend.

A frame that is read back on such a surface is now drawn into a texture
that can be copied from, read back from there, and then drawn onto the
swapchain image (wgpu::util::TextureBlitter), so the window still shows
it. Other frames, and surfaces with COPY_SRC, draw straight to the
swapchain as before. The check in write_screenshot that refused every
readback on these surfaces goes with it.

Fixes #3510

Co-Authored-By: Claude Opus 5.5 <[email protected]>

crates/horizon-client-render/src/gfx.rs

@@ -91,6 +91,7 @@ pub mod pack_uniforms;
 mod pack_weather;
 mod page_runs;
 pub mod panorama;
+mod readback;
 mod resident;
 mod stars;
 mod timing;
@@ -561,6 +562,8 @@ pub struct Renderer {
     pub hud: HudRenderer,
     /// Vanilla's menu background blur, made on first use.
     menu_blur: Option<crate::menu_blur::MenuBlur>,
+    /// Where a screenshot's frame is drawn when the swapchain cannot be read back.
+    readback: readback::Readback,
     pub info: wgpu::AdapterInfo,
     pub suspended: bool,
     pub drawn_sections: usize,
@@ -2660,6 +2663,7 @@ impl Renderer {
             pack_terrain_flat: false,
             hud,
             menu_blur: None,
+            readback: readback::Readback::default(),
             info,
             suspended: size.width == 0 || size.height == 0,
             drawn_sections: 0,
@@ -3586,7 +3590,12 @@ impl Renderer {
         let size = [self.config.width, self.config.height];
         let target_texture = match (&surface_frame, &mut self.hdr) {
             (Some(_), Some(hdr)) => hdr.frame(&self.device, size),
-            (Some(f), None) => f.texture.clone(),
+            // A screenshot of a swapchain without COPY_SRC (OpenGL) draws
+            // into a texture that has it, then onto the swapchain.
+            (Some(f), None) => self
+                .readback
+                .target(&self.device, &f.texture, self.screenshot.is_some())
+                .unwrap_or_else(|| f.texture.clone()),
             (None, _) => self.offscreen.clone().ok_or("no offscreen target")?,
         };
         // One command buffer for the frame, after the section copies staged
@@ -5193,7 +5202,11 @@ impl Renderer {
             // The copy and PNG writer both assume 8-bit pixels; a float surface would make
             // the row stride too small for the copy and abort in validation.
             if self.config.format.block_copy_size(None) == Some(4) {
-                Some((path, self.copy_frame(&mut encoder, &target_texture)))
+                let size = [self.config.width, self.config.height];
+                Some((
+                    path,
+                    copy_frame(&self.device, &mut encoder, &target_texture, size),
+                ))
             } else {
                 self.screenshot_result = Some(Err(format!(
                     "screenshots need a 32-bit surface format, not {:?}",
@@ -5207,6 +5220,10 @@ impl Renderer {
             self.section_copy_bytes = 0;
             self.section_uploads.finish(&encoder);
         }
+        if let Some(f) = &surface_frame {
+            self.readback
+                .present(&self.device, &mut encoder, &f.texture);
+        }
         if let (Some(hdr), Some(f)) = (&self.hdr, &surface_frame) {
             hdr.present(&self.queue, &mut encoder, &f.texture, !frame.title);
         }
@@ -5279,43 +5296,43 @@ impl Renderer {
         Ok(true)
     }
 }
+/// Bytes per row of a `width`-pixel frame read back at 4 bytes a pixel.
+fn padded_row(width: u32) -> u32 {
+    (width * 4).next_multiple_of(wgpu::COPY_BYTES_PER_ROW_ALIGNMENT)
+}
+/// Encodes the copy of `texture`'s `size` pixels into a mappable buffer.
+fn copy_frame(
+    device: &wgpu::Device,
+    encoder: &mut wgpu::CommandEncoder,
+    texture: &wgpu::Texture,
+    [width, height]: [u32; 2],
+) -> wgpu::Buffer {
+    let buffer = device.create_buffer(&wgpu::BufferDescriptor {
+        label: Some("Horizon screenshot"),
+        size: u64::from(padded_row(width)) * u64::from(height),
+        usage: wgpu::BufferUsages::COPY_DST | wgpu::BufferUsages::MAP_READ,
+        mapped_at_creation: false,
+    });
+    encoder.copy_texture_to_buffer(
+        texture.as_image_copy(),
+        wgpu::TexelCopyBufferInfo {
+            buffer: &buffer,
+            layout: wgpu::TexelCopyBufferLayout {
+                offset: 0,
+                bytes_per_row: Some(padded_row(width)),
+                rows_per_image: None,
+            },
+        },
+        wgpu::Extent3d {
+            width,
+            height,
+            depth_or_array_layers: 1,
+        },
+    );
+    buffer
+}
 impl Renderer {
-    fn padded_row(&self) -> u32 {
-        (self.config.width * 4).next_multiple_of(wgpu::COPY_BYTES_PER_ROW_ALIGNMENT)
-    }
-    fn copy_frame(
-        &self,
-        encoder: &mut wgpu::CommandEncoder,
-        texture: &wgpu::Texture,
-    ) -> wgpu::Buffer {
-        let buffer = self.device.create_buffer(&wgpu::BufferDescriptor {
-            label: Some("Horizon screenshot"),
-            size: u64::from(self.padded_row()) * u64::from(self.config.height),
-            usage: wgpu::BufferUsages::COPY_DST | wgpu::BufferUsages::MAP_READ,
-            mapped_at_creation: false,
-        });
-        encoder.copy_texture_to_buffer(
-            texture.as_image_copy(),
-            wgpu::TexelCopyBufferInfo {
-                buffer: &buffer,
-                layout: wgpu::TexelCopyBufferLayout {
-                    offset: 0,
-                    bytes_per_row: Some(self.padded_row()),
-                    rows_per_image: None,
-                },
-            },
-            wgpu::Extent3d {
-                width: self.config.width,
-                height: self.config.height,
-                depth_or_array_layers: 1,
-            },
-        );
-        buffer
-    }
     fn write_screenshot(&self, path: &std::path::Path, buffer: &wgpu::Buffer) -> Result<[u32; 2]> {
-        if !self.config.usage.contains(wgpu::TextureUsages::COPY_SRC) {
-            return Err("this surface does not support frame readback".into());
-        }
         let (sender, receiver) = std::sync::mpsc::channel();
         buffer.map_async(wgpu::MapMode::Read, .., move |r| {
             let _ = sender.send(r);
@@ -5330,7 +5347,7 @@ impl Renderer {
         let (width, height, row) = (
             self.config.width,
             self.config.height,
-            self.padded_row() as usize,
+            padded_row(self.config.width) as usize,
         );
         let bgra = matches!(
             self.config.format.remove_srgb_suffix(),

crates/horizon-client-render/src/gfx/readback.rs

@@ -0,0 +1,213 @@
+//! Screenshots of a swapchain that cannot be copied from. OpenGL's surfaces
+//! offer `RENDER_ATTACHMENT` only, so a frame whose pixels are read back
+//! (the screenshot key, a world's icon) is drawn into a texture that can be,
+//! then drawn onto the swapchain image as a fullscreen triangle.
+
+/// The texture a frame to read back is drawn into, and the pass that puts it
+/// on the swapchain afterwards.
+#[derive(Default)]
+pub struct Readback {
+    texture: Option<wgpu::Texture>,
+    blitter: Option<(wgpu::TextureFormat, wgpu::util::TextureBlitter)>,
+    /// A frame drew into `texture` and still has to reach the swapchain.
+    drawn: bool,
+}
+
+impl Readback {
+    /// The texture this frame draws into instead of the swapchain image
+    /// `surface`: `None` unless the frame is read back (`screenshot`) and the
+    /// image cannot be copied from.
+    pub fn target(
+        &mut self,
+        device: &wgpu::Device,
+        surface: &wgpu::Texture,
+        screenshot: bool,
+    ) -> Option<wgpu::Texture> {
+        if !screenshot || surface.usage().contains(wgpu::TextureUsages::COPY_SRC) {
+            return None;
+        }
+        let format = surface.format().remove_srgb_suffix();
+        if self
+            .texture
+            .as_ref()
+            .is_none_or(|t| t.size() != surface.size() || t.format() != format)
+        {
+            self.texture = Some(device.create_texture(&wgpu::TextureDescriptor {
+                label: Some("Horizon screenshot frame"),
+                size: surface.size(),
+                mip_level_count: 1,
+                sample_count: 1,
+                dimension: wgpu::TextureDimension::D2,
+                format,
+                usage: wgpu::TextureUsages::RENDER_ATTACHMENT
+                    | wgpu::TextureUsages::COPY_SRC
+                    | wgpu::TextureUsages::TEXTURE_BINDING,
+                view_formats: &[],
+            }));
+        }
+        self.drawn = true;
+        self.texture.clone()
+    }
+
+    /// Draws the frame `target` gave onto the swapchain image `surface`; nothing
+    /// when this frame drew straight into it.
+    pub fn present(
+        &mut self,
+        device: &wgpu::Device,
+        encoder: &mut wgpu::CommandEncoder,
+        surface: &wgpu::Texture,
+    ) {
+        if !std::mem::take(&mut self.drawn) {
+            return;
+        }
+        let Some(texture) = &self.texture else {
+            return;
+        };
+        // The frame's passes write the swapchain through its non-sRGB view
+        // (`view_formats`), so the copy goes there unconverted too.
+        let format = surface.format().remove_srgb_suffix();
+        if self.blitter.as_ref().is_none_or(|b| b.0 != format) {
+            self.blitter = Some((format, wgpu::util::TextureBlitter::new(device, format)));
+        }
+        let target = surface.create_view(&wgpu::TextureViewDescriptor {
+            format: Some(format),
+            ..Default::default()
+        });
+        let source = texture.create_view(&Default::default());
+        if let Some((_, blitter)) = &self.blitter {
+            blitter.copy(device, encoder, &source, &target);
+        }
+    }
+}
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+
+    fn device() -> Option<(wgpu::Device, wgpu::Queue)> {
+        let instance = wgpu::Instance::new(wgpu::InstanceDescriptor::new_without_display_handle());
+        let adapter = pollster::block_on(instance.request_adapter(&Default::default())).ok()?;
+        pollster::block_on(adapter.request_device(&Default::default())).ok()
+    }
+
+    fn texture(device: &wgpu::Device, usage: wgpu::TextureUsages) -> wgpu::Texture {
+        device.create_texture(&wgpu::TextureDescriptor {
+            label: None,
+            size: wgpu::Extent3d {
+                width: 70,
+                height: 3,
+                depth_or_array_layers: 1,
+            },
+            mip_level_count: 1,
+            sample_count: 1,
+            dimension: wgpu::TextureDimension::D2,
+            format: wgpu::TextureFormat::Bgra8Unorm,
+            usage,
+            view_formats: &[],
+        })
+    }
+
+    fn clear(encoder: &mut wgpu::CommandEncoder, texture: &wgpu::Texture, color: wgpu::Color) {
+        let view = texture.create_view(&Default::default());
+        encoder.begin_render_pass(&wgpu::RenderPassDescriptor {
+            label: None,
+            color_attachments: &[Some(wgpu::RenderPassColorAttachment {
+                view: &view,
+                resolve_target: None,
+                depth_slice: None,
+                ops: wgpu::Operations {
+                    load: wgpu::LoadOp::Clear(color),
+                    store: wgpu::StoreOp::Store,
+                },
+            })],
+            ..Default::default()
+        });
+    }
+
+    fn read(device: &wgpu::Device, buffer: &wgpu::Buffer) -> Vec<u8> {
+        buffer.map_async(wgpu::MapMode::Read, .., |r| r.unwrap());
+        device.poll(wgpu::PollType::wait_indefinitely()).unwrap();
+        buffer.get_mapped_range(..).unwrap().to_vec()
+    }
+
+    /// The last pixel of the last row of a 70x3 frame read back by `copy_frame`.
+    fn last_pixel(device: &wgpu::Device, buffer: &wgpu::Buffer) -> Vec<u8> {
+        let pixels = read(device, buffer);
+        let row = 70usize * 4;
+        let last = row.next_multiple_of(256) * 2 + row - 4;
+        pixels[last..last + 4].to_vec()
+    }
+
+    /// The renderer's screenshot path on a surface like OpenGL's (#3510): the
+    /// frame is drawn into `target`, read back with `copy_frame` and drawn on
+    /// the swapchain, with no validation error (the copy used to need the
+    /// swapchain's missing `COPY_SRC`), and the swapchain shows the frame.
+    #[test]
+    fn screenshot_of_a_surface_without_copy_src() {
+        let Some((device, queue)) = device() else {
+            return;
+        };
+        // Sampled only so the test can look at what reached it.
+        let surface = texture(
+            &device,
+            wgpu::TextureUsages::RENDER_ATTACHMENT | wgpu::TextureUsages::TEXTURE_BINDING,
+        );
+        let check = texture(
+            &device,
+            wgpu::TextureUsages::RENDER_ATTACHMENT | wgpu::TextureUsages::COPY_SRC,
+        );
+        let blitter = wgpu::util::TextureBlitter::new(&device, wgpu::TextureFormat::Bgra8Unorm);
+        let mut readback = Readback::default();
+        let colors = [
+            (1.0, [0, 0, 255, 255]),
+            // A second screenshot reuses the texture.
+            (0.0, [255, 0, 0, 255]),
+        ];
+        for (r, bgra) in colors {
+            let color = wgpu::Color {
+                r,
+                g: 0.0,
+                b: 1.0 - r,
+                a: 1.0,
+            };
+            let scope = device.push_error_scope(wgpu::ErrorFilter::Validation);
+            let mut encoder = device.create_command_encoder(&Default::default());
+            let frame = readback
+                .target(&device, &surface, true)
+                .unwrap_or_else(|| surface.clone());
+            clear(&mut encoder, &frame, color);
+            let buffer = crate::gfx::copy_frame(&device, &mut encoder, &frame, [70, 3]);
+            readback.present(&device, &mut encoder, &surface);
+            queue.submit([encoder.finish()]);
+            if let Some(error) = pollster::block_on(scope.pop()) {
+                panic!("{error}");
+            }
+            assert_eq!(last_pixel(&device, &buffer), bgra);
+            let mut encoder = device.create_command_encoder(&Default::default());
+            let view = |t: &wgpu::Texture| t.create_view(&Default::default());
+            blitter.copy(&device, &mut encoder, &view(&surface), &view(&check));
+            let buffer = crate::gfx::copy_frame(&device, &mut encoder, &check, [70, 3]);
+            queue.submit([encoder.finish()]);
+            assert_eq!(last_pixel(&device, &buffer), bgra);
+        }
+        assert!(!readback.drawn);
+    }
+
+    /// A surface that can be copied from, or a frame nobody reads back,
+    /// draws straight into the swapchain image.
+    #[test]
+    fn frames_draw_straight_to_a_readable_surface() {
+        let Some((device, _)) = device() else {
+            return;
+        };
+        let mut readback = Readback::default();
+        let readable = texture(
+            &device,
+            wgpu::TextureUsages::RENDER_ATTACHMENT | wgpu::TextureUsages::COPY_SRC,
+        );
+        assert!(readback.target(&device, &readable, true).is_none());
+        let surface = texture(&device, wgpu::TextureUsages::RENDER_ATTACHMENT);
+        assert!(readback.target(&device, &surface, false).is_none());
+        assert!(readback.texture.is_none() && !readback.drawn);
+    }
+}