1377321 - Do not use WebRender for small popups.

Status: RESOLVED FIXED

(Core :: Graphics: WebRender, defect, P1)

Description

[Nicolas Silva [:nical]]

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.

Comment 1

[Kartikaya Gupta (email:[e-mail])]

Why do we want to do this? And what would we use instead?

Comment 2

[Sotaro Ikeda [:sotaro]]

It seems that there is already a patch to fallback to BasicLayerManager(non-omtc).

Comment 3

[Nicolas Silva [:nical]]

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?

Comment 4

[Nicolas Silva [:nical]]

Attachment: Don't use WebRender with small popu widgets

As an alternative to falling back to the basic layer manager.

Comment 5

[Kartikaya Gupta (email:[e-mail])]

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

Comment 6

[Nicolas Silva [:nical]]

(In reply to Kartikaya Gupta (email:[e-mail]) 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.

Comment 7

[Nicolas Silva [:nical]]

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.

Comment 8

[Nicolas Silva [:nical]]

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.

Comment 9

[Markus Stange [:mstange]]

(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.

Comment 10

[Kartikaya Gupta (email:[e-mail])]

(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).

Comment 11

[Nicolas Silva [:nical]]

Nice! Looks like we can use BasicLayerManager if the widget's HasRemoteContent() returns false.

Comment 12

[Marco Mucci [:MarcoM]]

Deprioritized to P3 during triage - moving to Reserve Backlog.

Comment 13

[Nicolas Silva [:nical]]

Attachment: Only use WebRender for the top-level windows and with remote content.

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.

Comment 14

[Kartikaya Gupta (email:[e-mail])]

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.

Comment 15

[Nicolas Silva [:nical]]

(In reply to Kartikaya Gupta (email:[e-mail]) 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.

Comment 16

[Nicolas Silva [:nical]]

Attachment: Only use WebRender for the top-level windows and with remote content.

Updated patch.

Comment 17

[Kartikaya Gupta (email:[e-mail])]

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();

Comment 18

[Nicolas Silva [:nical]]

Good point, made this change locally.

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a99a53c8f13d
Only use WebRender for top-level windows and remote content. r=kats

Comment 20

[Kartikaya Gupta (email:[e-mail])]

Why leave-open?

Comment 21

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/a99a53c8f13d

Comment 22

[Nicolas Silva [:nical]]

(In reply to Kartikaya Gupta (email:[e-mail]) 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.

Comment 23

[Kartikaya Gupta (email:[e-mail])]

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.