Skip to content

Optimize ScreenResources::{enabled_crtcs, crtc}#14

Open
Erk- wants to merge 2 commits intodzfranklin:mainfrom
Erk-:optimize-crtc-screen-resources
Open

Optimize ScreenResources::{enabled_crtcs, crtc}#14
Erk- wants to merge 2 commits intodzfranklin:mainfrom
Erk-:optimize-crtc-screen-resources

Conversation

@Erk-
Copy link

@Erk- Erk- commented Jan 23, 2024

ScreenResources::enabled_crtcs: This method would cause a unnecessary allocation through ScreenResources::crtcs.

ScreenResources::crtc: This would call Crtc::from_xid on all XId's this would cause a massive amount of spam in the Xorg log files. This will now only instantiate the Crtc that is asked for.

ScreenResources::enabled_crtcs: This method would cause a unnecessary
allocation through ScreenResources::crtcs.

crtc: This would call Crtc::from_xid on all XId's this would cause a
massive amount of spam in the Xorg log files. This will now only
instantiate the Crtc that is asked for.
@Rintse
Copy link
Collaborator

Rintse commented Feb 13, 2024

Good changes, avoiding the collect() step from crtcs() in enabled_crts() is very much desired.

Your change to crtc() has made me think though:
Why is this function even a method of ScreenResources?
I think it originally must have been to mirror the call signature of libxrandr's XRRGetCrtcInfo(), which is provided a ScreenResources and an XID to obtain the Crtc info.

Perhaps it would be nice to check whether the XID requested is actually part of the ScreenResources instance we call crtc() on.

Your thoughts?

@Erk-
Copy link
Author

Erk- commented Feb 16, 2024

Perhaps it would be nice to check whether the XID requested is actually part of the ScreenResources instance we call crtc() on.

I have added that.

Why is this function even a method of ScreenResources?

Yeah I thought the same, though I attempted to not introduce any breaking changes in this pr, though it could be something to look into in the future.

@Rintse
Copy link
Collaborator

Rintse commented Feb 16, 2024

Sorry this issue has turned out to reveal a major oversight.

I looked into the libxrandr source and found the source of the confusion here.
There is a field of XRRScreenResources that is used in XRRGetCrtcInfo(): the timestamp.

This crate therefore should also make sure that this timestamp is properly passed to the XRRGetCrtcInfo() call.
The way to do this is not throw away the raw pointer ptr in the screenresources handle and pass it into the GetCrtcInfo call, rather than allocating a new handle like I do now.

Perhaps it is best if I fix this first, and then we continue with this PR.

@Rintse Rintse mentioned this pull request Feb 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants