Closed Bug 1254899 Opened 4 years ago Closed 3 years ago

Clean up acceleration decision-making

Categories

(Core :: Graphics, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla49
Tracking Status
firefox49 --- fixed

People

(Reporter: dvander, Assigned: dvander)

References

(Blocks 1 open bug)

Details

Attachments

(15 files, 8 obsolete files)

8.56 KB, patch
jrmuizel
: review+
Details | Diff | Splinter Review
39.46 KB, patch
milan
: review+
jrmuizel
: feedback+
Details | Diff | Splinter Review
6.39 KB, patch
milan
: review+
Details | Diff | Splinter Review
9.00 KB, patch
milan
: review+
Details | Diff | Splinter Review
12.31 KB, patch
milan
: review+
Details | Diff | Splinter Review
12.80 KB, patch
milan
: review+
Details | Diff | Splinter Review
15.93 KB, patch
milan
: review+
Details | Diff | Splinter Review
38.99 KB, patch
jrmuizel
: review+
Details | Diff | Splinter Review
24.57 KB, patch
milan
: review+
Details | Diff | Splinter Review
20.86 KB, patch
jrmuizel
: review+
Details | Diff | Splinter Review
4.58 KB, patch
milan
: review+
Details | Diff | Splinter Review
14.32 KB, image/png
Details
1.05 KB, patch
snorp
: review+
Details | Diff | Splinter Review
4.66 KB, patch
milan
: review+
Details | Diff | Splinter Review
6.75 KB, patch
jrmuizel
: review+
Details | Diff | Splinter Review
Our decision-making for acceleration is very hairy and spread across a large number of files. Often similar logic is duplicated or copy-pasta'd around and relies on raw access to prefs. This makes it difficult to see why we're in a certain state.
Attached patch prototype (obsolete) — Splinter Review
The idea I'm playing with is to have a layer on top of nsIGfxInfo and gfxPrefs. The platform will be responsible for taking everything it knows and initializing a new service, gfxConfig. gfxConfig will record each decision we make as well as why we made it. It'll also be the single point of truth to avoid testing random prefs and features all over the place.

gfxConfig has two kinds of knowledge: "parameters" which are currently just feature states, and "fallbacks" which is an indicator that something is working suboptimally or requires a special case.

A parameter has three sub-states:
 (1) The initial state determined by the platform.
 (2) The user state determined by preferences.
 (3) The runtime state, which can be set if a new condition is determined at runtime.

They take effect in reverse order: runtime overrides user, and user overrides initial. Only the initial state must be present. Each state is associated with a message if we consider the state to be abnormal.

"Fallbacks" are like parameters, except simpler. They're just a bit that gets toggled. They indicate something went wrong, and let us separate complicated checking logic from the code that will use the check.

The attached patch is just a prototype; it compiles but doesn't link since it only demonstrates the API and not the implementation. In this patch, all layers acceleration gfxPrefs have been converted to gfxConfig. Note that ShouldUseAcceleration has been folded into the initialization process and now former callers can query it as if it had the same meaning as the layers-acceleration pref.

Not that it's a great example of what a fallback is, but I also converted the HasBogusPopup weirdness in nsWindow to be a one-time-initialized fallback.

If this is the direction we want to go, all of the various acceleration prefs would go into gfxConfig.
Attachment #8728308 - Flags: feedback?(milan)
Attachment #8728308 - Flags: feedback?(jmuizelaar)
Comment on attachment 8728308 [details] [diff] [review]
prototype

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

Some of the comments are as I was reading different files, so I may have answered my own questions.  I like the overall direction.  I can see us taking out the configuration like things out of gfx*Platform eventually, or perhaps only leaving those in, and taking the rest out, but that can wait until we get further.  The only thin I don't like is removing the gfxPrefs entry, for the reasons stated below.

::: gfx/config/gfxFallback.h
@@ +17,5 @@
> +  _(POPUPS_MUST_USE_WS_EX_COMPOSITED)                                             \
> +  _(DISABLE_POPUP_SHADOWS_ON_MULTI_MONITOR)                                       \
> +  /* Add new entries above this comment */
> +
> +enum class Fallback : uint32_t {

This is capturing all the places where we don't do some functionality, or just things that are "errors"?

::: gfx/config/gfxParameter.h
@@ +20,5 @@
> +enum class Parameter : uint32_t {
> +#define MAKE_ENUM(name, type, desc) name,
> +  GFX_PARAMETER_MAP(MAKE_ENUM)
> +#undef MAKE_ENUM
> +  NumValues

Haven't seen this pattern, so I'll just write out what appears to be the goal.  We end up with this:

enum class Parameter : uint32_t {
HW_COMPOSITING,
SOMETHING_ELSE,
...
NumValues
};
but the code lets us "document" with type and description when using MAKE_ENUM, and maybe MAKE_ENUM does more in the future?

@@ +23,5 @@
> +#undef MAKE_ENUM
> +  NumValues
> +};
> +
> +class ParameterState

Perhaps early to talk about naming things, but I'm not sure what Parameter is representing, so more of a description would help.  What does it do?

::: gfx/src/gfxTelemetry.h
@@ +40,5 @@
>    // This feature is available for use.
> +  Available,
> +
> +  // This feature was explicitly force-enabled by the user.
> +  ForceEnabled

Does it make sense to have Disabled now be ForceDisabled, to match?  Or, perhaps UserEnabled, UserDisabled?

::: gfx/thebes/gfxPlatform.cpp
@@ +2054,5 @@
> +                            AccelerateLayersByDefault(),
> +                            FeatureStatus::Blocked,
> +                            "Acceleration blocked by platform"))
> +  {
> +    if (Preferences::GetBool("layers.acceleration.disabled", false)) {

gfxPrefs::LayersAccelerationDisabled() instead.

@@ +2066,5 @@
> +    }
> +  }
> +
> +  // This has specific meaning elsewhere, so we always record it.
> +  if (Preferences::GetBool("layers.acceleration.force-enabled", false)) {

gfxPrefs::LayersAccelerationForceEnabled() instead

::: gfx/thebes/gfxPrefs.h
@@ +314,5 @@
>    DECL_GFX_PREF(Once, "image.mem.surfacecache.size_factor",    ImageMemSurfaceCacheSizeFactor, uint32_t, 64);
>    DECL_GFX_PREF(Live, "image.mozsamplesize.enabled",           ImageMozSampleSizeEnabled, bool, false);
>    DECL_GFX_PREF(Once, "image.multithreaded_decoding.limit",    ImageMTDecodingLimit, int32_t, -1);
>    DECL_GFX_PREF(Live, "image.single-color-optimization.enabled", ImageSingleColorOptimizationEnabled, bool, true);
>  

This won't really stop people from using the preference, and we're now running a chance of getting the values from the wrong thread, and we lose the fact that this is a "needs restart" type of a preference.
Better way may be to just rename the function name from LayersAccelerationDisabled to ... DoNotUseDirectly_LayersAccelerationDisabled or something like that?
Attachment #8728308 - Flags: feedback?(milan) → feedback+
> ::: gfx/config/gfxFallback.h
> @@ +17,5 @@
> > +  _(POPUPS_MUST_USE_WS_EX_COMPOSITED)                                             \
> > +  _(DISABLE_POPUP_SHADOWS_ON_MULTI_MONITOR)                                       \
> > +  /* Add new entries above this comment */
> > +
> > +enum class Fallback : uint32_t {
> 
> This is capturing all the places where we don't do some functionality, or
> just things that are "errors"?

It's intended to be places where we need to change our decision-making, but still keep acceleration. Exactly where you draw the line would probably never be clear, but ... something like "use D3D9 for ANGLE instead of D3D11" would be an example. Or something where we have to take a slow path in rasterization or compositing.

> ::: gfx/config/gfxParameter.h
> @@ +20,5 @@
> > +enum class Parameter : uint32_t {
> > +#define MAKE_ENUM(name, type, desc) name,
> > +  GFX_PARAMETER_MAP(MAKE_ENUM)
> > +#undef MAKE_ENUM
> > +  NumValues
> 
> Haven't seen this pattern, so I'll just write out what appears to be the
> goal.  We end up with this:
> 
> enum class Parameter : uint32_t {
> HW_COMPOSITING,
> SOMETHING_ELSE,
> ...
> NumValues
> };
> but the code lets us "document" with type and description when using
> MAKE_ENUM, and maybe MAKE_ENUM does more in the future?

Yeah - for example, it's really easy to generate tables or helper methods with this pattern. Like a name namp:

> static const char** kParameterNames[] = {
> #define MAKE_ENTRY(name) ##name
>   GFX_PARAMETER_MAP(MAKE_ENTRY)
> #undef MAKE_ENTRY
> };

> Perhaps early to talk about naming things, but I'm not sure what Parameter
> is representing, so more of a description would help.  What does it do?

The intent was another word for "Preference" - except restricted to boolean decisions. But "Feature", "Variable", anything along those lines would work too.

> ::: gfx/src/gfxTelemetry.h
> @@ +40,5 @@
> >    // This feature is available for use.
> > +  Available,
> > +
> > +  // This feature was explicitly force-enabled by the user.
> > +  ForceEnabled
> 
> Does it make sense to have Disabled now be ForceDisabled, to match?  Or,
> perhaps UserEnabled, UserDisabled?

"ForceDisabled" is implied by the user state being "Disabled". If the user disables something, we'll never override that decision. The  "ForceEnabled" case is to differentiate from a situation where the user really wants acceleration so bad they're willing to override a blacklist entry.
 
> This won't really stop people from using the preference, and we're now
> running a chance of getting the values from the wrong thread, and we lose
> the fact that this is a "needs restart" type of a preference.
> Better way may be to just rename the function name from
> LayersAccelerationDisabled to ...
> DoNotUseDirectly_LayersAccelerationDisabled or something like that?

That makes sense, I can keep them in a do-not-touch section.
(In reply to David Anderson [:dvander] from comment #3)
> ...
> That makes sense, I can keep them in a do-not-touch section.

Just thought of it - we could also make them private, and have gfxPlatform be a friend?
Comment on attachment 8728308 [details] [diff] [review]
prototype

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

::: gfx/layers/d3d11/CompositorD3D11.cpp
@@ +194,5 @@
>  
>  bool
>  CompositorD3D11::Initialize()
>  {
> +  ScopedGfxFeatureReporter reporter("D3D11 Layers");

ScopedGfxFeatureReporter will add an '!' if forced. Are you intentionally dropping that functionality?
(In reply to Jeff Muizelaar [:jrmuizel] from comment #5)
> Comment on attachment 8728308 [details] [diff] [review]
> prototype
> 
> Review of attachment 8728308 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/layers/d3d11/CompositorD3D11.cpp
> @@ +194,5 @@
> >  
> >  bool
> >  CompositorD3D11::Initialize()
> >  {
> > +  ScopedGfxFeatureReporter reporter("D3D11 Layers");
> 
> ScopedGfxFeatureReporter will add an '!' if forced. Are you intentionally
> dropping that functionality?

For those instances, yes - it's data available elsewhere in the crash report. It's not really that hard to add back if we want.
This just fixes a bunch of 4-space indents in gfxWindowsPlatform.
Attachment #8730491 - Flags: review?(jmuizelaar)
This movies InitLayersAccelerationPrefs into gfxPlatform::InitAcceleration, a new function that gets called immediately after gfxPlatform is constructed. gfxWindowsPlatform now overrides this to also do device creation.
Attachment #8730492 - Flags: review?(jmuizelaar)
This is similar to the prototype, except now it's got an implementation. "Parameter" got renamed to "Feature".

The implementation is super simple right now. The idea is later we'd add more logging of the messages, hook it up to about:support, and potentially read some data over from the crash reporter.
Attachment #8728308 - Attachment is obsolete: true
Attachment #8728308 - Flags: feedback?(jmuizelaar)
Attachment #8730493 - Flags: feedback?(jmuizelaar)
Attachment #8730491 - Flags: review?(jmuizelaar) → review+
Attachment #8730492 - Flags: review?(jmuizelaar) → review+
Comment on attachment 8730493 [details] [diff] [review]
part 3, gfxConfig implementation

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

I like it. One thing to think about is features that have dependencies. i.e. WebGL, D2D and video all require working shared surfaces. Video requires working alpha shared surfaces. Currently these kinds of dependencies are expressed completely adhocly. It would be nice if there was some more structure around this. That being said, it's probably something that we can build in a second stage of this work.

It would be nice to see a 'generate a random config' environment variable or something as part of this work to help validate the design.
Attachment #8730493 - Flags: feedback?(jmuizelaar) → feedback+
(In reply to Jeff Muizelaar [:jrmuizel] from comment #10)
> Comment on attachment 8730493 [details] [diff] [review]
> part 3, gfxConfig implementation
> 
> Review of attachment 8730493 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I like it. One thing to think about is features that have dependencies. i.e.
> WebGL, D2D and video all require working shared surfaces. Video requires
> working alpha shared surfaces. Currently these kinds of dependencies are
> expressed completely adhocly. It would be nice if there was some more
> structure around this. That being said, it's probably something that we can
> build in a second stage of this work.

I agree, I was thinking about this given that certain features like D2D content acceleration are dependent on D3D11 compositing, which is dependent on accelerated compositing. I'm not yet sure how to express the dependencies or when to query them, but I'll try some stuff out.

> It would be nice to see a 'generate a random config' environment variable or
> something as part of this work to help validate the design.

Maybe we could do this: have a second gfxConfig instance with random data. The initial one would still get populated, but when querying values we'd use the random one instead.

That wouldn't test the actual initialization logic for gfxConfig, we'd need a much more intelligent system for that. But we would be able to take an existing gfxConfig output and replay it in a new session.
Comment on attachment 8730493 [details] [diff] [review]
part 3, gfxConfig implementation

Milan, this r? is something in between "feedback" and "should we proceed to write patches on top of this". I don't think I would want to land this without at least moving compositor and maybe media acceleration prefs over, addressing Jeff's ideas, and having about:support integration. But all of that can be done in incremental patches and I'm hoping this baseline will not change significantly.
Attachment #8730493 - Flags: review?(milan)
Comment on attachment 8730493 [details] [diff] [review]
part 3, gfxConfig implementation

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

Most of the comments are about naming and explaining.

I do have one general comment - we're potentially changing the timing of things by setting the feature status and querying it elsewhere, perhaps getting into situations where we query too soon?  Can we have asserts/crashes/gfxDevCrash/something that tells us "you are asking too soon, we haven't had a chance to set the correct values yet"?  In general, for all the features.  Or is that already in place with Undefined and we just want to make noise about it?

::: gfx/config/gfxConfig.h
@@ +18,5 @@
> +public:
> +  // Query whether a parameter is enabled, taking into account any user or
> +  // runtime overrides. The algorithm works as follow:
> +  //
> +  //  1. If a runtime decision disabled the parameter, return false.

The runtime decision are things were we attempt to do something, fail, and fallback, right?  Or does it also include blocklisting?

@@ +21,5 @@
> +  //
> +  //  1. If a runtime decision disabled the parameter, return false.
> +  //  2. If a user decision enabled or disabled the parameter, return true or
> +  //     false accordingly.
> +  //  3. Return whether or not the base value is enabled or disabled.

"Base value", as in "IsDisabledByDefault" below?

@@ +28,5 @@
> +  // Query the history of a parameter. ForcedOnByUser returns whether or not
> +  // the user specifically used a "force" preference to enable the parameter.
> +  // IsDisabledByDefault returns whether or not the initial status of the
> +  // feature, before adding user prefs and runtime decisions, was disabled.
> +  static bool IsForcedOnByUser(Feature aFeature);

What about "forced off by user"?

@@ +45,5 @@
> +  static void Disable(Feature aFeature,
> +                      FeatureStatus aStatus,
> +                      const char* aMessage);
> +
> +  // Convenience helper for Disable().

So, we can't use Update to change from disabled to enabled?  Without looking at how we use this, I'd say we at least want to rename the method.

::: gfx/src/gfxTelemetry.h
@@ +40,5 @@
>    // This feature is available for use.
> +  Available,
> +
> +  // This feature was explicitly force-enabled by the user.
> +  ForceEnabled

Do we need to worry about ForceDisabled?

::: gfx/thebes/gfxWindowsPlatform.cpp
@@ +2308,5 @@
>    if (IsFeatureStatusFailure(mAcceleration)) {
>      return;
>    }
>  
> +  MOZ_ASSERT(!InSafeMode());

Maybe stronger than assert?
Attachment #8730493 - Flags: review?(milan) → review+
(In reply to Milan Sreckovic [:milan] from comment #13)
> Comment on attachment 8730493 [details] [diff] [review]
> part 3, gfxConfig implementation
> 
> Review of attachment 8730493 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I do have one general comment - we're potentially changing the timing of
> things by setting the feature status and querying it elsewhere, perhaps
> getting into situations where we query too soon?  Can we have
> asserts/crashes/gfxDevCrash/something that tells us "you are asking too
> soon, we haven't had a chance to set the correct values yet"?  In general,
> for all the features.  Or is that already in place with Undefined and we
> just want to make noise about it?

Yeah, that is definitely true - we're now saying that all the baseline values must be set before you try to make a runtime decision. I'll add an assert that the value must not be "Undefined" when querying.

> ::: gfx/config/gfxConfig.h
> @@ +18,5 @@
> > +public:
> > +  // Query whether a parameter is enabled, taking into account any user or
> > +  // runtime overrides. The algorithm works as follow:
> > +  //
> > +  //  1. If a runtime decision disabled the parameter, return false.
> 
> The runtime decision are things were we attempt to do something, fail, and
> fallback, right?  Or does it also include blocklisting?

It's anything where we were unable to determine the status up-front. For example, if texture sharing is broken we have to change the status of D2D. Or if we crash or get a null device from D3D11CreateDevice, we have to change the status for D3D11 compositing. Blocklisting on the other hand we can query once at startup. (I think, I don't know how the downloadable blocklist works.)

> @@ +21,5 @@
> > +  //
> > +  //  1. If a runtime decision disabled the parameter, return false.
> > +  //  2. If a user decision enabled or disabled the parameter, return true or
> > +  //     false accordingly.
> > +  //  3. Return whether or not the base value is enabled or disabled.
> 
> "Base value", as in "IsDisabledByDefault" below?

Yeah. This, by the way, does not consider things like "layers.acceleration.disabled" being set to false as a user override. We consider that a default. But that's very easy to change, we just have to compare the user pref value vs all.js value.

> @@ +28,5 @@
> > +  // Query the history of a parameter. ForcedOnByUser returns whether or not
> > +  // the user specifically used a "force" preference to enable the parameter.
> > +  // IsDisabledByDefault returns whether or not the initial status of the
> > +  // feature, before adding user prefs and runtime decisions, was disabled.
> > +  static bool IsForcedOnByUser(Feature aFeature);
> 
> What about "forced off by user"?

We didn't have any of these yet, AFAICT, but if you know of one I'll make sure to add it.

> @@ +45,5 @@
> > +  static void Disable(Feature aFeature,
> > +                      FeatureStatus aStatus,
> > +                      const char* aMessage);
> > +
> > +  // Convenience helper for Disable().
> 
> So, we can't use Update to change from disabled to enabled?  Without looking
> at how we use this, I'd say we at least want to rename the method.

Right, the idea is 'Disabled' is sticky at runtime level. If D3D11CreateDevice succeeds, okay - if not, we never want to call it again. I'll see if a better name comes to mind.

> ::: gfx/src/gfxTelemetry.h
> @@ +40,5 @@
> >    // This feature is available for use.
> > +  Available,
> > +
> > +  // This feature was explicitly force-enabled by the user.
> > +  ForceEnabled
> 
> Do we need to worry about ForceDisabled?

Not yet, that's implied by the user disabling the feature. So far we don't have any prefs where the user can disable something and then we ignore the request.
Err, I meant "layers.acceleration.disabled" to "true" is not considered a user override yet.
FeatureStatus::Crashed is ambiguous - it means "crashed inside an SEH try block". I would like to have a separate status for "crashed on startup", so renaming this seems like a good idea.
Attachment #8735340 - Flags: review?(milan)
Currently only Windows uses the DeviceInitData IPC message. But it's meant to be cross-platform and it's easier to handle updates in the base gfxPlatform class, so this patch moves it there.
Attachment #8735354 - Flags: review?(milan)
gfxWindowsPlatform has a number of fields where it tracks the effective status of a feature. We can replace each of these with gfxConfig. The first to go is mAcceleration, which we can replace with Feature::HW_COMPOSITING.

We may want to separate this into two features/paths someday. For example, we may want to use a software compositor (due to blacklisting) but potentially create a D3D11 device for DXVA. But for now it's easier to not change this behavior.
Attachment #8735356 - Flags: review?(milan)
Attachment #8735340 - Flags: review?(milan) → review+
Attachment #8735354 - Flags: review?(milan) → review+
Comment on attachment 8735356 [details] [diff] [review]
part 6, remove gfxWindowsPlatform::mAcceleration

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

::: gfx/config/gfxFeature.h
@@ +35,5 @@
>    void SetUser(FeatureStatus aStatus, const char* aMessage);
>    void SetRuntime(FeatureStatus aStatus, const char* aMessage);
>    bool IsForcedOnByUser() const;
>    bool DisabledByDefault() const;
> +  bool IsInitialized() const {

We're not yet calling this or AssertInitialized yet, rigth?

@@ +42,3 @@
>  
>    void AssertInitialized() const {
> +    MOZ_ASSERT(mDefault.IsInitialized());

Let's make it a gfxCriticalError - it will still assert in debug builds, but there will be some noise in the non-debug builds.
Attachment #8735356 - Flags: review?(milan) → review+
Attached patch part 7, remove mD3D11Status (obsolete) — Splinter Review
This patch removes gfxWindowsPlatform::mD3D11Status and uses gfxConfig instead. Most of the patch is expanding all the failure cases for mD3D11Status to include an error message.

Two things to note, one is that I haven't done dependencies yet, so everywhere HW_COMPOSITING is disabled we have to mirror that into D3D11_COMPOSITING. (We don't *have* to, but it makes things easier.) Luckily this is only needed in two places that are right next to each other.

Also, this patch treats WARP as a "fallback" rather than a "feature". If D3D11 fails to acquire a device (which is basically the only reason we ever use WARP), we set its runtime status to disabled. Then if WARP looks okay, we re-enable D3D11 and the WARP fallback at the same time. This is expressed in the API, so you can't re-enable a failed feature without attaching a fallback to it.

I don't know whether or not this makes sense yet, but WARP is probably the hairiest pref in gfxPrefs.
Attachment #8736211 - Attachment is obsolete: true
Attachment #8737348 - Flags: review?(jmuizelaar)
Attached patch part 8, state machine changes (obsolete) — Splinter Review
Unfortunately I ran into a problem trying to convert D2D over: ideally, we would automatically infer the default and user states from a single pref, based on its default value and user setting. But if we do that, how does blacklisting fit in? It's not a default state and it's not a runtime state.

This patch introduces another state, called "environment", which is anything we determine *after* the default/user states but *before* we actually go to use the feature. The state machine now becomes:
 1. Set default value.
 2. Apply user prefs.
 3. Apply environment factors (like blocklist).
 4. Apply user force-enabled overrides.
 5. Attempt feature; set runtime state if necessary.

Hopefully this is not too complicated. I renamed a few functions to try and make it clearer, if you have any suggestions to help please let me know. Disable() now means "disable due to environment decision" and previous calls become "SetFailed" (or "ForceDisabled").

I'm also trying to find the best style for the really long function call sequences we're getting, so suggestions there are welcome too. Maybe in this bug (or right after) we should emulate GfxScopedFeature:

  FeatureState& feature = gfxConfig::GetFeature(Feature::D3D11_COMPOSITING);
  feature.EnableByDefault();
  feature.SetUser(...
  ...

Which would be much more readable.
Attachment #8737467 - Flags: review?(milan)
Actually, a better API sounds like a good idea so let me back up a few patches and do that first.

Milan, these are the state machine changes described in comment #23.
Attachment #8737348 - Attachment is obsolete: true
Attachment #8737467 - Attachment is obsolete: true
Attachment #8737348 - Flags: review?(jmuizelaar)
Attachment #8737467 - Flags: review?(milan)
Attachment #8737566 - Flags: review?(milan)
This patch makes gfxConfig interaction way cleaner. Instead of something like:

> gfxConfig::EnableByDefault(Feature::WHATEVER);
> gfxConfig::Disable(Feature::Whatever, FeatureStatus::Blacklisted, "Error");
> gfxConfig::UserForceEnable(Feature::WHATEVER, "Pref");

Now you can do:

> FeatureState& feature = gfxConfig::GetFeature(Feature::WHATEVER);
>
> feature.EnableByDefault();
> feature.Disable(FeatureStatus::Blacklisted, "Error");
> feature.UserForceEnable("Pref");
Attachment #8737567 - Flags: review?(milan)
... And now the D3D11 patch should be easier to follow.
Attachment #8737568 - Flags: review?(jmuizelaar)
Attached patch part 10, remove mD2D1Status (obsolete) — Splinter Review
Now we can get a pretty straightforward replacement for Direct2D config as well. This is what all prefs should hopefully look like in the future: start with an enabled/disabled pref, add a force-enabled pref, and then a few environment/runtime checks.
Attachment #8737655 - Flags: review?(milan)
(attached wrong patch)
Attachment #8737655 - Attachment is obsolete: true
Attachment #8737655 - Flags: review?(milan)
Attachment #8737656 - Flags: review?(milan)
Comment on attachment 8737568 [details] [diff] [review]
part 9, remove mD3D11Status

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

Looks good modulo the thread safety concerns. I'm excited by this infrastructure.

::: gfx/layers/d3d11/CompositorD3D11.cpp
@@ +198,5 @@
>  CompositorD3D11::Initialize()
>  {
>    ScopedGfxFeatureReporter reporter("D3D11 Layers");
>  
> +  MOZ_ASSERT(gfxConfig::IsEnabled(Feature::D3D11_COMPOSITING));

gfxConfig meant to be thread safe?
Attachment #8737568 - Flags: review?(jmuizelaar) → review+
Blocks: 1262008
mFallbackBits and Instance are not constructed. Is there a reason for this?
(In reply to Benoit Girard (:BenWa) from comment #30)
> mFallbackBits and Instance are not constructed. Is there a reason for this?

Since it's a static singleton I wanted to avoid extra stuff on startup. But there's not really a lot going on in this class so we can change that if needed.
Attachment #8737566 - Flags: review?(milan) → review+
Attachment #8737567 - Flags: review?(milan) → review+
Comment on attachment 8737656 [details] [diff] [review]
part 10, remove mD2D1Status

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

Starting to be difficult to review based on the patches :) where, as there are changes and additions.  Looks reasonable, but it will make sense to break them up differently when it comes time to "land".
Attachment #8737656 - Flags: review?(milan) → review+
Okay, I'll wrap this bug up then. This second-to-last patch just hooks up gfxConfig to nsIGfxInfo so we can access the decision history (and feature statuses) from about:support.
Attachment #8740321 - Flags: review?(jmuizelaar)
This patch is assuming bug 1263849 is not objectionable. This is a super rough cut at displaying decision history in about:support. It's not localized, and bug #s are not linked yet. Screenshot will get attached next.
Attachment #8740323 - Flags: review?(milan)
Attachment #8740323 - Flags: review?(milan) → review+
Comment on attachment 8740321 [details] [diff] [review]
part 11, nsIGfxInfo helper

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

::: widget/GfxInfoBase.cpp
@@ +1271,5 @@
>    return NS_OK;
>  }
>  
> +nsresult GfxInfoBase::GetFeatureLog(JSContext* aCx, JS::MutableHandle<JS::Value> aOut)
> +{

I wrote some helper classes in GfxInfoHelper.h/cpp that make interacting with JSAPI more pleasant. It might be worth trying to have some shared set of helpers that this code and that code can use.

@@ +1378,5 @@
> +    uint32_t index;
> +    if (!JS_GetArrayLength(aCx, log, &index)) {
> +      return;
> +    }
> +    JS_SetElement(aCx, log, index, obj);

It's probably worth having a JS_ArrayAppendElement helper that does this dance so we don't have to duplicate it. I don't know how difficult it is to add new JSAPI but just having the helper in GfxInfoBase would still be an improvement.
Attachment #8740321 - Flags: review?(jmuizelaar) → review+
Follow-up patch: copy text button was broken when a <td> contained a <table> - the text would be dumped twice.
Attachment #8741641 - Flags: review?(milan)
Android calls UsesOffMainThreadCompositing, but gfxPlatform hasn't been initialized yet. On Desktop it gets initialized way earlier somewhere in XUL, so this should be safe, and looks okay on try.
Attachment #8741642 - Flags: review?(snorp)
Better fix, handles the single-cell rows in the decision log.
Attachment #8741641 - Attachment is obsolete: true
Attachment #8741641 - Flags: review?(milan)
Attachment #8741643 - Flags: review?(milan)
Attachment #8741642 - Flags: review?(snorp) → review+
Attachment #8741643 - Flags: review?(milan) → review+
The second patch in this series is failing mochitest-gl on Windows 8, because we get the D3D9 ANGLE backend instead of D3D11. DoesRenderTargetViewNeedsRecreating is returning true", which causes ANGLE D3D11 to get disabled. [1]

This code is broken in two ways. One, it's not run on content processes nor is its value propagated to them. The child process will use ANGLE D3D11 when it's not supposed to. Either way, this code runs in the gfxWindowsPlatform constructor - *before* InitLayersAccelerationPrefs. That means gANGLESupportsD3D11 gets overwritten with "true" if the blocklist passes. [2]

This patch in this bug changed the ordering so that layers acceleration prefs are initialized before devices. Now we're intentionally falling back to D3D9 because things happen in the correct order, and that RenderTargetView check fails on our Windows 8 machines.

Jeff - what should we do here? If we just land this, we won't have D3D11 ANGLE testing on try.

[1] https://dxr.mozilla.org/mozilla-central/source/gfx/thebes/gfxWindowsPlatform.cpp#2043
[2] https://dxr.mozilla.org/mozilla-central/source/gfx/thebes/gfxPlatform.cpp#2103
Flags: needinfo?(jgilbert)
It is a surprise to me that DoesRenderTargetViewNeedsRecreating is returning true. I've only seen that happen on older optimus machines and it seemed to be triggering correctly. Which part of DoesRenderTargetViewNeedsRecreating is failing?
(In reply to Jeff Muizelaar [:jrmuizel] from comment #41)
> It is a surprise to me that DoesRenderTargetViewNeedsRecreating is returning
> true. I've only seen that happen on older optimus machines and it seemed to
> be triggering correctly. Which part of DoesRenderTargetViewNeedsRecreating
> is failing?

Doing another try run to find out, but I bet it's the FEATURE_LEVEL check.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=49a015a11215
Same as before, but fixes us checking the wrong return value from DoesRenderTargetViewNeedsRecreating.
Attachment #8730492 - Attachment is obsolete: true
Flags: needinfo?(jgilbert)
Attachment #8746171 - Flags: review?(jmuizelaar)
Attachment #8746171 - Flags: review?(jmuizelaar) → review+
Backed out for test failure in browser_Troubleshoot.js.

Backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/df82a3088812
Push showing the failure (your own push had been affected by a bustage pushed earlier): https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=28f944410c7fdf27f7ddf16fc01be4eb0a6eade8
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=26782531&repo=mozilla-inbound
08:15:42     INFO -  482 INFO TEST-START | toolkit/modules/tests/browser/browser_Troubleshoot.js
08:15:42     INFO -  Chrome file doesn't exist: /home/worker/workspace/build/tests/mochitest/browser/toolkit/modules/tests/browser/head.js
08:15:42     INFO -  [h264 @ 0x7fbefc592000] err{or,}_recognition separate: 1; 1
08:15:42     INFO -  [h264 @ 0x7fbefc592000] err{or,}_recognition combined: 1; 1
08:15:42     INFO -  [h264 @ 0x7fbefc592000] Unsupported bit depth: 0
08:15:42     INFO -  ATTENTION: default value of option force_s3tc_enable overridden by environment.
08:15:43     INFO -  [2805] WARNING: robustness marked as unsupported: file /home/worker/workspace/build/src/gfx/gl/GLContextFeatures.cpp, line 896
08:15:43     INFO -  [2805] WARNING: robustness marked as unsupported: file /home/worker/workspace/build/src/gfx/gl/GLContextFeatures.cpp, line 896
08:15:43     INFO -  JavaScript warning: resource://gre/modules/Troubleshoot.jsm, line 435: Error: WebGL: Error during native OpenGL init.
08:15:43     INFO -  [2805] WARNING: robustness marked as unsupported: file /home/worker/workspace/build/src/gfx/gl/GLContextFeatures.cpp, line 896
08:15:43     INFO -  TEST-INFO | started process screentopng
08:15:45     INFO -  TEST-INFO | screentopng: exit 0
08:15:45     INFO -  483 INFO checking window state
08:15:45     INFO -  484 INFO Console message: [JavaScript Warning: "Error: WebGL: Error during native OpenGL init." {file: "resource://gre/modules/Troubleshoot.jsm" line: 435}]
08:15:45     INFO -  485 INFO TEST-UNEXPECTED-FAIL | toolkit/modules/tests/browser/browser_Troubleshoot.js | {} -
08:15:45     INFO -  Stack trace:
08:15:45     INFO -  chrome://mochitests/content/browser/toolkit/modules/tests/browser/browser_Troubleshoot.js:snapshotSchema/<:39
Flags: needinfo?(dvander)
Some random testing with this may have gotten me in a state where I have the basic compositor and Direct2D (when I force CreateBlendState() to fail in CompositorD3D11::Initialize.  Not 100% convinced this was caused by this patch, but it's worth investigating.
(In reply to Milan Sreckovic [:milan] from comment #46)
> Some random testing with this may have gotten me in a state where I have the
> basic compositor and Direct2D (when I force CreateBlendState() to fail in
> CompositorD3D11::Initialize.  Not 100% convinced this was caused by this
> patch, but it's worth investigating.

OK, so, this is a problem, but it isn't due to these patches.  We may still want to fix it, but this bug doesn't make it worse.
Yeah, that should never happen. FWIW you should be able to see the decision log in about:support with these patches applied, which could help see why.

bug 1267253 did change D2D decision code recently.
Flags: needinfo?(dvander)
Depends on: 1269565
You need to log in before you can comment on or make changes to this bug.