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
108 stars 18 forks source link

Fix: support correct saving for scaled displays #85

Closed AndreasBackx closed 5 months ago

AndreasBackx commented 5 months ago

In #78 it was discussed that master did not support scaled displays. The way screenshotting works is that screencopies are requested in logical regions (so after scaling, a pixel is not a real world pixel), but they're actually saved as "physical" regions (before scaling, aka the unit is 1 real world pixel). Some issues that I fixed were:

  1. We created the final image as a "logical" size. So when someone took a screenshot of a 4K screen that was scaled 200%, the picture in memory would be 4K but we would create a 1080p size output file. A 4K output file would now be created.
  2. Following the previous step, we need to convert the "logical" region to a "phyiscal" region, aka scale it as well.

But how and where do we scale? From all of the outputs, we take the maximum scale and make sure to scale all other outputs the same amount. This makes sure that our final image will lose the least amount of information. I'm having a hard time explaining this so I'll give some examples in hopes that it makes sense:

  1. 1x 1080p + 1x 4K @ 100% -> neither will be resized.
  2. 1x 1080p + 1x 4K @ 200% -> the 1080p one will be scaled 200%.
  3. 1x 4K @ 100% + 1x 4K @ 200% -> the 4K @ 100% will be scaled 200%.

This seemed the most logical default scaling to me. We could technically make it configurable, though the way it's setup now is that it assumes it will never scale it down.

Still have to do some testing when attaching multiple monitors, but it should work. ™ I am working on my desk setup so it might take me a while to have more than 1 monitor, I'd appreciate someone else testing it with 2 screens and different scaling.

Note: this PR does not fix the freeze overlaying. I actually removed it even because the changes wouldn't allow for it and it needs to be fixed in another PR.

Shinyzenith commented 5 months ago

The upscale policy seems acceptable to me. The code from a preliminary view looks fine to me! I don't have a dual monitor system at the moment.

@Decodetalkers Would you mind testing the PR?

Shinyzenith commented 5 months ago

Note: this PR does not fix the freeze overlaying. I actually removed it even because the changes wouldn't allow for it and it needs to be fixed in another PR.

Pardon me, I seem to be losing track due to the large changes recently, what overlaying issue is being discussed here? Is it the image that decode posted in the earlier pr which was due to us not scaling the overlay buffer to physical size?

AndreasBackx commented 5 months ago

Note: this PR does not fix the freeze overlaying. I actually removed it even because the changes wouldn't allow for it and it needs to be fixed in another PR.

Pardon me, I seem to be losing track due to the large changes recently, what overlaying issue is being discussed here? Is it the image that decode posted in the earlier pr which was due to us not scaling the overlay buffer to physical size?

There are still two issues with overlaying:

  1. Hyprland rotation problem, will need to look into it further and either fix or report a bug.
  2. Overlaying in general has not worked yet on a scaled output because this will require one of the newer wlr protocols that allows you to specify that a surface is fractional scaling aware. On a scaled display, it won't overlay correctly afaik because of this. I haven't looked into it much, though we might not even need the special protocol, I'm unsure. Anyhow, overlaying during a freeze is not scaled properly so does not cover the entire output.

Primarily referring to 2 here.

Decodetalkers commented 5 months ago

Emm. you should resize in freeze stage, not in finally picture output stage, so I think you just need resize the picture you get in freeze is enough, and maybe this pr is not needed

Decodetalkers commented 5 months ago

Maybe you can add a option, for if it is preview image or not, preview image will always resize

AndreasBackx commented 5 months ago

Emm. you should resize in freeze stage, not in finally picture output stage, so I think you just need resize the picture you get in freeze is enough, and maybe this pr is not needed

We should not need to resize at all for a freeze as we'd lose quality. We already have a pixel perfect screenshot we simply need to overlay properly.

Also, we would not be able to anyhow. Freezing is done using the same buffer as the screenshot and resizing would mean reading it from there which would make the freeze noticeably lag behind the startup.

Decodetalkers commented 5 months ago

no, the fix induced many problem, so I create another pr. the size is still not right

https://github.com/waycrate/wayshot/pull/88

AndreasBackx commented 5 months ago

@Decodetalkers can you please explain the issues? You created a PR without any information. When looking at the PR you are doing exactly what I said in my previous comment will cause noticeable lag:

Freezing is done using the same buffer as the screenshot and resizing would mean reading it from there which would make the freeze noticeably lag behind the startup.

You are allocating reading from the existing buffer which takes a lot of time, resizing it, and then writing it to a new buffer. This will lose quality as well and resizing is not at all needed because we have a pixel perfect version of the output in the existing buffers.

Could you explain the issues or possibly point out where my logic is incorrect?

Shinyzenith commented 5 months ago

no, the fix induced many problem

Hi @Decodetalkers, can you kindly enumerate the problems induced so we can improve the PR? From what I can tell, apart from the buffer scale setter function which only accepts integers and not fractionally scaled surfaces -- other things look fine.

Shinyzenith commented 5 months ago

Also, we would not be able to anyhow. Freezing is done using the same buffer as the screenshot and resizing would mean reading it from there which would make the freeze noticeably lag behind the startup.

And this matters heavily as we ideally want the least amount of time from binary invocation to overlay display -- scaling would make us lag behind due to increased allocations.

Decodetalkers commented 5 months ago

@Decodetalkers can you please explain the issues? You created a PR without any information. When looking at the PR you are doing exactly what I said in my previous comment will cause noticeable lag:

Freezing is done using the same buffer as the screenshot and resizing would mean reading it from there which would make the freeze noticeably lag behind the startup.

You are allocating reading from the existing buffer which takes a lot of time, resizing it, and then writing it to a new buffer. This will lose quality as well and resizing is not at all needed because we have a pixel perfect version of the output in the existing buffers.

Could you explain the issues or possibly point out where my logic is incorrect?

Emmm. I have test your pr, but the size of picture is not correct, the overlay image become too big when the scale is 1.2, I have test your pr on sway, but it not works well

Decodetalkers commented 5 months ago

you not not adjust the picture on the overlay , I found it still be the size of picture, it will not fully show over the screen. at the beginning, what I concerned is that. the picture size by the wlr-screencopy won't be the size of the screen , so it need to resize to cover on the whole screen, but this pr do not solve that problem. because it does not resize the image it captured. so my problem still not solved.

The buffer scale always be integer, so it won't cover the fractional case, and the size by wlr-screencopy always be the real size of the image, so if is set with fractional size, the frame_copy.frame_format.size won't be the screen size, so it cause problem. So after I test and view this pr, I think this pr has not solve my problem, so I make another pr

Decodetalkers commented 5 months ago

https://github.com/waycrate/wayshot/blob/0a8e69d5b8c7bf95735df2b615261774a5cac775/libwayshot/src/lib.rs#L475-L478

I mean the size will always not be the size of the screen, so this is my problem

Decodetalkers commented 5 months ago

PXL_20240216_120849463.jpg

So the problem for me is that I think the picture should over the whole screen, the size should just be the size of screen, other things will be ok it is what I am thinking

Decodetalkers commented 5 months ago

I see, this pr is not about freeze overlay, so we are solving two problems

Decodetalkers commented 5 months ago

Emm. but without this pr, when use different scale, what will happened? @AndreasBackx I think it will works fine. only I concerned is that the pciture will not cover the screen, the picture shoted will be wrong

Decodetalkers commented 5 months ago

oh, I see, the logic you use will make the picture be best resized, greate work, have you test on muti screens? I do not have muti screens now

sorry I misunderstood your pr, you are doing greate work, thanks

Shinyzenith commented 5 months ago

I'd like to merge this PR. Before I do so, can anyone confirm that this works on multiple monitor system? I do not have access to one sadly.

Decodetalkers commented 5 months ago

it is ok, I will merge it