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

Prevent a layer shell infinite looping case #6709

Closed
wants to merge 1 commit into from

Conversation

stacyharper
Copy link
Contributor

@stacyharper stacyharper commented Dec 13, 2021

This commit try to fix a case we encounter when running wayout, a layer
shell to display text above the background.

When running wayout, other layer shell start to consume 100% of the CPU.
This seems related to a previous fix 744e85b. Then later, another commit
reworks this handle_surface_commit method 5fd5d64.

https://todo.sr.ht/~mil/sxmo-tickets/413

This is a tentative to fix the issue without missing the points of those
patches.

We basically try to not re-arrange layers if no layer changed.

Related to : #6546

This patch seems to solve our issue, but am I missing something ?

This commit try to fix a case we encounter when running swayout, a layer
shell to display text above the background.

When running wayout, other layer shell start to consume 100% of the CPU.
This seems related to a previous fix 744e85b. Then later, another commit
reworks this handle_surface_commit method 5fd5d64.

This is a tentative to fix the issue without missing the points of those
patches.

We basically try to not re-arrange layers if no layer changed.
@emersion
Copy link
Member

cc @vyivel

The change looks fine to me, it seems like an oversight in the patches referenced above?

@vyivel
Copy link
Member

vyivel commented Dec 13, 2021

I think this will break cases when a layer surface changes its keyboard interactivity but not anything else.

@emersion
Copy link
Member

Shouldn't this be handled by layer_surface->current.committed != 0?

@vyivel
Copy link
Member

vyivel commented Dec 13, 2021

With this patch, arrange_layers() will only be called if the layer has changed (which implies committed != 0).
At least for me, GitHub screws up indentation up making the flow of logic a bit hard to see; I recommend opening the full file.

@stacyharper
Copy link
Contributor Author

stacyharper commented Dec 13, 2021

This seems to cause some issues :

  • Sometime the swaybar do not display itself. Had to open my virtual keyboard to make all layer shell to pop
  • bemenu --center is not centered

@vyivel
Copy link
Member

vyivel commented Dec 13, 2021

Right, because changing anchor is also not handled.

@vyivel
Copy link
Member

vyivel commented Dec 13, 2021

Presumably what happens here is a loop of "Sway configures a layer surface <-> the client needlessly sets the same size/anchor/etc". While a client shouldn't really do this, it's perfectly valid behavior, so the fix would to be check not only committed mask but also if the values have actually changed.

@emersion
Copy link
Member

wlroots could have this logic, so that compositors don't need to manually check this.

@vyivel
Copy link
Member

vyivel commented Dec 13, 2021

That would make sense; also e.g. wlr_output_enable() unsets the flag if the value didn't change.
And for Consistency™ we would need to have the same logic everywhere. :P

@stacyharper
Copy link
Contributor Author

I tried this : https://gitlab.freedesktop.org/wlroots/wlroots/-/merge_requests/3402

It seems to work. What do you guys think of that ?

@emersion
Copy link
Member

Superseded by the wlroots MR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants