1179051 - Add layers and rendering features to the telemetry environment

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[David Anderson [:dvander] - inactive, e-mail if emergency]

We want the graphics telemetry environment to contain information about which compositor and rendering modes we're going to use. We'd also like information on why we chose to use those modes (for example, were certain prefs enabled or was a driver blocked?) and what versions of each feature we managed to get.

It's sort of possible to reconstruct this data based on existing telemetry information, like user prefs, OS version, adapter info and the blocklist. But just having it directly will be a lot more accurate.

Initially I'd like feature status information for d3d11, d3d9, d2d, and opengl - for layers. Conceivably we could add WebGL features as well.

Comment 1

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: part 1, split up UpdateRenderMode

Make UpdateRenderMode() easier to follow by splitting it into a few helper functions. The only semantic change this should have, is that gfx.font_rendering.directwrite.enabled is no longer treated as a live value.

Comment 2

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: part 2, break up InitD3D11Devices

This splits up InitD3D11Devices into smaller functions, which will help later steps (for example not repeating tests we know to fail later, and logging each decision we make for telemetry).

It should be functionally equivalent with a few exceptions that (I hope) won't matter. For example, GetDXGIAdapter() is called as if it were idempotent. The old code cached its return value locally.

Comment 3

[Bas Schouten (:bas.schouten)]

Comment on attachment 8628026 [details] [diff] [review]
part 1, split up UpdateRenderMode

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

::: gfx/thebes/gfxWindowsPlatform.cpp
@@ +594,5 @@
>      }
>  
>      mRenderMode = RENDER_GDI;
>  
> +    mUseDirectWrite = gfxPrefs::DirectWriteFontRenderingEnabled();

Hrm, we need to force this to true when D2D is being used, or things will be screwed.

Comment 4

[Bas Schouten (:bas.schouten)]

Comment on attachment 8628128 [details] [diff] [review]
part 2, break up InitD3D11Devices

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

Looks good.

Comment 5

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: part 3, move compositor backend decision-making

This patch moves compositor backend decision-making into gfxPlatform. The goal is to have all preferences and blocklists checked in one place so it's easier to communicate to Telemetry. Hopefully this will also reduce complexity. The next patch will merge more checks into gfxPlatform and attempt to make the backend list static.

Specific changes:

 * mUseLayersAcceleration has been removed, since it is only used to determine compositor backends.
 * ComputeShouldAccelerate was mostly non-widget checks. Those have been moved to gfxPlatform::ShouldUseLayersAcceleration().
 * GetPreferredCompositorBackends was mostly non-widget checks, so that logic has been moved to gfxPlatform::GetAcceleratedCompositorBackends. On Windows this contained some per-widget checks, those have been moved into ComputeShouldAccelerate.
 * LayerManagerPrefs+GetLayerManagerPrefs have been removed since they were just wrappers around gfxPrefs. It got folded into gfxPlatform::ShouldUseLayersAcceleration and gfxPlatform::ComputeAcceleratedCompositorBackends.

I *think* this preserves all the old behavior.

Comment 6

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: part 4, move more checks into gfxPlatform

Move a few more checks. It's not clear whether we even need these, since device initialization should be doing the same checks. But as long as they're there we might as well put them in the right place.

Comment 7

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8631824 [details] [diff] [review]
part 3, move compositor backend decision-making

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

::: gfx/thebes/gfxPlatform.h
@@ +514,5 @@
> +    // Returns whether or not layers should be accelerated by default on this platform.
> +    virtual bool AccelerateLayersByDefault();
> +
> +    // Returns a prioritized list of available compositor backends for acceleration.
> +    virtual void GetAcceleratedCompositorBackends(nsTArray<mozilla::layers::LayersBackend>& aBackends);

Maybe make AccelerateLayersByDefault, GetAcceleratedCompositorBackends and SupportsBasicCompositor protected methods, since we don't want external things calling those.

Comment 8

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: part 5, api for telemetry

This patch adds a new nsIGfxInfo call to grab an object describing GFX features. The layout currently looks like this:

{
  "compositor": "d3d11",
  "d3d11": {
    "status": "available",
    "version": 0xblah,
    "warp": false,
  },
  "d2d": {
    "status": "available",
    "version": "1.1",
  }
}

The gfxPlatform changes sort of overlap with ScopedFeatureReporter, I expect they could be merged together. This patch also doesn't bother reporting D3D9 info (other than whether or not the compositor uses it), or the rendering mode (which right now is implied by the d2d status).

It also exposes an observer notification so Telemetry can update its environment when compositors are created.

Comment 9

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: part 6, telemetry reporting

Actual telemetry reporting. The schema that gets sent is in comment #8. Not all of the information is available when we first query nsIGfxInfo (name we have not created a window/compositor yet), so there's an additional notification that updates the environment when that happens. This notification is only set once.

Comment 10

[Georg Fritzsche [:gfritzsche]]

(In reply to David Anderson [:dvander] from comment #9)
> Created attachment 8633663 [details] [diff] [review]
> part 6, telemetry reporting
> 
> Actual telemetry reporting. The schema that gets sent is in comment #8. Not
> all of the information is available when we first query nsIGfxInfo (name we
> have not created a window/compositor yet), so there's an additional
> notification that updates the environment when that happens. This
> notification is only set once.

I'm off sick this week, if this is urgent please move it to another reviewer.

Comment 11

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8633347 [details] [diff] [review]
part 5, api for telemetry

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

::: gfx/thebes/gfxWindowsPlatform.cpp
@@ +469,2 @@
>    }
> +        

WHITESPACE! (panic)

@@ +475,4 @@
>    }
>  
> +  if (!IsVistaOrLater() ||
> +      !GetD3D11ContentDevice() ||

I'd prefer if you split out the s/GetD3D11Device/GetD3D11ContentDevice change into a separate patch for landing, since it's a (potentially) functional change.

@@ +475,5 @@
>    }
>  
> +  if (!IsVistaOrLater() ||
> +      !GetD3D11ContentDevice() ||
> +      !mDoesD3D11TextureSharingWork)

Texture sharing not working might be better under 'Failed'?

@@ +480,5 @@
>    {
> +    return FeatureStatus::Unavailable;
> +  }
> +
> +  if (InSafeMode()) {

The gfxTelemetry comment for Unavailable specifically mentions safe mode as a possible reason, so we should return that instead of Blocked or change the comment.

@@ +2060,5 @@
>    MOZ_ASSERT(!mD3D11Device); 
>  
>    DriverInitCrashDetection detectCrashes;
>    if (InSafeMode() || detectCrashes.DisableAcceleration()) {
> +    mD3D11Status = FeatureStatus::Blocked;

Same as above wrt safe mode.

::: widget/GfxInfoBase.h
@@ +105,5 @@
>    // (while subclasses check for more specific ones).
>    virtual const nsTArray<GfxDriverInfo>& GetGfxDriverInfo() = 0;
>  
> +  virtual void DescribeFeatures(JSContext* aCx, JS::Handle<JSObject*> obj);
> +  virtual bool InitFeatureObject(

This doesn't really need to be virtual does it?

::: widget/nsBaseWidget.cpp
@@ +1126,5 @@
>    WindowUsesOMTC();
>  
>    mLayerManager = lm.forget();
> +
> +  gfxPlatform::GetPlatform()->NotifyCompositorCreated(mLayerManager->GetBackendType());

I think you want GetCompositorBackendType() here, otherwise you'll just get LAYERS_CLIENT 100% of the time.

::: widget/windows/GfxInfo.cpp
@@ +1273,5 @@
> +    val = JS::BooleanValue(platform->IsWARP());
> +    JS_SetProperty(aCx, obj, "warp", val);
> +
> +    val = JS::BooleanValue(platform->DoesD3D11TextureSharingWork());
> +    JS_SetProperty(aCx, obj, "textureSharing", val);

This wasn't mentioned in the nsIGfxInfo.idl comment, not that it really matters.

Comment 12

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/94450f09642a
https://hg.mozilla.org/integration/mozilla-inbound/rev/bfea9da8cf9a

Comment 13

[David Anderson [:dvander] - inactive, e-mail if emergency]

I just reailzed part 3 widens the loophole where we can create a basic compositor after a d3d one, since the OMTC/acceleration checks are now separate. I'll fix that before landing.

Comment 14

[Alessio Placitelli [:Dexter]]

Comment on attachment 8633663 [details] [diff] [review]
part 6, telemetry reporting

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

This looks good, with the changes below addressed.

Also, could you please update the documentation in [0] in a separate patch and flag me for a review?

[0] - http://mxr.mozilla.org/mozilla-central/source/toolkit/components/telemetry/docs/environment.rst

::: toolkit/components/telemetry/TelemetryEnvironment.jsm
@@ +816,5 @@
>    _removeObservers: function () {
>      // Remove the search engine change and service observers.
>      Services.obs.removeObserver(this, SEARCH_ENGINE_MODIFIED_TOPIC);
>      Services.obs.removeObserver(this, SEARCH_SERVICE_TOPIC);
> +    Services.obs.removeObserver(this, COMPOSITOR_CREATED_TOPIC);

Please note that |_removeObservers| is never called: I've filed bug 1184458 for that.

@@ +837,5 @@
>          // Now that the search engine init is complete, record the default search choice.
>          this._updateSearchEngine();
>          break;
> +      case COMPOSITOR_CREATED_TOPIC:
> +        this._onCompositorCreated();

Could you please add a comment to explain that this is needed bacause "not all of the information is available when we first query nsIGfxInfo (name we have not created a window/compositor yet)"?

::: toolkit/components/telemetry/tests/unit/test_TelemetryEnvironment.js
@@ +429,5 @@
>        Assert.equal(typeof gfxData.monitors[0].scale, "number");
>      }
>    }
> +  Assert.equal(typeof gfxData.features, "object");
> +  Assert.equal(typeof gfxData.features.compositor, "string");

It would be great if you could test that the expected values are sent, like done with the vendor id [1], if that isn't too much effort.

[1] - https://dxr.mozilla.org/mozilla-central/source/toolkit/components/telemetry/tests/unit/test_TelemetryEnvironment.js?from=test_TelemetryEnvironment.js#439

Comment 15

[Jeff Muizelaar [:jrmuizel]]

It looks like this will cause us to try WARP on Windows 7 if D3D11Device creation fails.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/94450f09642a
https://hg.mozilla.org/mozilla-central/rev/bfea9da8cf9a

Comment 17

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: part 2.1, don't use WARP on win7

Comment 18

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/fb262fae5d13

Comment 19

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d2de18bbfedc
https://hg.mozilla.org/integration/mozilla-inbound/rev/08508716085a

Comment 20

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: part 7, update telemetry environment docs

Here's the follow-up for documentation.

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/fb262fae5d13
https://hg.mozilla.org/mozilla-central/rev/d2de18bbfedc
https://hg.mozilla.org/mozilla-central/rev/08508716085a

Comment 22

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c4f4027f9f3a
https://hg.mozilla.org/integration/mozilla-inbound/rev/90446493d402

Comment 23

[Ryan VanderMeulen [:RyanVM]]

Backed out for crashes.
https://treeherder.mozilla.org/logviewer.html#?job_id=11883132&repo=mozilla-inbound

https://hg.mozilla.org/integration/mozilla-inbound/rev/4a0fba2132eb

Comment 24

[Ryan VanderMeulen [:RyanVM]]

Also https://treeherder.mozilla.org/logviewer.html#?job_id=11882985&repo=mozilla-inbound

Comment 25

[David Anderson [:dvander] - inactive, e-mail if emergency]

Stupid omission. Pushing again w/ LAYERS_BASIC in the info switch.

Comment 26

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5b4aec6951dd
https://hg.mozilla.org/integration/mozilla-inbound/rev/7e15449cb6e8

Comment 27

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/5b4aec6951dd
https://hg.mozilla.org/mozilla-central/rev/7e15449cb6e8