Fix save to image on Windows (#32914)

* Read pixels in same format (gl::RGBA) as the texture

Signed-off-by: crbrz <cristianb@gmail.com>

* Add read pixels test

Signed-off-by: crbrz <cristianb@gmail.com>

* Use patched surfman

Signed-off-by: crbrz <cristianb@gmail.com>

* Update surfman to 0.9.5

Signed-off-by: crbrz <cristianb@gmail.com>

---------

Signed-off-by: crbrz <cristianb@gmail.com>

Cargo.lock

@@ -896,6 +896,7 @@ dependencies = [
  "servo_geometry",
  "servo_url",
  "style_traits",
+ "surfman",
  "time 0.1.45",
  "toml 0.5.11",
  "webrender",
@@ -5538,6 +5539,15 @@ dependencies = [
  "winapi-util",
 ]
 
+[[package]]
+name = "scc"
+version = "2.1.7"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "a870e34715d5d59c8536040d4d4e7a41af44d527dc50237036ba4090db7996fc"
+dependencies = [
+ "sdd",
+]
+
 [[package]]
 name = "scoped-tls"
 version = "1.0.1"
@@ -5761,6 +5771,12 @@ dependencies = [
  "tiny-skia",
 ]
 
+[[package]]
+name = "sdd"
+version = "2.1.0"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "177258b64c0faaa9ffd3c65cd3262c2bc7e2588dbbd9c1641d0346145c1bbda8"
+
 [[package]]
 name = "selectors"
 version = "0.24.0"
@@ -5859,6 +5875,31 @@ dependencies = [
  "serde",
 ]
 
+[[package]]
+name = "serial_test"
+version = "3.1.1"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "4b4b487fe2acf240a021cf57c6b2b4903b1e78ca0ecd862a71b71d2a51fed77d"
+dependencies = [
+ "futures 0.3.30",
+ "log",
+ "once_cell",
+ "parking_lot",
+ "scc",
+ "serial_test_derive",
+]
+
+[[package]]
+name = "serial_test_derive"
+version = "3.1.1"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "82fe9db325bcef1fbcde82e078a5cc4efdf787e96b3b9cf45b50b529f2083d67"
+dependencies = [
+ "proc-macro2",
+ "quote",
+ "syn 2.0.72",
+]
+
 [[package]]
 name = "servo-display-link"
 version = "0.2.0"
@@ -6577,9 +6618,9 @@ dependencies = [
 
 [[package]]
 name = "surfman"
-version = "0.9.4"
+version = "0.9.5"
 source = "registry+https://github.com/rust-lang/crates.io-index"
-checksum = "1ef31fb4346fc13b83872a73fa2a1f4856dbf3ab3eb3495b32fb41c26ec8cc7a"
+checksum = "2e0fa3767391402bf43c1dd4a70bb76f0573856748f7c240e9180994cc3bc3bb"
 dependencies = [
  "bitflags 1.3.2",
  "cfg_aliases 0.1.1",
@@ -6598,6 +6639,7 @@ dependencies = [
  "metal 0.24.0",
  "objc",
  "raw-window-handle",
+ "serial_test",
  "servo-display-link",
  "sparkle",
  "wayland-sys 0.30.1",

Cargo.toml

@@ -116,7 +116,7 @@ style = { git = "https://github.com/servo/stylo", branch = "2024-07-16", feature
 style_config = { git = "https://github.com/servo/stylo", branch = "2024-07-16" }
 style_dom = { git = "https://github.com/servo/stylo", package = "dom", branch = "2024-07-16" }
 style_traits = { git = "https://github.com/servo/stylo", branch = "2024-07-16", features = ["servo"] }
-surfman = { version = "0.9", features = ["chains"] }
+surfman = { version = "0.9.5", features = ["chains"] }
 syn = { version = "2", default-features = false, features = ["clone-impls", "derive", "parsing"] }
 synstructure = "0.13"
 thin-vec = "0.2.13"

components/compositing/Cargo.toml

@@ -46,5 +46,8 @@ webrender_api = { workspace = true }
 webrender_traits = { workspace = true }
 webxr = { git = "https://github.com/servo/webxr" }
 
+[dev-dependencies]
+surfman = { workspace = true }
+
 [build-dependencies]
 toml = "0.5"

components/compositing/compositor.rs

@@ -2246,7 +2246,7 @@ impl<Window: WindowMethods + ?Sized> IOCompositor<Window> {
                 Some(Image {
                     width: img.width(),
                     height: img.height(),
-                    format: PixelFormat::RGB8,
+                    format: PixelFormat::RGBA8,
                     bytes: ipc::IpcSharedMemory::from_bytes(&img),
                     id: None,
                     cors_status: CorsStatus::Safe,
@@ -2269,7 +2269,7 @@ impl<Window: WindowMethods + ?Sized> IOCompositor<Window> {
                                 FramebufferUintLength::new(width),
                                 FramebufferUintLength::new(height),
                             );
-                            let dynamic_image = DynamicImage::ImageRgb8(img);
+                            let dynamic_image = DynamicImage::ImageRgba8(img);
                             if let Err(e) = dynamic_image.write_to(&mut file, ImageFormat::Png) {
                                 error!("Failed to save {} ({}).", path, e);
                             }

components/compositing/gl.rs

@@ -5,8 +5,8 @@
 use std::rc::Rc;
 
 use gleam::gl::{self, Gl};
-use image::RgbImage;
+use image::RgbaImage;
-use log::trace;
+use log::{trace, warn};
 use servo_geometry::FramebufferUintLength;
 
 pub struct RenderTargetInfo {
@@ -105,7 +105,7 @@ impl RenderTargetInfo {
         y: i32,
         width: FramebufferUintLength,
         height: FramebufferUintLength,
-    ) -> RgbImage {
+    ) -> RgbaImage {
         let width = width.get() as usize;
         let height = height.get() as usize;
         // For some reason, OSMesa fails to render on the 3rd
@@ -121,13 +121,17 @@ impl RenderTargetInfo {
             y,
             width as gl::GLsizei,
             height as gl::GLsizei,
-            gl::RGB,
+            gl::RGBA,
             gl::UNSIGNED_BYTE,
         );
+        let gl_error = self.gl.get_error();
+        if gl_error != gl::NO_ERROR {
+            warn!("GL error code 0x{gl_error:x} set after read_pixels");
+        }
 
         // flip image vertically (texture is upside down)
         let orig_pixels = pixels.clone();
-        let stride = width * 3;
+        let stride = width * 4;
         for y in 0..height {
             let dst_start = y * stride;
             let src_start = (height - y - 1) * stride;
@@ -135,7 +139,7 @@ impl RenderTargetInfo {
             pixels[dst_start..dst_start + stride].clone_from_slice(&src_slice[..stride]);
         }
 
-        RgbImage::from_raw(width as u32, height as u32, pixels).expect("Flipping image failed!")
+        RgbaImage::from_raw(width as u32, height as u32, pixels).expect("Flipping image failed!")
     }
 }
 
@@ -148,3 +152,57 @@ impl Drop for RenderTargetInfo {
         self.gl.delete_framebuffers(&self.framebuffer_ids);
     }
 }
+
+#[cfg(test)]
+mod test {
+    use gleam::gl;
+    use image::Rgba;
+    use servo_geometry::FramebufferUintLength;
+    use surfman::{Connection, ContextAttributeFlags, ContextAttributes, Error, GLApi, GLVersion};
+
+    use super::RenderTargetInfo;
+
+    #[test]
+    #[allow(unsafe_code)]
+    fn test_read_pixels() -> Result<(), Error> {
+        let connection = Connection::new()?;
+        let adapter = connection.create_software_adapter()?;
+        let mut device = connection.create_device(&adapter)?;
+        let context_descriptor = device.create_context_descriptor(&ContextAttributes {
+            version: GLVersion::new(3, 0),
+            flags: ContextAttributeFlags::empty(),
+        })?;
+        let mut context = device.create_context(&context_descriptor, None)?;
+
+        let gl = match connection.gl_api() {
+            GLApi::GL => unsafe { gl::GlFns::load_with(|s| device.get_proc_address(&context, s)) },
+            GLApi::GLES => unsafe {
+                gl::GlesFns::load_with(|s| device.get_proc_address(&context, s))
+            },
+        };
+
+        device.make_context_current(&context)?;
+
+        {
+            const WIDTH: FramebufferUintLength = FramebufferUintLength::new(16);
+            const HEIGHT: FramebufferUintLength = FramebufferUintLength::new(16);
+            let render_target = RenderTargetInfo::new(gl, WIDTH, HEIGHT);
+            render_target.bind();
+            render_target
+                .gl
+                .clear_color(12.0 / 255.0, 34.0 / 255.0, 56.0 / 255.0, 78.0 / 255.0);
+            render_target.gl.clear(gl::COLOR_BUFFER_BIT);
+
+            let img = render_target.read_back_from_gpu(0, 0, WIDTH, HEIGHT);
+            assert_eq!(img.width(), WIDTH.get());
+            assert_eq!(img.height(), HEIGHT.get());
+
+            let expected_pixel: Rgba<u8> = Rgba([12, 34, 56, 78]);
+            assert!(img.pixels().all(|&p| p == expected_pixel));
+        }
+
+        device.destroy_context(&mut context)?;
+
+        Ok(())
+    }
+}