Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

tearing: add fullscreen option #1568

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

Consolatis
Copy link
Member

If set, labwc will try to automatically enable tearing for fullscreen applications.

Fixes:


@ahesford @Ph42oN review appreciated.

Copy link
Member

@ahesford ahesford left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The logic looks right, but I dont' have the means to actually test any of the tearing options.

@Consolatis
Copy link
Member Author

Consolatis commented Feb 28, 2024

Technically I guess we should actually check for a tearing hint by the application first and only if there is none allow tearing for fullscreen. Otherwise applications don't have a chance to disable tearing when fullscreen (and enabled by the user).

That would require view->tearing_hint to become some yes|no|unspec enum.

If set, labwc will try to automatically enable tearing for fullscreen
applications.

Fixes: labwc#1557
@jp7677
Copy link
Contributor

jp7677 commented Feb 28, 2024

Technically I guess we should actually check for a tearing hint by the application first and only if there is none allow tearing for fullscreen. Otherwise applications don't have a chance to disable tearing when fullscreen (and enabled by the user).

Just a thought, may be split the fullscreen configuration option into two values to allow more fine grained control?

  • fullscreen -> tearing in full screen mode unless hinted otherwise by the application
  • forceFullscreen (or fullscreenForcedor similar, I haven't had a better idea yet) -> always set tearing in full screen mode regardless hints by the application

@ahesford
Copy link
Member

If we're going to respect a no-trading hint from the client, I don't think we should further complicate the interface by splitting the fullscreen option in two. If users wish to override the client hint, they can just use the ToggleTearing action.

@Faugus
Copy link

Faugus commented Feb 29, 2024

I found a problem. If I play a video with mpv on fullscreen, it speeds up the playback.

@Consolatis
Copy link
Member Author

Consolatis commented Feb 29, 2024

Automatically detecting a fullscreen game vs a fullscreen video player doesn't sound that easy. Not sure if we can make the new fullscreen work for both. If we end up respecting application / user configuration via ToggleTearing - even if configured for fullscreen - the user could at least manually disable tearing again for cases likes mpv.

It isn't optimal though, the wine wayland backend should request tearing instead, then <allowTearing>yes</allowTearing> would suffice.

@jp7677
Copy link
Contributor

jp7677 commented Feb 29, 2024

Imho video players should set the no-tearing hint (may be mpv already does that, I don’t know), together with #1568 (comment) should disable tearing for those cases.

Edit: but yeah, having it the other way around, wine setting the tearing hint, would be nicer.

@Faugus
Copy link

Faugus commented Feb 29, 2024

This option video-sync=display-resample in MPV's config file with allowTearing was causing the issue.
I set this a long time ago and I didn't remember.

Sorry.

@Consolatis
Copy link
Member Author

This option video-sync=display-resample in MPV's config file with allowTearing was causing the issue. I set this a long time ago and I didn't remember.

Sorry.

No worries, thanks for mentioning the underlying reason.

Changed the PR to prefer client state (either set by the client or via ToggleTearing) over fullscreen.

@Faugus
Copy link

Faugus commented Mar 1, 2024

If you need me to do any tests, just let me know.

@Consolatis
Copy link
Member Author

Consolatis commented Mar 1, 2024

Feel free to test the current state of the PR. What should happen now is that with <allowTearing>fullscreen</allowTearing>

  • tearing should be enabled automatically when in fullscreen
  • when then using ToggleTearing you should still be able to disable / re-enable tearing while in fullscreen
  • there should be no difference in behavior compared to the current master branch for the yes or no config settings

@Faugus
Copy link

Faugus commented Mar 1, 2024

On mpv with video-sync=display-resample in the config file, it works.
But it's not working with games.
If I toggle it, it breaks the gamepad compatibility like before, but it doesn't seems to turn on the allowTearing.
The game looks like the allowTearing is always off, cause I don't see the jittering.

@Consolatis Consolatis marked this pull request as draft March 4, 2024 01:29
@Ph42oN
Copy link
Contributor

Ph42oN commented Mar 9, 2024

I removed fullscreen option before it was merged because i thought that with tearing protocol and ToggleTearing it was not needed.
Using this branch, tearing does not seem to work at all in VRRTest. This breaks enabling it even with ToggleTearing that used to work. But it continues to work in games that request tearing. I think it requests to not tear, and i wanted to have way to override that, in previous patch i made this work by setting tearing_hint in set_tearing_hint() only if enabling it.

Copy link
Contributor

@Ph42oN Ph42oN left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this would be simple way to make ToggleTearing override it.

diff --git a/include/view.h b/include/view.h
index c3363623..c4bfca2f 100644
--- a/include/view.h
+++ b/include/view.h
@@ -170,6 +170,7 @@ struct view {
 	enum view_axis maximized;
 	bool fullscreen;
 	enum three_state tearing_hint;
+	bool tearing_override;
 	bool visible_on_all_workspaces;
 	enum view_edge tiled;
 	uint32_t edges_visible;  /* enum wlr_edges bitset */
diff --git a/src/action.c b/src/action.c
index b564f4fd..bdbee257 100644
--- a/src/action.c
+++ b/src/action.c
@@ -1007,6 +1007,7 @@ actions_run(struct view *activator, struct server *server,
 			break;
 		case ACTION_TYPE_TOGGLE_TEARING:
 			if (view) {
+				view->tearing_override = !view->tearing_override;
 				switch (view->tearing_hint) {
 				case LAB_STATE_UNSPECIFIED:
 				case LAB_STATE_DISABLED:
diff --git a/src/tearing.c b/src/tearing.c
index 3cf1aa92..4b2470ed 100644
--- a/src/tearing.c
+++ b/src/tearing.c
@@ -14,7 +14,7 @@ set_tearing_hint(struct wl_listener *listener, void *data)
 {
 	struct tearing_controller *controller = wl_container_of(listener, controller, set_hint);
 	struct view *view = view_from_wlr_surface(controller->tearing_control->surface);
-	if (view) {
+	if (view && !view->tearing_override) {
 		/*
 		 * tearing_control->hint is actually an enum:
 		 * WP_TEARING_CONTROL_V1_PRESENTATION_HINT_VSYNC = 0
-- 
2.43.0

@Faugus
Copy link

Faugus commented Mar 9, 2024

@Ph42oN I just tested your changes and it's behaving the same way it was.

If I toggle it, it breaks the gamepad compatibility like before, but it doesn't seems to turn on the allowTearing.
The game looks like the allowTearing is always off, cause I don't see the jittering.

@Faugus
Copy link

Faugus commented Mar 9, 2024

G-SYNC Pendulum Demo is another great tool to test VRR, Tearing, Vsync...
Just run it with Wine. It uses DirectX11 (DXVK).
image

@Ph42oN
Copy link
Contributor

Ph42oN commented Mar 15, 2024

@Ph42oN I just tested your changes and it's behaving the same way it was.

Well my changes are to fix the problem that ToggleTearing does not override what application requests.

Im not sure why you are having problem, it works as expected in games for me, at least on xwayland. There was some talk about wine wayland previously... if its specific to that maybe i can test, but anyway wine wayland is unusable because camera does not turn at all with mouse on it (and this is not labwc specific, i tested on sway too).

@Consolatis
Copy link
Member Author

Consolatis commented Mar 16, 2024

I can't really see an issue in the logic of this PR and can't test it so if somebody else wants to take this over feel free.

Regarding the question of overriding what a client sets with the protocol: unless the client repeatedly sets it to disabled in an endless loop, the ToggleTearing action should overwrite whatever was set by a client (with only this PR applied) so I am not sure what is going on here.

I think it is important that a client can disable tearing via the protocol, especially if we enable it automatically based on the fullscreen state of a client. It should still be possible for the user to override that manually via the action though.

@Ph42oN
Copy link
Contributor

Ph42oN commented Mar 18, 2024

Regarding the question of overriding what a client sets with the protocol: unless the client repeatedly sets it to disabled in an endless loop, the ToggleTearing action should overwrite whatever was set by a client (with only this PR applied) so I am not sure what is going on here.

That is what i would have expected too. But when making code to work with that expectation it doesn't work.

I believe tearing protocol was made to take info that clients were already giving on presentation, to not need to add tearing protocol support to every client. Maybe it just sets tearing hint based on that every frame.

Edit: Maybe best way would be to revert tearing hint to bool, and add variable that defaults to respecting tearing hint, and 2 other values to force enable or disable it, similarly to how its done on sway tearing patch.

@Consolatis Consolatis modified the milestones: 0.7.2, 0.7.3 Apr 11, 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.

None yet

5 participants