Closed Bug 1179051 Opened 9 years ago Closed 9 years ago

Add layers and rendering features to the telemetry environment


(Core :: Graphics, defect)

Not set





(Reporter: dvander, Assigned: dvander)




(8 files)

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.
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.
Attachment #8628026 - Flags: review?(bas)
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.
Attachment #8628128 - Flags: review?(bas)
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.
Attachment #8628026 - Flags: review?(bas) → review+
Comment on attachment 8628128 [details] [diff] [review]
part 2, break up InitD3D11Devices

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

Looks good.
Attachment #8628128 - Flags: review?(bas) → review+
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.
Attachment #8631824 - Flags: review?(matt.woodrow)
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.
Attachment #8631833 - Flags: review?(matt.woodrow)
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.
Attachment #8631824 - Flags: review?(matt.woodrow) → review+
Attachment #8631833 - Flags: review?(matt.woodrow) → review+
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.
Attachment #8633347 - Flags: review?(matt.woodrow)
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.
Attachment #8633663 - Flags: review?(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.
Attachment #8633663 - Flags: review?(gfritzsche) → review?(alessio.placitelli)
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 @@
>    }
> +        


@@ +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.
Attachment #8633347 - Flags: review?(matt.woodrow) → review+
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.
Keywords: leave-open
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] -

::: 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;
> +        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] -
Attachment #8633663 - Flags: review?(alessio.placitelli) → review+
It looks like this will cause us to try WARP on Windows 7 if D3D11Device creation fails.
Attachment #8634872 - Flags: review?(jmuizelaar) → review+
Here's the follow-up for documentation.
Attachment #8635128 - Flags: review?(alessio.placitelli)
Attachment #8635128 - Flags: review?(alessio.placitelli) → review+
Stupid omission. Pushing again w/ LAYERS_BASIC in the info switch.
Closed: 9 years ago
Keywords: leave-open
Resolution: --- → FIXED
Blocks: 1212858
Target Milestone: --- → mozilla42
You need to log in before you can comment on or make changes to this bug.