Closed Bug 1337487 Opened 3 years ago Closed 3 years ago

Disable webrender on windows if GPU process is *not* being used

Categories

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

Other Branch
defect

Tracking

()

RESOLVED FIXED
mozilla54

People

(Reporter: kats, Assigned: rhunt)

References

Details

(Whiteboard: [gfx-noted])

Attachments

(1 file, 1 obsolete file)

This should be easy enough to do on startup - we just need to change the code at [1] to intialize the gfxVar to false if we're on windows without the GPU process. I'm not sure offhand how to handle the case where we start using the GPU process and then downgrade back to non-GPU.

[1] https://hg.mozilla.org/projects/graphics/file/34521aa2e278/gfx/thebes/gfxPlatform.cpp#l694
Maybe just change the value in GPUProcessManager::DisableGPUProcess - or call back into a gfxPlatform function to make new decisions about acceleration.
So I can see us accumulating a few decisions about whether to use WebRender or not, besides the GPU process. It could be nice to use gfxFeature for accumulating all the decisions we make, and then set the gfxVar.

And yes, hooking into GPUProcessManager::DisableGPUProcess should work. All the compositors will be recreated or will have not been created yet, so everything should switch over.
Attached patch webrender-gpu-process.patch (obsolete) — Splinter Review
This adds a gfxFeature for tracking the decisions we make for using WebRender, and hooks in the discussed dependency on the GPU process.
Assignee: nobody → rhunt
Attachment #8835309 - Flags: review?(bugmail)
Comment on attachment 8835309 [details] [diff] [review]
webrender-gpu-process.patch

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

While I like the goal of the patch, I feel like the flow of decision-making is getting more convoluted rather than simpler, mostly because of the spread of the #ifdef. I think we should try to reduce using the #ifdef as much as possible (specially since we have a not-too-distant-future goal of turning on --enable-webrender on nightly builds). See comments below. Minusing for now, would like to see the updated patch.

::: gfx/config/gfxFeature.h
@@ +24,5 @@
>    _(DIRECT2D,                     Feature,      "Direct2D")                       \
>    _(D3D11_HW_ANGLE,               Feature,      "Direct3D11 hardware ANGLE")      \
>    _(DIRECT_DRAW,                  Feature,      "DirectDraw")                     \
>    _(GPU_PROCESS,                  Feature,      "GPU Process")                    \
> +  _(WEB_RENDER,                   Feature,      "Web Render")                     \

Let's change this to WEBRENDER (no underscore) and "WebRender" (no space) for consistency

::: gfx/thebes/gfxPlatform.cpp
@@ +2302,5 @@
>  
> +void
> +gfxPlatform::InitWebRenderConfig()
> +{
> +#ifdef MOZ_ENABLE_WEBRENDER

I'd prefer scoping this ifdef more narrowly, so that most of this function would not be wrapped in the ifdef, but there would be one hunk:

#ifndef MOZ_ENABLE_WEBRENDER
  featureWebRender.ForceDisable(
    FeatureStatus::Unavailable,
    "Build doesn't include WebRender",
    ...);
#endif

It would be best if this hunk was the last one in the file, so that it overrides all the other reasons for disabling.

@@ +2308,5 @@
> +
> +  featureWebRender.SetDefaultFromPref(
> +    gfxPrefs::GetWebRenderEnabledPrefName(),
> +    true,
> +    gfxPrefs::GetWebRenderEnabledPrefDefault());

Instead of setting the default from the pref, I'd prefer just setting the default to true (see my other comment in gfxPrefs.h for why). Then, we can do a one-time check of the pref here using Preferences::GetBool and call UserDisable on the feature if the pref is false.

@@ +2312,5 @@
> +    gfxPrefs::GetWebRenderEnabledPrefDefault());
> +
> +  // WebRender relies on the GPU process
> +  if (!gfxConfig::IsEnabled(Feature::GPU_PROCESS)) {
> +    featureWebRender.ForceDisable(

We should only check for the GPU process on windows, right? I imagine on Mac/Linux the GPU process check will return false always, and we don't want to block webrender because of that.

@@ +2320,5 @@
> +  }
> +
> +  if (InSafeMode()) {
> +    featureWebRender.ForceDisable(
> +      FeatureStatus::Blocked,

According to the FeatureStatus documentation in gfxTelemetry.h, "Unavailable" is the appropriate value to used (as opposed to Blocked) for safe-mode reasons.

@@ +2585,5 @@
> +/* static */ void
> +gfxPlatform::NotifyGPUProcessDisabled()
> +{
> +#ifdef MOZ_ENABLE_WEBRENDER
> +  if (gfxConfig::IsEnabled(Feature::WEB_RENDER)) {

I'd like to drop the use of #ifdef here

::: gfx/thebes/gfxPrefs.h
@@ +425,5 @@
> +#ifdef MOZ_ENABLE_WEBRENDER
> +  DECL_GFX_PREF(Live, "gfx.webrender.enabled",                 WebRenderEnabled, bool, true);
> +#else
> +  DECL_GFX_PREF(Live, "gfx.webrender.enabled",                 WebRenderEnabled, bool, false);
> +#endif

I'm not too happy with this. For one thing it's tagged as a "live" pref but it's not really "live" in that the user flipping it makes an immediate difference. I also don't like how we're ifdef'ing it, specially since the runtime value in all.js is also ifdef'd. I also don't like having this pref in gfxPrefs at all because then people might accidentally start using it in code instead of using the gfxVar, which is more correct.

For the latter reason I think it's better to just remove this entirely, and initialize the gfxFeature to enabled-by-default. (The scoped ifdef I suggested should set the runtime value to disabled, which has a higher priority).

I also think that with all the changes I suggested, we can also get rid of the ifdef in all.js. That way, when we decide to make --enable-webrender on by default, all we will need to change is to flip the pref in all.js from true to false, which will result in webrender being compiled in and enable-able, but disabled by default. And all the user has to do to enable it is flip the pref and restart.
Attachment #8835309 - Flags: review?(bugmail) → review-
I like those changes, it makes it simpler.

I'm mildly concerned about the case where --enable-webrender is set to true in nightly, but all.js is set to false. In that case everyone's Feature::WEBRENDER will be UserDisabled even though it's technically disabled by default. Using gfxPrefs gave us a way to get the default for this, although I agree people might be tempted to use gfxPref::WebRenderEnabled, which is bad.

I don't know how important that all is, but if it's important when we flip --enable-webrender and all.js we can flip something in this function.

Also it seems that the other checks for IsInSafeMode use BLOCKED as their reason, not unavailable. But that doesn't mean we can't use the right one here, so I changed it.

I also forgot to mention that I tested killing the GPU process once with WebRender enabled and it restarted in non WebRender alright (we don't restart the GPU process yet). There probably will be more work to keep this working. Especially for things that only run on start up. We can hook into the instant restart functionality for this.
Attachment #8835309 - Attachment is obsolete: true
Attachment #8835593 - Flags: review?(bugmail)
Comment on attachment 8835593 [details] [diff] [review]
webrender-gpu-process.patch

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

Looks good, thanks!

(In reply to Ryan Hunt [:rhunt] from comment #5)
> I don't know how important that all is, but if it's important when we flip
> --enable-webrender and all.js we can flip something in this function.

Yeah, agreed, we can probably make it disabled by default in the function and have a true pref force it on, or something along those lines. That should avoid having everybody misleadingly reporting UserDisabled.

> Also it seems that the other checks for IsInSafeMode use BLOCKED as their
> reason, not unavailable. But that doesn't mean we can't use the right one
> here, so I changed it.

Thanks.

> I also forgot to mention that I tested killing the GPU process once with
> WebRender enabled and it restarted in non WebRender alright (we don't
> restart the GPU process yet). There probably will be more work to keep this
> working. Especially for things that only run on start up. We can hook into
> the instant restart functionality for this.

Sounds good.

::: gfx/thebes/gfxPlatform.cpp
@@ +2595,5 @@
> +gfxPlatform::NotifyGPUProcessDisabled()
> +{
> +  if (gfxConfig::IsEnabled(Feature::WEBRENDER)) {
> +    gfxConfig::GetFeature(Feature::WEBRENDER).ForceDisable(
> +      FeatureStatus::Unavailable,

Just to confirm: this function should only ever get called on Windows, right? (Or generally, it should only get called when the GPU process fails for whatever reason).
Attachment #8835593 - Flags: review?(bugmail) → review+
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #6)
> 
> ::: gfx/thebes/gfxPlatform.cpp
> @@ +2595,5 @@
> > +gfxPlatform::NotifyGPUProcessDisabled()
> > +{
> > +  if (gfxConfig::IsEnabled(Feature::WEBRENDER)) {
> > +    gfxConfig::GetFeature(Feature::WEBRENDER).ForceDisable(
> > +      FeatureStatus::Unavailable,
> 
> Just to confirm: this function should only ever get called on Windows,
> right? (Or generally, it should only get called when the GPU process fails
> for whatever reason).

Yes it is never called unless the GPU process is enabled and then fails. I had to double check because I didn't think about that.
Pushed by rhunt@eqrion.net:
https://hg.mozilla.org/projects/graphics/rev/b8a64223f86e
Disable WebRender on windows when the GPU process is disabled r=kats
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
You need to log in before you can comment on or make changes to this bug.