Support pixel formats other than 8bpc

[basaran]

Hello, how are you?

I use 16-bit color to fight against temporal dithering, but shotgun gives me an error:

Section "Screen"
        Identifier "Default Screen Section"
        Device "<default device>"
        Monitor "<default monitor>"
        DefaultDepth 16
        SubSection "Display"
                Depth 16
                Viewport 0 0
        EndSubSection
EndSection

Failed to convert captured framebuffer, only 24/32 bit (A)RGB8 is supported

[9ary]

The message and your configuration are both slightly unclear about this. shotgun only supports 24/32 bits per pixel (8 bits per color). Adding support for your pixel format shouldn't be too much trouble.

Are you using 16bpp or 16bpc? We need to know the exact pixel format for this, iirc xdpyinfo should tell you that.

I'm not sure how changing your pixel format affects temporal dithering. Do you perhaps have a 5bpc panel that you're trying to match? Doesn't this break other software?

[basaran]

Thank you for getting back. Sorry for not being very clear, I use 16 bit color depth. Please find the the xdpyinfo below.

xdpyinfo.txt

P.S I use the lower color depth so that my monitor doesn't get creative with 8bit+FRC. This seems to help with the eye strain a bit at the expense of lower quality videos.

[9ary]

Thank you for getting back. Sorry for not being very clear, I use 16 bit color depth. Please find the the xdpyinfo below.

That doesn't really answer the question but your xdpyinfo dump tells me exactly what I needed to know. Looks like you're running RGB565. We're gonna need to make pixel format support more generic to fix this.

P.S I use the lower color depth so that my monitor doesn't get creative with 8bit+FRC. This seems to help with the eye strain a bit at the expense of lower quality videos.

That makes sense.
I originally hardcoded shotgun to only support 8 bit per channel formats because I didn't think higher bit depths (for HDR) were worth supporting considering how poorly supported they are on X. However, your use case of matching the lower native color depth of your panel is perfectly valid and a good reason to improve the code.

[jrmarino]

We are hitting this issue as well.
There's the xdpyinfo file:

flex-xdpyinfo.txt

Is it same issue or yet another format that needs to be supported?
It's actually fairly time critical to get a patch for this. I'd be willing to test a patch. Worst case scenario, could you tell me what needs to be fixed and maybe I can try?

[9ary]

Yes, it looks like you are using the same default visual as OP:

  default visual id:  0x21
  visual:.5 
    visual id:    0x21
    class:    TrueColor
    depth:    16 planes
    available colormap entries:    64 per subfield
    red, green, blue masks:    0xf800, 0x7e0, 0x1f
    significant bits in color specification:    8 bits

You will have to adapt this function to fix it:

shotgun/src/xwrap.rs

Lines 188 to 257 in c3e26dc

	pub fn into_image_buffer(&self) -> Option<RgbaImage> {
	unsafe {
	// Extract values from the XImage into our own scope
	macro_rules! get {
	($($a:ident),+) => ($(let $a = (*self.handle).$a;)+);
	}
	get!(
	width,
	height,
	byte_order,
	depth,
	bytes_per_line,
	bits_per_pixel,
	red_mask,
	green_mask,
	blue_mask
	);
	// Pixel size
	let stride = match (depth, bits_per_pixel) {
	(24, 24) => 3,
	(24, 32) | (32, 32) => 4,
	_ => return None,
	};
	// Compute subpixel offsets into each pixel according the the bitmasks X gives us
	// Only 8 bit, byte-aligned values are supported
	// Truncate masks to the lower 32 bits as that is the maximum pixel size
	macro_rules! channel_offset {
	($mask:expr) => {
	match (byte_order, $mask & 0xFFFFFFFF) {
	(0, 0xFF) | (1, 0xFF000000) => 0,
	(0, 0xFF00) | (1, 0xFF0000) => 1,
	(0, 0xFF0000) | (1, 0xFF00) => 2,
	(0, 0xFF000000) | (1, 0xFF) => 3,
	_ => return None,
	}
	};
	}
	let red_offset = channel_offset!(red_mask);
	let green_offset = channel_offset!(green_mask);
	let blue_offset = channel_offset!(blue_mask);
	let alpha_offset = channel_offset!(!(red_mask | green_mask | blue_mask));
	// Wrap the pixel buffer into a slice
	let size = (bytes_per_line * height) as usize;
	let data = slice::from_raw_parts((*self.handle).data as *const u8, size);
	// Finally, generate the image object
	Some(RgbaImage::from_fn(width as u32, height as u32, |x, y| {
	macro_rules! subpixel {
	($channel_offset:ident) => {
	data[(y * bytes_per_line as u32 + x * stride as u32 + $channel_offset)
	as usize]
	};
	}
	Rgba::from_channels(
	subpixel!(red_offset),
	subpixel!(green_offset),
	subpixel!(blue_offset),
	// Make the alpha channel fully opaque if none is provided
	if depth == 24 {
	0xFF
	} else {
	subpixel!(alpha_offset)
	},
	)
	}))
	}
	}

From a cursory glance, you need to properly handle stride and bit masks. Currently it's only designed to handle RGB24, ARGB32 and XRGB32-like formats. Properly handling arbitrary pixel formats might be a bit too complex, but it should be fairly easy to hack RGB565 into it.

Edit: according to the xdpyinfo you posted, you're running x.org 1.6.5 which is really old, so I'm curious as to what you're running this on? :P

[jrmarino]

This is on a proprietary machine in a foreign country. It's running on a very old SuSE Linux Enterprise Server 11 SP3, i586. Updating isn't an option (it would probably be replaced with something entirely new first).

[jrmarino]

By the way, it was a nightmare to get shotgun even to build on it. My third attempt, many hours later, involved finding probably the last available online copy of i586 SLES 11SP3 and installing it on virtual box. Even this I had to jump through hoops because the SLES 11 rpm repositories are long gone.

[9ary]

Updating isn't an option (it would probably be replaced with something entirely new first).

Yeah that's understandable. I was only asking out of curiosity. There's no reason why shotgun shouldn't be able to work on your machine.

By the way, it was a nightmare to get shotgun even to build on it. My third attempt, many hours later, involved finding probably the last available online copy of i586 SLES 11SP3 and installing it on virtual box. Even this I had to jump through hoops because the SLES 11 rpm repositories are long gone.

Unfortunately not much I can help with here. I guess you could try compiling against more recent X11 libraries on modern 32 bit distro but that's all I can suggest. If ported to use x11rb, shotgun could be built as a pure rust statically linked binary, which would alleviate your problem (just cross-compile for i586-unknown-linux-musl), but this is not what this issue is about.

I can't very easily write and test patches for shotgun anymore because I don't use X11 anymore myself. I've asked the other members to take a look at this issue but so far no one has stepped up.

[jrmarino]

well, I can now repeatedly build shotgun so it's not an issue.
I had originally built it on a modern debian, but the issue is unreferenced glibc symbols. Both rust and and the platform need to be limited to glibc 2.11 in this case. Using the MUSL target in rust failed spectacularly. Like I said, I tried many, many things until I finally succeeded.

[jrmarino]

I can't very easily write and test patches for shotgun anymore because I don't use X11 anymore myself. I've asked the other members to take a look at this issue but so far no one has stepped up.

well, how about a compromise?
You write the first draft of a patch supporting this format, I'll test it.

[9ary]

Sorry, what I mean is that I only have limited interest in working on a tool that I do not use. I may still take a look because I need to get back into my programming rhythm and this seems like a decent warmup.

[jrmarino]

I would certainly appreciate it. My rust is rusty and I'd have to figure out what this code is doing. I did at least figure out 565 means 5-bit (Rb), 6-bit (G), 5-bit (B) but I figure you could whip something out a lot faster than it would take me to get up to speed.

it's a pretty useful tool; thank you for building it in the first place.

[jrmarino]

so I think the addition to stride would be:

     (16, 16) => 2,

[jrmarino]

I'm wondering if this should be addition to channel offset since RGB565 is big ending only:

                        (0, 0xF800) => 0,
                        (0, 0x07E0) => 1,
                        (0, 0x001F) => 2,

[9ary]

That won't work. The endianness is still relevant, and you can't rely on byte offsets to extract values smaller than 8 bits. The current code is a bit of a hack since it assumes 8 bits per color. It needs to be thrown away pretty much entirely to properly support arbitrary formats.

[jrmarino]

you are right, of course.

REG009:~ # env DISPLAY=:0.0 ./shotgun
byte_order: 0
red_mask:   f800
green_mask: 7e0
blue_mask:  1f
depth:      16
bits/pixel: 16
Failed to convert captured framebuffer, only 24/32 bit (A)RGB8 is supported

[9ary]

Alright so after some digging it looks like the way we extract the alpha channel seems a bit undefined (the API we use to get image data from X is very old and didn't account for translucent windows - fair enough - so it really just works by sheer luck).

In the long run, I would have considered rewriting most of shotgun to use a better X11 client library, and a newer API to grab image data. But X11 is on life support, and Wayland is the true way forward, so it's just not worth it. Let's just maintain the tools that already exist and do no more than necessary to keep things tolerable.

Considering this, I think it'd be best to just hardcode RGB565 support rather than make things truly generic. This would be significantly less complex and it will retain the existing code path for 8bpc formats. I don't think it's worth the effort to support unknown pixel formats until they're actually needed. 565 is fair enough, 555 may be out there too, but I'm not sure anything else is. 10bpc is essentially unusable on X11, luma/chroma formats are used on some hardware setups, but I'm not sure X11 actually supports them at all.

I'll take a stab at it tomorrow.

[jrmarino]

I totally agree. Thanks for looking at it.

[9ary]

@jrmarino please test #38.

[jrmarino]

That worked like a charm for me!

[jrmarino]

REG009:~ # env DISPLAY=:0.0 ./shotgun
No output specified, defaulting to 1683186184.png

The resulting png file looks good.

[9ary]

Thanks for testing! Waiting for final code review on the PR and it'll be good to merge.

[jrmarino]

FYI I had to apply it as a patch because the raw files on your branch didn't quite match up with the files I had. I think there must have been other changes since the last tag.

[9ary]

Yeah, I committed a few cleanups to master before I started working on this. The PR itself does not depend on them though so it's fine.

[jrmarino]

I would have pulled via git, but this ancient box doesn't have it.

Which brings up another point - you can't build shotgun without git because of the requirement in build.rs. I had to remove the code and define the version string to something I hardcoded. I am sure you were aware of that.

[jrmarino]

moreover, this box can't communicate with github because it's ancient ssl protocols are rejected!

[9ary]

Which brings up another point - you can't build shotgun without git because of the requirement in build.rs. I had to remove the code and define the version string to something I hardcoded. I am sure you were aware of that.

Huh, that should work. I'll look into it.

[jrmarino]

I think the missing "git" executable actually broke the build but perhaps I was mistaken and it was only a warning. I'm not sure right now, but I could easily test it again if necessary.

[9ary]

No need, I've already reproduced and fixed the problem. Thanks for reporting!