Do not use WebRender for small popups.

RESOLVED FIXED in mozilla57

Status

()

defect
P1
normal
RESOLVED FIXED
2 years ago
Last year

People

(Reporter: nical, Assigned: nical)

Tracking

(Blocks 1 bug)

unspecified
mozilla57
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox56 unaffected, firefox57 unaffected)

Details

(Whiteboard: [wr-reserve] [gfx-noted])

Attachments

(1 attachment, 2 obsolete attachments)

As it turns out it isn't easy to do because a lot of code decides whether to pick webrender specific code paths depending on gfxVars::UseWebRender which is a global.
Why do we want to do this? And what would we use instead?
It seems that there is already a patch to fallback to BasicLayerManager(non-omtc).
Initializing webrender is pretty heavy-weight, so it's not great to have to go through it when right-clicking on something or when a tooltip appears because the user mouses over something.
Falling back to basic layers would be fine, where is this patch?
As an alternative to falling back to the basic layer manager.
Assignee: nobody → nical.bugzilla
Two points:
1) some popups can now host remote webextension content and so they need a layer manager that can do e10s (i.e. WebRenderLayerManager or ClientLayerManager). I believe that all of these popups are of type ePopupTypePanel and so are excluded from the small popup check, but it would be good to confirm that.
2) There is some previous related discussion about how we should not mix and match ClientLayerManager and WebRenderLayerManager at [1]. I'm not sure how much of it is still true or applicable here but if we fall back to Client then we should make sure we don't run afoul of those things.

[1] https://groups.google.com/forum/m/#!topic/mozilla.dev.tech.gfx/31oJLl8ic10
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #5)
> 2) There is some previous related discussion about how we should not mix and
> match ClientLayerManager and WebRenderLayerManager at [1].

My hope is that widgets flagged as small popups don't have videos or anything image bridge related so that we avoid the problem of ImageBridge not knowing what compositor will pick up the images. The points raised in the discussion you linked to are still valid, but my understanding is that it should be fine for small popup windows.
Bas told me that addons want to be able to use OMTC for potentially every types of popups, and that there might be something about adding a property to the window so that the addon can specify when it wants omtc/acceleration, but I don't know if/where this work is happening and its status.
I still think that "small popups" should not be affected unless a web extension can put a video in a tooltip which it should not be able to, but that deserves some more investigation.
That might have come out a bit out of context, my previous comment was in the context of potentially using BasicLayerManager instead of webrender for small popups.
Also on linux currently OMTC is disabled for small popups and there doesn't seem to be complaints about it.
(In reply to Nicolas Silva [:nical] from comment #8)
> Also on linux currently OMTC is disabled for small popups and there doesn't
> seem to be complaints about it.

Bug 1356317 comment 0 has one. But outside of OOP webextension panels, there isn't really a need for OMTC in popups.
(In reply to Nicolas Silva [:nical] from comment #7)
> Bas told me that addons want to be able to use OMTC for potentially every
> types of popups, and that there might be something about adding a property
> to the window so that the addon can specify when it wants omtc/acceleration,
> but I don't know if/where this work is happening and its status.

Bug 1365660 (landed), bug 1356317 (bounced once, about to land again).
Nice! Looks like we can use BasicLayerManager if the widget's HasRemoteContent() returns false.
Status: NEW → ASSIGNED
Priority: P3 → P1
Whiteboard: [gfx-noted] → [wr-mvp] [gfx-noted]
Target Milestone: --- → mozilla57
Deprioritized to P3 during triage - moving to Reserve Backlog.
Assignee: nical.bugzilla → nobody
Status: ASSIGNED → NEW
Priority: P1 → P3
Whiteboard: [wr-mvp] [gfx-noted] → [wr-reserve] [gfx-noted]
Assignee: nobody → nical.bugzilla
This patch disables WebRender for widgets that aren't the main window(s) unless they have remote content.
Since there has been some uncertainty around remote content this patch tries to not change the behaviour for them as a first step, but I'd like to eventually stop using webrender for pops with remote content as well.
In the mean time we'll see if this more limited change highlights interesting bugs (gfxVars::UseWebRender() being global).

The most important change is probably that with this patch, the awesome bar's menu uses the basic layer manager instead of WebRender, which is great because it won't take 4 seconds for this widget to get created anymore, but I suppose we should be careful about moving stuff to the parent process' main thread. It would probably be better if we used OMTC for that but I don't know how much work that would represent. I think (but I don't have one to handy to check) that webrender is already disabled for the awesome bar's menu on mac.
Attachment #8882470 - Attachment is obsolete: true
Attachment #8941803 - Flags: review?(bugmail)
Comment on attachment 8941803 [details] [diff] [review]
Only use WebRender for the top-level windows and with remote content.

Review of attachment 8941803 [details] [diff] [review]:
-----------------------------------------------------------------

Patch looks ok but I'd like to see the updated version with comments addressed.

::: widget/nsBaseWidget.cpp
@@ +886,5 @@
>  nsBaseWidget::ComputeShouldAccelerate()
>  {
>    return gfx::gfxConfig::IsEnabled(gfx::Feature::HW_COMPOSITING) &&
> +         WidgetTypeSupportsAcceleration() &&
> +         !(gfx::gfxVars::UseWebRender() && DisableWebRenderForThisWindow());

This change needs a comment, preferably by extracting this new clause into a local variable with a meaningful name and/or inline comment. I had a hard time understanding the purpose of this, but I'm guessing it's what makes us fallback to basic instead of client, in the case where DisableWebRenderForThisWindow returns true - is that correct?

@@ +902,5 @@
>  
> +bool
> +nsBaseWidget::DisableWebRenderForThisWindow()
> +{
> +  return WindowType() != eWindowType_toplevel && !HasRemoteContent();

I'd much prefer if you inverted everything, so that the function was called AllowWebRenderForThisWindow() instead. Too many negations make the code hard to read.
Attachment #8941803 - Flags: review?(bugmail) → review-
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #14)
> This change needs a comment, preferably by extracting this new clause into a
> local variable with a meaningful name and/or inline comment. I had a hard
> time understanding the purpose of this, but I'm guessing it's what makes us
> fallback to basic instead of client, in the case where
> DisableWebRenderForThisWindow returns true - is that correct?

Yes, I updated the patch with a comment.

> I'd much prefer if you inverted everything, so that the function was called
> AllowWebRenderForThisWindow() instead. Too many negations make the code hard
> to read.

I agree, the patch reflects how my brain formulated the problem the first time around but upon re-reading I also find it simpler the other way around.
Updated patch.
Attachment #8941803 - Attachment is obsolete: true
Attachment #8942159 - Flags: review?(bugmail)
Comment on attachment 8942159 [details] [diff] [review]
Only use WebRender for the top-level windows and with remote content.

Review of attachment 8942159 [details] [diff] [review]:
-----------------------------------------------------------------

Much better, thanks!

::: widget/nsBaseWidget.cpp
@@ +893,2 @@
>    return gfx::gfxConfig::IsEnabled(gfx::Feature::HW_COMPOSITING) &&
> +         WidgetTypeSupportsAcceleration() && !wrEnabledButNotForThis;

On reading this I realized it might be even clearer to do this:

if (gfx::gfxVars::UseWebRender() && !AllowWebRenderForThisWindow()) {
  // your comment here
  return false;
}
return gfx::gfxConfig::IsEnabled(gfx::Feature::HW_COMPOSITING) &&
  WidgetTypeSupportsAcceleration();
Attachment #8942159 - Flags: review?(bugmail) → review+
Good point, made this change locally.
Pushed by nsilva@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a99a53c8f13d
Only use WebRender for top-level windows and remote content. r=kats
Keywords: stale-bugleave-open
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #20)
> Why leave-open?

Because we should probably also disable webrender for windows with remote content too, or at least that's my intial goal but I made it a two step thing to avoid unleashing all of the regressions at once.
I'd prefer to spin that out into a new bug. Any patches for that will almost certainly land in a different firefox nightly version since merge day is coming up.
Duplicate of this bug: 1391086
Status: NEW → RESOLVED
Closed: Last year
Keywords: leave-open
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.