Fix: support correct saving for scaled displays #85

AndreasBackx · 2024-02-10T22:59:15Z

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:

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.
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:

1x 1080p + 1x 4K @ 100% -> neither will be resized.
1x 1080p + 1x 4K @ 200% -> the 1080p one will be scaled 200%.
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 · 2024-02-15T19:44:10Z

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 · 2024-02-15T19:45:02Z

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 · 2024-02-16T00:36:20Z

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:

Hyprland rotation problem, will need to look into it further and either fix or report a bug.
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 · 2024-02-16T01:25:05Z

libwayshot/src/lib.rs

@@ -492,7 +486,7 @@ impl WayshotConnection {
                }

                surface.set_buffer_transform(output_info.transform);
-                surface.set_buffer_scale(output_info.scale);
+                // surface.set_buffer_scale(output_info.scale());


why comment this line? will this line never use again?

This accepts only an integer and if we were to round it, it'd still be incorrect. It needs fractional scaling support.

libwayshot/src/image_util.rs

Decodetalkers · 2024-02-16T01:28:23Z

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 · 2024-02-16T01:40:20Z

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

AndreasBackx · 2024-02-16T02:27:34Z

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 · 2024-02-16T04:45:49Z

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

#88

AndreasBackx · 2024-02-16T11:15:41Z

@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 · 2024-02-16T11:28:37Z

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 · 2024-02-16T11:33:11Z

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 · 2024-02-16T11:47:01Z

@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 · 2024-02-16T11:53:31Z

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 · 2024-02-16T12:02:10Z

wayshot/libwayshot/src/lib.rs

Lines 475 to 478 in 0a8e69d

    
           layer_surface.set_size( 
        
               frame_copy.frame_format.size.width, 
        
               frame_copy.frame_format.size.height, 
        
           );

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

Decodetalkers · 2024-02-16T12:09:08Z

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 · 2024-02-16T12:13:12Z

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

Decodetalkers · 2024-02-16T12:16:01Z

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

libwayshot/src/lib.rs

Decodetalkers · 2024-02-16T12:24:29Z

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 · 2024-02-20T23:38:14Z

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 · 2024-02-21T01:45:59Z

it is ok, I will merge it

First version of scaling fix.

0a8e69d

AndreasBackx changed the base branch from main to freeze-feat-andreas February 12, 2024 11:03

Decodetalkers reviewed Feb 16, 2024

View reviewed changes

AndreasBackx mentioned this pull request Feb 16, 2024

fix: scale problem #88

Closed

Decodetalkers reviewed Feb 16, 2024

View reviewed changes

libwayshot/src/lib.rs Show resolved Hide resolved

Decodetalkers approved these changes Feb 16, 2024

View reviewed changes

Decodetalkers requested a review from Shinyzenith February 16, 2024 12:25

Decodetalkers merged commit 8f22e6d into waycrate:freeze-feat-andreas Feb 21, 2024
3 checks passed

murlakatamenka pushed a commit to murlakatamenka/wayshot that referenced this pull request Mar 13, 2024

First version of scaling fix. (waycrate#85)

5e3eb37

nukelet pushed a commit to nukelet/wayshot that referenced this pull request Mar 21, 2024

First version of scaling fix. (waycrate#85)

37326d5

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix: support correct saving for scaled displays #85

Fix: support correct saving for scaled displays #85

AndreasBackx commented Feb 10, 2024 •

edited

Loading

Shinyzenith commented Feb 15, 2024

Shinyzenith commented Feb 15, 2024

AndreasBackx commented Feb 16, 2024

Decodetalkers Feb 16, 2024

AndreasBackx Feb 16, 2024

Decodetalkers commented Feb 16, 2024 •

edited

Loading

Decodetalkers commented Feb 16, 2024

AndreasBackx commented Feb 16, 2024

Decodetalkers commented Feb 16, 2024 •

edited

Loading

AndreasBackx commented Feb 16, 2024

Shinyzenith commented Feb 16, 2024

Shinyzenith commented Feb 16, 2024

Decodetalkers commented Feb 16, 2024 •

edited

Loading

Decodetalkers commented Feb 16, 2024 •

edited

Loading

Decodetalkers commented Feb 16, 2024

Decodetalkers commented Feb 16, 2024 •

edited

Loading

Decodetalkers commented Feb 16, 2024

Decodetalkers commented Feb 16, 2024

Decodetalkers commented Feb 16, 2024 •

edited

Loading

Shinyzenith commented Feb 20, 2024

Decodetalkers commented Feb 21, 2024

Fix: support correct saving for scaled displays #85

Fix: support correct saving for scaled displays #85

Conversation

AndreasBackx commented Feb 10, 2024 • edited Loading

Shinyzenith commented Feb 15, 2024

Shinyzenith commented Feb 15, 2024

AndreasBackx commented Feb 16, 2024

Decodetalkers Feb 16, 2024

Choose a reason for hiding this comment

AndreasBackx Feb 16, 2024

Choose a reason for hiding this comment

Decodetalkers commented Feb 16, 2024 • edited Loading

Decodetalkers commented Feb 16, 2024

AndreasBackx commented Feb 16, 2024

Decodetalkers commented Feb 16, 2024 • edited Loading

AndreasBackx commented Feb 16, 2024

Shinyzenith commented Feb 16, 2024

Shinyzenith commented Feb 16, 2024

Decodetalkers commented Feb 16, 2024 • edited Loading

Decodetalkers commented Feb 16, 2024 • edited Loading

Decodetalkers commented Feb 16, 2024

Decodetalkers commented Feb 16, 2024 • edited Loading

Decodetalkers commented Feb 16, 2024

Decodetalkers commented Feb 16, 2024

Decodetalkers commented Feb 16, 2024 • edited Loading

Shinyzenith commented Feb 20, 2024

Decodetalkers commented Feb 21, 2024

AndreasBackx commented Feb 10, 2024 •

edited

Loading

Decodetalkers commented Feb 16, 2024 •

edited

Loading

Decodetalkers commented Feb 16, 2024 •

edited

Loading

Decodetalkers commented Feb 16, 2024 •

edited

Loading

Decodetalkers commented Feb 16, 2024 •

edited

Loading

Decodetalkers commented Feb 16, 2024 •

edited

Loading

Decodetalkers commented Feb 16, 2024 •

edited

Loading