search

FedeDP / Clightd

A linux bus interface that lets you change screen brightness, compute captured webcam frames brightness and change screen temperature.
GNU General Public License v3.0
83 stars 10 forks source link

pure dmabuf implementation of screen #95

Closed tpeacock19 closed 1 year ago

tpeacock19 commented 1 year ago

This commit is rather large but provides two main features:

In order to accomplish the above the following protocols updated/included:

I also took the liberty to attempt to centralize some structs that could be reused for the other wayland modules cl_display, cl_buffer, cl_frame, & cl_output. I haven't looked into incorporating them but I've included it in case you find it useful. If not, I can move them back to the screen/wl.c file.

FedeDP commented 1 year ago

Hi! Thanks for this!

It updates the wayland screen implementation to use dmabufs with shm buffers as a fallback.

Perhaps i am missing some context though: what is wrong with current screen implementation? I am not much into wayland stuff honestly, i just want the smallest possible code base for this feature (note also that Clightd is not a performance-driven application, therefore even if we are not super performant but get the things done, is ok IMO) and wayland protocols are a bit of an headache for me to work with. But again, it is highly possible that i am missing some key point here :laughing:

Providing an ability to get screen brightness per output. Currently, it averages the brightness to be sent from a get_frame_brightness call, but can be useful if we allow for per-output screen brightness modifications.

This is cool!

I also took the liberty to attempt to centralize some structs that could be reused for the other wayland modules cl_display, cl_buffer, cl_frame, & cl_output. I haven't looked into incorporating them but I've included it in case you find it useful. If not, I can move them back to the screen/wl.c file.

Can you split this change to a new commit so that i am able to review the feature first, and then eventually the other commit with the refactor? Thank you very much!

tpeacock19 commented 1 year ago

Perhaps i am missing some context though: what is wrong with current screen implementation? I am not much into wayland stuff honestly, i just want the smallest possible code base for this feature (note also that Clightd is not a performance-driven application, therefore even if we are not super performant but get the things done, is ok IMO) and wayland protocols are a bit of an headache for me to work with. But again, it is highly possible that i am missing some key point here laughing

Nothing is wrong with the current implementation at all. I really just started trying to get the per-output brightness when I realized we are only using the first one. Reading the Wayland spec I found out about dmabufs and figured it would be useful to try it out as it's meant to be more efficient. I was able to get comparable or slightly less memory usage when running it with the dmabuf backend and setting Clight to make adjustments every 3 seconds. I can understand if you don't find it necessary and can remove it.

Can you split this change to a new commit so that i am able to review the feature first, and then eventually the other commit with the refactor? Thank you very much!

Yes, I'll update the PR to have all the structs in wl.c and then you can decide whether you think it useful to move it to wl_utils.h

tpeacock19 commented 1 year ago

I'm going to delete this PR and create a new one with a simpler commit. Sorry for the clutter.

FedeDP commented 1 year ago

No problem sir :) and thanks for the explanation! I will review the new one once it's opened!