rust-windowing / softbuffer

Easily write an image to a window
Apache License 2.0
282 stars 44 forks source link

X11: Wrong behaviour for "weird" visuals? #184

Closed psychon closed 6 months ago

psychon commented 6 months ago

According to the docs, pixels are represented as:

https://github.com/rust-windowing/softbuffer/blob/64dad395368388263c337a468f77c808c35a7f9e/src/lib.rs#L371-L378

However, I cannot find anything in src/x11.rs that actually ensures this. Any visual will do.

For my currently running X11 server, all visuals actually seem to use this format:

$ xdpyinfo| grep red,\ g | uniq  
    red, green, blue masks:    0xff0000, 0xff00, 0xff

However, not all of these are 24 bit depth:

$ xdpyinfo| grep depth: | uniq
    depth:    24 planes
    depth:    32 planes

The last one of these is actually ARGB (and I bet many others, too; I only checked this one).

$ xdpyinfo | grep -B 2 'depth:    32' | tail -n 3
    visual id:    0x767
    class:    TrueColor
    depth:    32 planes
$ xdpyinfo -ext RENDER | grep -1 0x767 | tail -n 3
      visual format:
        visual id:      0x767
        pict format id: 0x25
$ xdpyinfo -ext RENDER | grep -A 5 'format id:    0x25' 
    format id:    0x25
    type:         Direct
    depth:        32
    alpha:        24 mask 0xff
    red:          16 mask 0xff
    green:         8 mask 0xff

How should softbuffer handle these? Should it reject any visual that does not match its expectation? Should it just ignore this and hope for the best? Should it try to actively convert between visuals (sloooooooow)?

This might also be endianness-related, so #109. I don't actually know what big endian X11 servers usually do here. Worse, what happens to a client running on a big-endian machine and talking to a little endian server (or vice-versa)?

psychon commented 6 months ago

Patch to the winit example that makes it print some pixel values that are invalid according to the docs. Output for me is

Using visual 7a of class TRUE_COLOR, 8 bits per rgb, masks ff0000, ff00, ff
Some pixel values: [70000000, ff010000, 10020000, 90030000, a0040000]

(After having written this, I noticed that the output doesn't change even if I do not pick a non-default visual and stay with depth=42. Apparently the X11 server doesn't clear "unimportant" bits.)

diff --git a/examples/winit.rs b/examples/winit.rs
index 7c903e2..679720f 100644
--- a/examples/winit.rs
+++ b/examples/winit.rs
@@ -5,9 +5,28 @@ use winit::event_loop::{ControlFlow, EventLoop};
 use winit::keyboard::{Key, NamedKey};
 use winit::window::WindowBuilder;

+use winit::platform::x11::WindowBuilderExtX11;
+use winit::raw_window_handle::{HasDisplayHandle, DisplayHandle, RawDisplayHandle};
+
+fn pick_visual() -> u32 {
+    use x11rb::connection::Connection;
+
+    let (conn, screen) = x11rb::rust_connection::RustConnection::connect(None).unwrap();
+    // Find a random visual with depth=32
+    conn.setup().roots[screen].allowed_depths.iter().filter(|depth| depth.depth == 32)
+        .map(|depth| {
+            let visual = depth.visuals[0];
+            println!("Using visual {:x} of class {:?}, {} bits per rgb, masks {:x}, {:x}, {:x}",
+                visual.visual_id, visual.class, visual.bits_per_rgb_value, visual.red_mask, visual.green_mask, visual.blue_mask);
+            visual.visual_id
+        })
+        .next()
+        .unwrap()
+}
+
 fn main() {
     let event_loop = EventLoop::new().unwrap();
-    let window = Rc::new(WindowBuilder::new().build(&event_loop).unwrap());
+    let window = Rc::new(WindowBuilder::new().with_x11_visual(pick_visual()).build(&event_loop).unwrap());

     #[cfg(target_arch = "wasm32")]
     {
@@ -52,7 +71,15 @@ fn main() {
                             }
                         }

+                        buffer[0] |= 0x70 << 24;
+                        buffer[1] |= 0xff << 24;
+                        buffer[2] |= 0x10 << 24;
+                        buffer[3] |= 0x90 << 24;
+                        buffer[4] |= 0xa0 << 24;
                         buffer.present().unwrap();
+
+                        let fetched = surface.fetch().unwrap();
+                        println!("Some pixel values: {:08x?}", &fetched[..5]);
                     }
                 }
                 Event::WindowEvent {
notgull commented 6 months ago

Generally "assuming RGBA" is a problem for softbuffer at the moment. It’s the main reason we don’t support Android (#44) and the API for non-RGBA surfaces still needs to be figured out (#98).

In X11’s case though you have to go out of your way to use a non-RGBA format. I think there is a lot of software that breaks on non-RGBA aside from softbuffer anyways. So I think a good solution for this for now would be to raise an error if we try to use a window with a strange format.