Closed Bug 1617002 Opened 4 years ago Closed 4 years ago

[Webrender][X11] Opaque region does not get set / gets reset

Categories

(Core :: Widget: Gtk, defect)

Product:
Core
Component:
Widget: Gtk
Version:
73 Branch
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
81 Branch
Tracking Flags:
Tracking Status
firefox81 --- fixed

People

(Reporter: rmader, Assigned: rmader)

References

(Blocks 1 open bug)

Details

Attachments

(7 files)

basic.png
475.65 KB, image/png
Details
opengl.png
458.67 KB, image/png
Details
webrender.png
458.88 KB, image/png
Details
opaque_region_overlay.png
551.72 KB, image/png
Details
Bug 1617002 - Remove checks for unsupported GTK3 versions. r=stransky
47 bytes, text/x-phabricator-request
Details | Review
Bug 1617002 - Rework GTK opaque region handling. r=stransky,jhorak
47 bytes, text/x-phabricator-request
Details | Review
webrender-fixed.png
608.77 KB, image/png
Details

Summary: when using WR we always use textures with alpha channel. Thus we need to set an opaque region to let the OS compositor know that it can skip painting everything behind the FF window (minus rounded corners). This is already implemented but does not work properly on X11 atm.

User Agent: Mozilla/5.0 (X11; Fedora; Linux x86_64; rv:73.0) Gecko/20100101 Firefox/73.0

Steps to reproduce:

Working on culling in Mutter (Gnome Shell), testing different applications.

Exact steps:

  • use Mutter >= 3.35.91
  • use the Wayland session
  • start weston-simple-damage --verbose in a terminal (part of Weston demo clients)
  • hide the demo app behind other applications
  • if the application sets an opaque region or doesn't have an alpha channel, weston-simple-damage will get culled out, therefore not receive frame callbacks any more -> output in terminal stops

Actual results:

demo app gets culled out behind Firefox on Wayland (basic, ogl, Webrender) and X11 (basic, ogl)

demo app does not get culled out behind Firefox X11 Webrender

Expected results:

Should get culled out, too.

I did some testing quick testing in nightly and apparently there's nothing wrong in the part in nsWindow.cpp (it sets the region and does not unset it). On resize, Mutter first receives a valid region and then NULL again. Now I'm stuck.

Blocks: wr-linux
See Also: → 1648872

While looking into bug 1648872 I stumbled over this again and it's still valid. Fixing this should allow us to remove the duplicated code in UpdateTopLevelOpaqueRegionWayland, getting rid of one occurrence of gdk_window_invalidate_rect which might be the cause for bug 1648872 (this is pure guessing atm).

Blocks: wr-linux-mvp

Without going through the steps in comment 0 (just running a local build on x11 with webrender enabled), I see gdk_window_set_opaque_region being called. Is this specific to a certain configuration?

(In reply to Nicolas Silva [:nical] from comment #2)

Without going through the steps in comment 0 (just running a local build on x11 with webrender enabled), I see gdk_window_set_opaque_region being called. Is this specific to a certain configuration?

Yes, it's getting called but it does not have the desired effect - I don't think it's specific to a configuration, it certainly applies to default settings (I also tested with and without title bar). AFAICS the problem is that GTK also calls gdk_window_set_opaque_region internally, directly unsetting the region again - and I think the problem is that we don't call it the way it's intended by GTK (we don't seem to follow the docs).

I'm planning to look into this again until the weekend, just so you know.

P.S.: I also assume this affects all DEs (as I think it's a wrong handling of GTK), but didn't test yet

In order to make it easier to validate opaque region usage I created https://gitlab.gnome.org/GNOME/mutter/-/merge_requests/1372, which will allow us to get an easy to use visual feedback (if accepted of course).

I was actually wrong about the layers/opengl render - it's also affected. The default basic/software render work fine.

Attached image basic.png —
Attached image opengl.png —
Attached image webrender.png —

Looking at the code this is roughly how it should look like: cut out corners in the top-left and top-right (basic mode uses Xshape - we don't want that when using WR :) ). Just like most GTK3 applications.

(Is bug 1572625 comment 13 relevant? Would disabling layers.gpu-process.enabled help on X11?)

See darkspirit's question. We intend to disable the GPU process by default, and I'm curious if this is another sync issue we are seeing....seems likely given the others issues we have seen with i3, which you have managed to see with mutter/gnome. You can disable the GPU process by flipping the pref layers.gpu-process.enabled to false.

Can you attach your about:support? Mostly I want to know if you are using Wayland, or XWayland.

Flags: needinfo?(robert.mader)

(In reply to Andrew Osmond [:aosmond] from comment #10)

See darkspirit's question. We intend to disable the GPU process by default, and I'm curious if this is another sync issue we are seeing....seems likely given the others issues we have seen with i3, which you have managed to see with mutter/gnome. You can disable the GPU process by flipping the pref layers.gpu-process.enabled to false.

Can you attach your about:support? Mostly I want to know if you are using Wayland, or XWayland.

Haven't pulled the patch adding XWayland to about:support, but I do run on Xwayland. Just tried with layers.gpu-process.enabled=false and it doesn't change anything - but as I said before, I'm quite sure it's GTK related.

FTR: I'm trying to debug this right now, just finalizing a patch that is I think is good to have beforehand, https://phabricator.services.mozilla.com/D82804

Flags: needinfo?(robert.mader)

Cleans up the code a bit to make actually used fallback code easier to spot
and update the required GTK version so deprecation warnings are more accurate.

Also make gdk_window_set_opaque_region always available - we can now assume
it to be present in all supported versions.

Assignee: nobody → robert.mader
  • always update the opaque region if the toplevel window allocation
    changes - this fixes opaque regions on X11 with WR and OGL renderers,
    as GTK resets the opaque region in this case. The previous solution
    was modeled similar to what's done on Windows - we have to follow GTK
    though.
  • use gdk_window_set_opaque_region also for Wayland - this fixes
    occasional glitches cases where the opaque region was not properly
    updated. Now we let GTK handle that.
  • remove a failed attempt to work around bug 1615098 from
    MozContainerWayland
  • repurpose widget.wayland.use-opaque-region for subsurface opaque
    regions and disable it by default. We want to enable it again eventually
    and not all Wayland compositors suffer from bug 1615098
  • some cleanups - some optimizations should not be needed anymore now

Depends on D82804

Depends on D82804

Attachment #9164460 - Attachment description: Bug 1617002 - Rework GTK opaque region handling → Bug 1617002 - Rework GTK opaque region handling. r=stransky,jhorak
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Attached image webrender-fixed.png —

All green now. Sorry for the noise, I just like the opaque region color feature :)

Blocks: 1648872
See Also: → 1625070

Note: gdk_window_set_opaque_region on X11 only affects EWMH capable window managers - simple WMs / compositors will likely ignore it. See https://developer.gnome.org/wm-spec/#idm45384480021120

Another note: this affects full screen usage. AFAIK firefox requests compositor bypass in that case (at least for video playback etc.), increasing performance at the risk of tearing. Before this patch, I assume most compositors just ignored that request, due to the texture being potentially transparent. So this patch might turn on fullscreen unredirect for some users, which again might introduce tearing for them, if something goes wrong with VSync. Didn't test that yet, but in case reports appear: it's expected and would need to be solved differently.

Keywords: leave-open
Pushed by abutkovits@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b9853df48d89
Remove checks for unsupported GTK3 versions. r=stransky

Martin, I think I have a somewhat clean an liable solution now, but I'm struggling with the title bar case. Are you aware of any reliable way to get its size? Unfortunately it looks like we need to set the opaque region for it as well.

Alternatively we can just ignore that case for now.

Flags: needinfo?(stransky)

(In reply to Robert Mader [:rmader] from comment #20)

Martin, I think I have a somewhat clean an liable solution now, but I'm struggling with the title bar case. Are you aware of any reliable way to get its size? Unfortunately it looks like we need to set the opaque region for it as well.

Alternatively we can just ignore that case for now.

Which titlebar do you mean?

Flags: needinfo?(stransky)

Is that what do you mean?

I tested the latest version with MOZ_GTK_TITLEBAR_DECORATION=client and MOZ_GTK_TITLEBAR_DECORATION=system on X11, as well as Wayland. I all cases things should now work nicely if no classic title bar is used. If it is used, it will not be set as opaque in combination with WR or layers - which is still better then the current status, so I'd prefer to get this reviewed and pushed already.

(In reply to Martin Stránský [:stransky] from comment #22)

Is that what do you mean?

Yes, the classic title bar, !mDrawInTitlebar. As far as I can see the calculation for it is different each time for MOZ_GTK_TITLEBAR_DECORATION=client on X11, MOZ_GTK_TITLEBAR_DECORATION=system on X11 and Wayland. If I understand things correctly, normally GTK would handle that for us - but the opaque region gets unset for the whole window / surface on allocation change. So we'd need to include the title bar in our opaque region.

For me on Fedora 32 it works when just I expand the opaque region 37px to the top - but I struggle to find a way to calculate these 37px reliably in the different cases.

(In reply to Robert Mader [:rmader] from comment #23)

(In reply to Martin Stránský [:stransky] from comment #22)

Is that what do you mean?

Yes, the classic title bar, !mDrawInTitlebar. As far as I can see the calculation for it is different each time for MOZ_GTK_TITLEBAR_DECORATION=client on X11, MOZ_GTK_TITLEBAR_DECORATION=system on X11 and Wayland. If I understand things correctly, normally GTK would handle that for us - but the opaque region gets unset for the whole window / surface on allocation change. So we'd need to include the title bar in our opaque region.

Can we force Gtk somehow to re-set the opaque region after the allocation change or can we just set whole widget (mShell) opaque?

Martin, given that bug 1653612 has landed I guess we should make sure D84024 also lands in time for FF 80 - even when missing out on title bars. Depending on the scenario, opaque regions can heavily influence composition times, so especially for bandwidth constrained GPUs like weak intel iGPUs this is very important to not regress in comparison to software rendering. I'll try to update as soon as possible if you find more issues to be addressed :)

Flags: needinfo?(stransky)

(In reply to Robert Mader [:rmader] from comment #25)

Martin, given that bug 1653612 has landed I guess we should make sure D84024 also lands in time for FF 80 - even when missing out on title bars. Depending on the scenario, opaque regions can heavily influence composition times, so especially for bandwidth constrained GPUs like weak intel iGPUs this is very important to not regress in comparison to software rendering. I'll try to update as soon as possible if you find more issues to be addressed :)

Yes I understand you. OTOH I'm bit scared of such radical changes in beta without testing at nightly as our ability to fix beta is limited. So I'd rated ship that at 81 first even with the possible performance pennality. IMHO any GPU based rendering is faster than SW compositing/rendering.

Flags: needinfo?(stransky)

(In reply to Martin Stránský [:stransky] from comment #26)

Yes I understand you. OTOH I'm bit scared of such radical changes in beta without testing at nightly as our ability to fix beta is limited.
So I'd rated ship that at 81 first even with the possible performance pennality.

Agreed :)

IMHO any GPU based rendering is faster than SW compositing/rendering.

Unfortunately I can guarantee you that this is not true for certain scenarios. Some users will hit bandwidth limits and therefore frame-drops because of this, especially when having multiple windows stacked on top of each other. As long as people don't do that they should be fine though.

Pushed by btara@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/16bf991df000
Rework GTK opaque region handling. r=stransky
Keywords: leave-open
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
status-firefox81: --- → fixed
Target Milestone: --- → 81 Branch

FTR: I just accidentally verified that Mutter 3.37/3.38 does now use fullscreen unredirect for fullscreen videos with this - so just in case people start to see tearing with WR now they should disable that.

Re-enabling widget.wayland.use-opaque-region after this patch reintroduces bug 1615098 (or similar symptoms) on Mutter 3.36.4 (latest Nightly, Wayland, WebRender).

Is this expected?

(In reply to Ivan Shapovalov from comment #31)

Re-enabling widget.wayland.use-opaque-region after this patch reintroduces bug 1615098 (or similar symptoms) on Mutter 3.36.4 (latest Nightly, Wayland, WebRender).

Is this expected?

Yep :) I re-purposed that setting for the subsurface, as that's where things can break. That's also why the setting is switched off by default, still.

Regressions: 1661715
Regressions: 1666839
See Also: → 1750737
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: