-
Notifications
You must be signed in to change notification settings - Fork 29
Conversation
unstable/wlr-output-manager-v1.xml
Outdated
</description> | ||
</event> | ||
|
||
<event name="disconnected"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this is a global, we can just remove the global instead.
unstable/wlr-output-manager-v1.xml
Outdated
This event is sent after the client binds to the output_device | ||
and whenever the output device is enabled or disabled. | ||
</description> | ||
<arg name="enabled" type="int" summary="output enabled state"/> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of having an enum, we can just have an int
which is non-zero when enabled
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or something like:
<event name="wl_output> <arg name="output" type="object" interface="wl_output" nullable/> <event>
non-null when it's active, and null when sent to inactivity.
A direct link to wl_output (avoiding string comparisons) is missing either way IMO
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not against a link between this and wl_output, but iirc there was some discussion on #wayland about different protocols creating the same interfaces and why that was a bad idea. Might be wrong on that. This would also be a server-created object, which is tricky to destroy. (see http://ppaalanen.blogspot.com/2014/07/wayland-protocol-design-object-lifespan.html )
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"A link to wl_output" would not mean that this interface would create wl_output objects.
However it is tricky, because output_device objects are published as globals, and we can "link" wl_outputs only after the client has bound to the global…
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, got it, that would be tricky indeed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The more I think about this, the more I don't like this approach. An alternative solution would be to send the name that the registry gives to the global. However, as far as I can tell, it's not possible to obtain that in the current api.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be client specific, because you may hide others, and they are just a counter from what I've seen.
Well, it would be sent with all other events on binding. It can't be linked to it like xdg-output is either way. It's just an "attribute" the client can track.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking at the libwayland code, the names are indeed a counter. However, there's currently no way to obtain the name of a global, except guessing it.
I think we should try to get this used by KDE as well, might be nicer to have it as optional output attribute for those that want to support it for that purpose (and in general) |
unstable/wlr-output-manager-v1.xml
Outdated
|
||
The naming convention is compositor defined, but limited to | ||
alphanumeric characters and dashes (-). Each name is unique among all | ||
wl_output globals, but if a wl_output global is destroyed the same name |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wl_output
globals? :)
I think you copy+pasted a bit eagerly.
Should probably reference the xdg-output
name event (and force equality?).
I'm leaning towards string. Enum could be annoying, and if we want to include e.g. "leaves gaps" which is allowed by the protocol, but might be imposed by the compositor, it gets ugly.
As always: IANAL Copyright is a mess. You didn't base this on the core, you just re-used necessary parts, afaik no need to consider that. |
They can just use "name" for this. |
Or rather description, yes. They didn't have this field before, and I'd bind the name to xdg-output name, as mentioned before |
@VincentVanlaer Why did you use "transformation" and not "transform" throughout the protocol? The latter is used in |
unstable/wlr-output-manager-v1.xml
Outdated
<description summary="create a new output configuration object"> | ||
Create a new output configuration object. | ||
</description> | ||
<arg name="id" type="new_id" interface="zwlr_output_device_v1"/> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
zwlr_output_configuration_v1
unstable/wlr-output-manager-v1.xml
Outdated
</request> | ||
</interface> | ||
|
||
<interface name="zwlr_output_configuration_v1" version="1"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How do you set the scale?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ceil(size/mode)
But that needs an addition that enforces that the same result with both width and height.
And some tolerance for floats :/
I had a discussion with Pekka on #wayland recently, and the result was that we should probably add a non-integer sizing hint, and just use scale for non-HiDPI-aware clients, so I wonder if we should just drop the fractional scale concept in protocols.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, I mean there's no set_scale
request in this interface.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added size. Similar to xdg-output's logical_size event. Allows for fractional scaling.
It is computable from this (provided I understood the intended usecase correctly)
But I wonder if it's a good idea to have this in the long run
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This allows to read the scale, not to set it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My bad, must have forgotten to add the set_size event. I don't think we should drop fractional scaling in this protocol, since it is used for configuration instead of rendering. I used this on X on my old laptop to scale from 1600x900 (built in screen resolution) to 1920x1080 for certain external displays when giving presentations.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure a set_size
request would work well: what happens if the compositor doesn't support fractional scaling? What happens if the x-axis ratio is different from the y-axis ratio?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The compositor would reject the change in that case.
unstable/wlr-output-manager-v1.xml
Outdated
@@ -0,0 +1,350 @@ | |||
<?xml version="1.0" encoding="UTF-8"?> | |||
<protocol name="wlr_output_manager_v1"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be wlr_output_manager_unstable_v1
(or maybe management
?)
unstable/wlr-output-manager-v1.xml
Outdated
<arg name="refresh" type="int" summary="vertical refresh rate in mHz"/> | ||
</event> | ||
|
||
<event name="active_mode"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"Refresh" doesn't always make sense, some outputs might have variable refresh rates (such as virtual outputs)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe the compositor could send zero if it doesn't apply?
unstable/wlr-output-manager-v1.xml
Outdated
<arg name="refresh" type="int" summary="vertical refresh rate in mHz"/> | ||
</event> | ||
|
||
<event name="transformation"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some of these events don't make sense if the output is disabled. What then? Should the compositor not send them?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that would be the cleanest approach.
unstable/wlr-output-manager-v1.xml
Outdated
summary="height in global compositor space"/> | ||
</event> | ||
|
||
<event name="position"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if it makes sense to have these events at all. These events only make sense if the output is enabled. In this case this data can be obtained from wl_output and xdg-output as normal.
The only added value provided by output_device is that the object exists even if the output is disabled. So I wonder if it's necessary to add these at all.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The only two events that we would be able to drop are transformation and position. The others are different from the wl_output and xdg_output interfaces. It doesn't feel right to to reference only those two.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(Same for "size")
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(And "active_mode")
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
size is different from the xdg_output logical_size in the current description.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm, if xdg_output.logical_size would be exactly the same, I'd rather have them separate too. That would solve #23 (comment) .
This is handled by wl_registry.global_remove
transform is consistent with wl_output
Fix for copy-paste mistake. Add references to xdg-output
Add unstable manager -> management: seem sto be more consistent with the protocols in wayland-protocols.
Great this is moving forward! Two things I'm wondering about: where would we put output cloning (one display showing the same as another)? Should this be a part of this protocol? How would I map a {wl,xdg}_output to an output_device ? |
I think that output cloning is to complex for the first version, see swaywm/sway#1666 . However, it would make sense for compositors to check whether
It's being discussed in #23 (comment) , but gihub is being annoying and has marked everything outdated. |
fae38fd
to
7cf8748
Compare
Should have addressed all comments except for |
event once for every supported mode. | ||
|
||
If the concept of a specific refresh rate does not apply to the mode, | ||
the refresh argument shpuld be zero. An example of this is a monitor |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo
summary="y position within the global compositor space"/> | ||
</event> | ||
|
||
<enum name="enablement"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not used anymore
Additional prior art: https://lists.freedesktop.org/archives/wayland-devel/2014-February/013481.html Reading through the discussion and why it hasn't been merged might be interesting. |
I would prefer to refactor this into a design similar to xdg-output, where the client explicitly maps wl_outputs to zwlr_output_device_v1, for consistnecy and so that we don't have to duplicate the information provided by xdg_output. |
@emersion Unless I missed something, it just stops at v4 without any comments. The discussion might be interesting, I'll give it a read. @SirCmpwn Aye, I agree that duplication is not optimal, but we still want to represent disabled outputs. Should we have a global for each disabled output? So one either has a wl_output or a wl_disabled_output for each output that is connected. It makes the configuration interface messy though since it needs to accept either type of output. |
Right, I forgot about disabled outputs. Bleh. |
One issue this protocol doesn't address is frame-perfect output configuration on hotplug. When the logic to configure outputs is in the compositor, it's easy to apply it on hotplug and get the desired configuration on first modeset. When the logic is in a client like this, we don't know if the client will attempt to change it on hotplug so there will be two modesets: one done by the compositor after hotplug, and another one done by the client to apply the new configuration. For instance if I have configured an output to be disabled in my client config, plugging it will first enable it and then disable it. Same if I've configured a different mode. Maybe this could be solved with a |
Calling this "frame perfect" is funny with the time some outputs take to modeset :) The interesting challenge here is to provide a mechanism that will work without an application and can be driven with one modeset from the an application. One simple way would be to mandate compositors don't automatically change related settings while any application has an object of this protocol. |
Additional prior art: GNOME https://wiki.gnome.org/Initiatives/Wayland/Gaps/DisplayConfig |
We should design this protocol under the assumption that 10 years from now it'll be used to modeset outputs which can change modes within a single frame |
For hotplugged outputs (when the compositor is already running) we can keep them off and then only apply the configuration once the compositor sees the "apply" request, no? It's more cumbersome when the compositor starts up and applies a default configuraton and the client isn't happy with that which would introduce screen flickering. |
Yes, but there are issues with output changes not seen as atomic. So we should probably have an event saying "I've sent all newly plugged outputs and removed all newly unplugged outputs, now please send a configuration" and an event saying "you asked me to configure all outputs, please apply this state atomically". This will help in cases where there are less CRTCs than connected outputs. |
Here is a new proposal: #38 |
Fixes #15
I still need to do a some descriptions and a final spell/grammar check, but the protocol itself can be reviewed.
The protocol references the wl_output.transform enum, so it will only build on the alpha that was just released.
Prior art by KDE:
https://cgit.kde.org/kwayland.git/tree/src/client/protocols/outputdevice.xml
https://cgit.kde.org/kwayland.git/tree/src/client/protocols/output-management.xml
Difference with the KDE protocols
Questions I still have: