search

waycrate / wayshot

Mirrored at https://git.sr.ht/~shinyzenith/wayshot | screenshot tool for wlroots based compositors implementing zwlr_screencopy_v1
https://crates.io/crates/wayshot
BSD 2-Clause "Simplified" License
114 stars 20 forks source link

feat: add support for BGR888 image format #82

Closed vivienm closed 8 months ago

vivienm commented 9 months ago

Hello,

I use the dev version of Sway with NVidia proprietary drivers and I'm unable to take a screenshot using Wayshot.

2024-01-08T16:56:40.302884Z ERROR libwayshot: No suitable frame format found
Error: NoSupportedBufferFormat

Turns out that with my setup, the frame format is BGR888, which is not supported yet. So here is a small PR that seems to make it work for me.

BGR888 is a 3-byte-per-pixel frame format with no transparency, unlike other formats used so far. So in convert_inplace, there is no room in the buffer to turn it into an RGBA image. As a workaround, I use a DynamicImage type in subsequent operations to handle both RGB and RGBA formats.

I'm aware this may not be the best solution, please let me know if there is anything I can do to improve this code :)

Shinyzenith commented 9 months ago

Hi I am on my phone so I can't give a full verdict yet. However the patch looks fine to me with just 1 concern.

Please correct me if my understanding is incorrect, but you're coercing the dynamic image into rgba8 - is it possible to let image crate pick the highest supported bit channel? Eg: 12 bit / 16 bit?

Shinyzenith commented 9 months ago

@Decodetalkers any comments?

vivienm commented 9 months ago

Please correct me if my understanding is incorrect, but you're coercing the dynamic image into rgba8 - is it possible to let image crate pick the highest supported bit channel? Eg: 12 bit / 16 bit?

You're right. I've pushed a new commit to avoid that. Most of this happens in the rotate_image_buffer function. I was not able to turn a DynamicImage into some kind of generic buffer, so I went the other way around so that each branch of the match is a DynamicImage.

Decodetalkers commented 9 months ago

@Decodetalkers any comments?

Emm. I think it should be ok

Shinyzenith commented 9 months ago

Yep this seems fine to me.

Shinyzenith commented 9 months ago

I'll test the pr rq and go ahead with the merge once I get home.

vivienm commented 9 months ago

I'll test the pr rq and go ahead with the merge once I get home.

TY :)

Shinyzenith commented 8 months ago

Hey sorry for the late response! @Decodetalkers can you merge this PR? I'll mirror it to sourcehut when I'm home. Just drop a message in this thread once you merge.

Decodetalkers commented 8 months ago

Ok