fix: return on None instead of panic

[wash2]

this avoids some panics and mutex PoisonError that I saw in the logs

[ids1024]

I thought wl_shm on cosmic-comp should support both? Even though argb8888 is the one that all compositors must support.

If you just the color just here, won't the colors of saved screenshots or pipewire shm screen capture have their r and b channels swapped? Since this doesn't do any conversion or change the formats used there.

I was going to test that, but I'm getting failed to receive screenshot response: Response(Cancelled). At least something is logged:

Jan 22 12:33:06 pop-os cosmic-session[258131]: [2024-01-22T20:33:06Z ERROR xdg_desktop_portal_cosmic::screenshot] Screenshot output count mismatch: 3 != 0
Jan 22 12:33:06 pop-os cosmic-session[258131]: [2024-01-22T20:33:06Z WARN  xdg_desktop_portal_cosmic::screenshot] Screenshot outputs: [OutputState { output: WlOutput { id: ObjectId(wl_output@6), version: 4, data: Some(ObjectData { .. }), backend: WeakBackend { inner: WeakInnerBackend { inner: (Weak) } } }, id: Id(1), name: "eDP-1", logical_size: (1920, 1080), logical_pos: (0, 0), has_pointer: false, bg_source: None }, OutputState { output: WlOutput { id: ObjectId(wl_output@7), version: 4, data: Some(ObjectData { .. }), backend: WeakBackend { inner: WeakInnerBackend { inner: (Weak) } } }, id: Id(2), name: "DP-7", logical_size: (2560, 1440), logical_pos: (1920, 0), has_pointer: false, bg_source: None }, OutputState { output: WlOutput { id: ObjectId(wl_output@8), version: 4, data: Some(ObjectData { .. }), backend: WeakBackend { inner: WeakInnerBackend { inner: (Weak) } } }, id: Id(3), name: "DP-8", logical_size: (2560, 1440), logical_pos: (4480, 0), has_pointer: false, bg_source: None }]
Jan 22 12:33:06 pop-os cosmic-session[258131]: [2024-01-22T20:33:06Z WARN  xdg_desktop_portal_cosmic::screenshot] Screenshot images: []

[wash2]

I thought wl_shm on cosmic-comp should support both? Even though argb8888 is the one that all compositors must support.

If you just the color just here, won't the colors of saved screenshots or pipewire shm screen capture have their r and b channels swapped? Since this doesn't do any conversion or change the formats used there.

I was going to test that, but I'm getting failed to receive screenshot response: Response(Cancelled). At least something is logged:

Jan 22 12:33:06 pop-os cosmic-session[258131]: [2024-01-22T20:33:06Z ERROR xdg_desktop_portal_cosmic::screenshot] Screenshot output count mismatch: 3 != 0
Jan 22 12:33:06 pop-os cosmic-session[258131]: [2024-01-22T20:33:06Z WARN  xdg_desktop_portal_cosmic::screenshot] Screenshot outputs: [OutputState { output: WlOutput { id: ObjectId(wl_output@6), version: 4, data: Some(ObjectData { .. }), backend: WeakBackend { inner: WeakInnerBackend { inner: (Weak) } } }, id: Id(1), name: "eDP-1", logical_size: (1920, 1080), logical_pos: (0, 0), has_pointer: false, bg_source: None }, OutputState { output: WlOutput { id: ObjectId(wl_output@7), version: 4, data: Some(ObjectData { .. }), backend: WeakBackend { inner: WeakInnerBackend { inner: (Weak) } } }, id: Id(2), name: "DP-7", logical_size: (2560, 1440), logical_pos: (1920, 0), has_pointer: false, bg_source: None }, OutputState { output: WlOutput { id: ObjectId(wl_output@8), version: 4, data: Some(ObjectData { .. }), backend: WeakBackend { inner: WeakInnerBackend { inner: (Weak) } } }, id: Id(3), name: "DP-8", logical_size: (2560, 1440), logical_pos: (4480, 0), has_pointer: false, bg_source: None }]
Jan 22 12:33:06 pop-os cosmic-session[258131]: [2024-01-22T20:33:06Z WARN  xdg_desktop_portal_cosmic::screenshot] Screenshot images: []

When I tested it, it didn't have any color issues. I'll check out the logs though, thanks.

[wash2]

It looks like from your logs, the screenshot images are failing. Maybe I will add logs to the methods that can fail when creating the images

[ids1024]

Yeah, these logged errors aren't related to this particular change, but for some reason they started occurring, and I can't test it with that error.

A lot of things will look mostly normal with r and b swapped, but look to see if screen capture and screenshots of blue or red things are wrong.

[wash2]

A lot of things will look mostly normal with r and b swapped, but look to see if screen capture and screenshots of blue or red things are wrong.

Ya, I did but I think when I tested originally, I forgot to add prefix=/usr when installing :sweat_smile: After testing with that, it also fails the way you describe, so I reverted the format change.

[ids1024]

Anyway, returning an error instead of panicking isn't a bad thing to do, but in what circumstance does this fail?

https://github.com/pop-os/cosmic-comp/blob/ce74675b0ebc23e9bf547b9315450fd74d7ae49c/src/wayland/handlers/screencopy.rs#L468-L479 always advertises ShmFormat::Abgr8888 as a buffer_info, and seems to never advertise formats like Argb8888.

If that fails, it may be a compositor bug.

[wash2]

Anyway, returning an error instead of panicking isn't a bad thing to do, but in what circumstance does this fail?

It's difficult for me to recreate the error, but the portal unwrapped on that error when I attempted to take a screenshot. I am not sure why, It hasn't happened for a while though.

If OutputEvent::InfoUpdate occurs for an output we didn't get OutputEvent::Created, would that be a bug in iced-sctk? Or did we get OutputEvent::Created(None)?

My assumption is that the Created event was previously ignored because the output info had no logical size or position. I guess I haven't confirmed it because it was also a bit difficult to recreate. I'll check cosmic-comp to see if that is actually possible.

[ids1024]

Yeah, it's hard to test rare issues like that. Though if one of these two unwraps in capture_source_shm_fd was panicking, that's odd. And it would be good to figure out the cause, since that's likely a bug elsewhere, and screen capture / screenshotting won't be working properly when this is failing for some reason.

[wash2]

Yeah, it's hard to test rare issues like that. Though if one of these two unwraps in capture_source_shm_fd was panicking, that's odd. And it would be good to figure out the cause, since that's likely a bug elsewhere, and screen capture / screenshotting won't be working properly when this is failing for some reason.

Ok I've added some improvements to the logged info, which might help in determining the cause. I intend to keep an eye out for any screenshots that don't work properly and check the logs if I see anything.