1254899 - Clean up acceleration decision-making

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

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

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.

Comment 1

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

Attachment: prototype

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.

Comment 2

[Milan Sreckovic [:milan] (needinfo for best results)]

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?

Comment 3

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

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

Comment 4

[Milan Sreckovic [:milan] (needinfo for best results)]

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

[Jeff Muizelaar [:jrmuizel]]

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?

Comment 6

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

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

Comment 7

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

Attachment: part 1, fix indentation

This just fixes a bunch of 4-space indents in gfxWindowsPlatform.

Comment 8

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

Attachment: part 2, move InitLayersAccelerationPrefs

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.

Comment 9

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

Attachment: part 3, gfxConfig implementation

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.

Comment 10

[Jeff Muizelaar [:jrmuizel]]

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.

Comment 11

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

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

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

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.

Comment 13

[Milan Sreckovic [:milan] (needinfo for best results)]

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?

Comment 14

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

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

Comment 15

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

Err, I meant "layers.acceleration.disabled" to "true" is not considered a user override yet.

Comment 16

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

Attachment: part 4, rename FeatureStatus::Crashed

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.

Comment 17

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

Attachment: part 5, move DeviceInitData handling

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.

Comment 18

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

Attachment: part 6, remove gfxWindowsPlatform::mAcceleration

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.

Comment 19

[Milan Sreckovic [:milan] (needinfo for best results)]

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.

Comment 20

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

Attachment: part 7, WIP - remove mD3D11Status

Comment 21

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

Attachment: part 7, WIP - remove mD3D11Status

Comment 22

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

Attachment: part 7, remove mD3D11Status

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.

Comment 23

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

Attachment: part 8, state machine changes

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.

Comment 24

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

Attachment: part 7, state machine changes

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.

Comment 25

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

Attachment: part 8, cleaner API

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

Comment 26

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

Attachment: part 9, remove mD3D11Status

... And now the D3D11 patch should be easier to follow.

Comment 27

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

Attachment: part 10, remove mD2D1Status

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.

Comment 28

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

Attachment: part 10, remove mD2D1Status

(attached wrong patch)

Comment 29

[Jeff Muizelaar [:jrmuizel]]

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?

Comment 30

[Benoit Girard (:BenWa)]

mFallbackBits and Instance are not constructed. Is there a reason for this?

Comment 31

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

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

Comment 32

[Milan Sreckovic [:milan] (needinfo for best results)]

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

Comment 33

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

Attachment: part 11, nsIGfxInfo helper

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.

Comment 34

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

Attachment: part 12, about:support integration

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.

Comment 35

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

Attachment: about:support screenshot

Comment 36

[Jeff Muizelaar [:jrmuizel]]

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.

Comment 37

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

Attachment: part 13, fix about:support copy text

Follow-up patch: copy text button was broken when a <td> contained a <table> - the text would be dumped twice.

Comment 38

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

Attachment: part 3.1, fix android

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.

Comment 39

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

Attachment: part 13, fix about:support copy text

Better fix, handles the single-cell rows in the decision log.

Comment 40

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

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

Comment 41

[Jeff Muizelaar [:jrmuizel]]

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?

Comment 42

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

(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

Comment 43

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

Attachment: part 2, move InitLayersAccelerationPrefs

Same as before, but fixes us checking the wrong return value from DoesRenderTargetViewNeedsRecreating.

Comment 44

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b805df0c8296
https://hg.mozilla.org/integration/mozilla-inbound/rev/f2056259d227
https://hg.mozilla.org/integration/mozilla-inbound/rev/2de214d5d1e0
https://hg.mozilla.org/integration/mozilla-inbound/rev/64e58f9625eb
https://hg.mozilla.org/integration/mozilla-inbound/rev/21c8a7ea2b93
https://hg.mozilla.org/integration/mozilla-inbound/rev/d17f98a9918a
https://hg.mozilla.org/integration/mozilla-inbound/rev/f8f3ae8c5cef
https://hg.mozilla.org/integration/mozilla-inbound/rev/e9c311c68e4a
https://hg.mozilla.org/integration/mozilla-inbound/rev/e8abac801a51
https://hg.mozilla.org/integration/mozilla-inbound/rev/993906e53329
https://hg.mozilla.org/integration/mozilla-inbound/rev/b5ab83af342c
https://hg.mozilla.org/integration/mozilla-inbound/rev/1d7eeee5a5c5
https://hg.mozilla.org/integration/mozilla-inbound/rev/f749ee384012

Comment 45

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

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

Comment 46

[Milan Sreckovic [:milan] (needinfo for best results)]

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.

Comment 47

[Milan Sreckovic [:milan] (needinfo for best results)]

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

Comment 48

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

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.

Comment 49

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/501c8f9c5d06
https://hg.mozilla.org/integration/mozilla-inbound/rev/1e9582763b1d
https://hg.mozilla.org/integration/mozilla-inbound/rev/4dbe9e240867
https://hg.mozilla.org/integration/mozilla-inbound/rev/84587ebfdadf
https://hg.mozilla.org/integration/mozilla-inbound/rev/c8ef3ba0af86
https://hg.mozilla.org/integration/mozilla-inbound/rev/acceb80441c5
https://hg.mozilla.org/integration/mozilla-inbound/rev/1a69236ad2b0
https://hg.mozilla.org/integration/mozilla-inbound/rev/17cc05353b2d
https://hg.mozilla.org/integration/mozilla-inbound/rev/bc29b45d95c2
https://hg.mozilla.org/integration/mozilla-inbound/rev/deeca272bf66
https://hg.mozilla.org/integration/mozilla-inbound/rev/f4b8910028b3
https://hg.mozilla.org/integration/mozilla-inbound/rev/c1ca5d9a644c
https://hg.mozilla.org/integration/mozilla-inbound/rev/32295f6fbc6a

Comment 50

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/501c8f9c5d06
https://hg.mozilla.org/mozilla-central/rev/1e9582763b1d
https://hg.mozilla.org/mozilla-central/rev/4dbe9e240867
https://hg.mozilla.org/mozilla-central/rev/84587ebfdadf
https://hg.mozilla.org/mozilla-central/rev/c8ef3ba0af86
https://hg.mozilla.org/mozilla-central/rev/acceb80441c5
https://hg.mozilla.org/mozilla-central/rev/17cc05353b2d
https://hg.mozilla.org/mozilla-central/rev/bc29b45d95c2
https://hg.mozilla.org/mozilla-central/rev/deeca272bf66
https://hg.mozilla.org/mozilla-central/rev/f4b8910028b3
https://hg.mozilla.org/mozilla-central/rev/c1ca5d9a644c
https://hg.mozilla.org/mozilla-central/rev/32295f6fbc6a