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

Magnifier is broken for pixman renderer #1822

Open
tokyo4j opened this issue May 16, 2024 · 9 comments
Open

Magnifier is broken for pixman renderer #1822

tokyo4j opened this issue May 16, 2024 · 9 comments
Labels
bug Something isn't working
Milestone

Comments

@tokyo4j
Copy link
Contributor

tokyo4j commented May 16, 2024

See #1820.

On pixman renderer, the border of magnifier is not drawn and damage tracking for magnifier doesn't work correctly.

According to @Consolatis, this is already fixed in wlroots (https://gitlab.freedesktop.org/wlroots/wlroots/-/merge_requests/4659).

I think either of following options should be taken before the next release:

  • Confirm the wlroots fix is backported and the issue is solved
  • Highly discourage users from using magnifier on pixman renderer
  • Disable magnifier for pixman renderer
@tokyo4j tokyo4j added this to the 0.7.3 milestone May 16, 2024
@Consolatis
Copy link
Member

My 2 cents:
Variant 1 would be the best, variant 3 is a good fallback approach, 2 doesn't sound that great.

For anybody wanting to test the fixes backported to wlroots 0.17:
https://gitlab.freedesktop.org/Consolatis/wlroots/-/tree/backport_017/pixman_render_perf

At least nested it did fix the pixman + magnifier issue for me but also causes a pixman + dropshadow issues instead so there is further work to be done on the wlroots side.

CC @spl237 @cillian64

@droc12345
Copy link
Contributor

I'm not sure the logic is 100% correct in scene-helpers.c
I upgraded to the latest git yesterday, and I noticed my idle watts went up from 3.1 to 5.1 (on average)
I chased it down to lab_wlr_scene_output_commit in scene-helpers..

I reworked it slightly and my watts went back to normal.
Basically I moved the checks for last_mag and wants_magnification to where they made sense.
I haven't tested the magnifier part, as I've not used it yet.

If interested I'll pull up a PR.

@spl237
Copy link
Contributor

spl237 commented May 20, 2024

Just to confirm, I've tried the wlroots patch referred to above on Pi, and it does indeed fix the magnifier under pixman for me as well, both windowed and full-screen. (And does also break drop shadows!)

@droc12345
Copy link
Contributor

droc12345 commented May 20, 2024

Try this patch #1832 and see if it still works for magnification.
I think it should I just rearranged and added extra checks.

Edit to add: just tested the fix that tokyo4j mentioned in the PR conversation and it seems to work.
So I closed that PR.

@Consolatis
Copy link
Member

It seems like the magnifier and drop shadows are both fixed in wlroots master / 0.18 (not yet tested though). However there is some hesitation to backport the two changesets to 0.17 (as its more than just a bugfix). So the question is if we can fix the wlroots 0.17 branch separately for this magnifier bug. Thoughts @cillian64 ?

@cillian64
Copy link
Contributor

cillian64 commented May 23, 2024

It seems like the magnifier and drop shadows are both fixed in wlroots master / 0.18 (not yet tested though).

I've just done a quick test and both the magnifier and drop-shadows work for me with pixman on wlroots master (using your chase/0.18 labwc branch with the magnifier cherry-picked)

However there is some hesitation to backport the two changesets to 0.17 (as its more than just a bugfix). So the question is if we can fix the wlroots 0.17 branch separately for this magnifier bug. Thoughts @cillian64 ?

A lot of the old pixman transform code (before my "performance" patch basically rewrote that function) just don't make any sense to me, especially how it handles source crop (see https://gitlab.freedesktop.org/wlroots/wlroots/-/merge_requests/4653#note_2386254) which is what I suspect is going wrong here. I briefly tried a few simple things I thought might fix it but it's remaining thoroughly broken with the magnifier.

I understand the hesitation to backport something which isn't a minimal bugfix but given how many issues we found with the old pixman transform code (and that nobody noticed them outside of drop shadows and the screen magnifier) it feels reasonable to backport my changes, I think we're unlikely to make it more broken than the original 0.17 release. (I also think that the old pixman transform function has such horrible performance that you wouldn't want to use the magnifier with it anyway)

@Consolatis
Copy link
Member

Consolatis commented May 23, 2024

Makes sense, should I prepare a 0.17 backport MR for wlroots then containing the pixman rewrite MR + the recent fixup MR or do you want to do so instead?

@cillian64
Copy link
Contributor

If you'd be able to do the MR that would be great, I'm going to be mostly AFK tomorrow and next week.

@Consolatis
Copy link
Member

Consolatis commented May 23, 2024

If you'd be able to do the MR that would be great, I'm going to be mostly AFK tomorrow and next week.

Will do over the weekend. Thanks for your work on the pixman renderer.

Edit:
https://gitlab.freedesktop.org/wlroots/wlroots/-/merge_requests/4691

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

5 participants