Closed Bug 1277314 Opened 6 years ago Closed 6 years ago

Add OpenGL Compositing feature

Categories

(Core :: Graphics: Layers, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla50
Tracking Status
firefox50 --- fixed

People

(Reporter: ernest, Assigned: ernest)

References

Details

Attachments

(1 file)

No specific feature to use opengl preference, will check to see hw compositing enabled, then check if opengl preferred and disable d3d
Comment on attachment 8758839 [details]
Bug 1277314 - Add ogl compositing feature in gfxplatform

https://reviewboard.mozilla.org/r/56962/#review54444

Looks good, let's just make sure we check the feature now when we can.

::: gfx/thebes/gfxPlatform.cpp:2525
(Diff revision 1)
> +
> +  openGLFeature.EnableByDefault();
> +
> +  // If the user prefers OpenGL, disable d3d9 and d3d11
> +  // This code assumes OpenGL pref > d3d9 pref, and d3d9 pref > d3d11
> +  if (gfxPrefs::LayersPreferOpenGL()) {

In places where we check this preference now we can check the feature instead:
http://mxr.mozilla.org/mozilla-central/source/gfx/thebes/gfxWindowsPlatform.cpp#2933
http://mxr.mozilla.org/mozilla-central/source/widget/windows/nsWindow.cpp#6806
Attachment #8758839 - Flags: review?(bgirard)
Comment on attachment 8758839 [details]
Bug 1277314 - Add ogl compositing feature in gfxplatform

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56962/diff/1-2/
Attachment #8758839 - Attachment description: MozReview Request: Bug 1277314 - Add ogl compositing feature in gfxplatform → Bug 1277314 - Add ogl compositing feature in gfxplatform
Attachment #8758839 - Flags: review?(bgirard)
Comment on attachment 8758839 [details]
Bug 1277314 - Add ogl compositing feature in gfxplatform

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56962/diff/2-3/
Could you please take a look at the change to AllowOpenGl and gfxPlatform::GetAcceleratedCompositorBackends, we could not find the use case where we would enable open gl layers with hw compositing enabled and open gl compositing disabled, so we changed it to just check if open gl feature is enabled instead.
Flags: needinfo?(dvander)
Attachment #8758839 - Flags: review?(dvander)
Attachment #8758839 - Flags: review?(bgirard) → review+
Comment on attachment 8758839 [details]
Bug 1277314 - Add ogl compositing feature in gfxplatform

https://reviewboard.mozilla.org/r/56962/#review54502

LGTM
Attachment #8758839 - Flags: review?(dvander) → review-
Comment on attachment 8758839 [details]
Bug 1277314 - Add ogl compositing feature in gfxplatform

https://reviewboard.mozilla.org/r/56962/#review55788

Sorry for the late review! Was on PTO and forgot to set Bugzilla status.

This mostly looks fine but I'd like to change how gfxPlatform initializes the pref - I'll r+ ASAP with that fixed.

::: gfx/thebes/gfxPlatform.cpp:2496
(Diff revision 3)
> +
> +  openGLFeature.EnableByDefault();
> +
> +  // If the user prefers OpenGL, disable d3d9 and d3d11
> +  // This code assumes OpenGL pref > d3d9 pref, and d3d9 pref > d3d11
> +  if (gfxPrefs::LayersPreferOpenGL()) {

GetAcceleratedCompositorBackends will automatically prioritize LAYERS_OPENGL over D3D9/D3D11 on Windows, so this will change functionality such that those no longer get used as a fallback if OpenGL fails. (That's a fine thing to do if it was intended, but better to do it with a separate pref.)

Another problem is that d3d9/d3d11 aren't initialized on non-Windows platforms, so these state changes will assert.

I'd just remove the whole block, and do something like:

```
#ifdef XP_WIN
  // Don't enable by default on Windows, since it could show up in about:support even
  // though it'll never get used.
  openGLFeature.SetDefaultFromPref(
    gfxPrefs::GetLayersPreferOpenGLPrefName(),
    false,
    gfxPrefs::GetLayersPreferOpenGLPrefDefault());
#else
  openGLFeature.EnableByDefault()
#endif
```

::: gfx/thebes/gfxPlatform.cpp:2508
(Diff revision 3)
> +  }
> +
> +  nsCString message;
> +  nsCString failureId;
> +  if (!IsGfxInfoStatusOkay(nsIGfxInfo::FEATURE_OPENGL_LAYERS, &message, &failureId)) {
> +      openGLFeature.Disable(FeatureStatus::Blacklisted, message.get(), failureId);

nit: two-space indent here

::: gfx/thebes/gfxWindowsPlatform.cpp:2929
(Diff revision 3)
>  }
>  
>  void
>  gfxWindowsPlatform::GetAcceleratedCompositorBackends(nsTArray<LayersBackend>& aBackends)
>  {
> -  if (gfxPrefs::LayersPreferOpenGL()) {
> +  if (gfxConfig::IsEnabled(Feature::OPENGL_COMPOSITING)) {

Please add "&& gfxPrefs::LayersPreferOpenGL()" here, for consistency with how D3D9 works. It's a little redundant, but continues to work if we change how OpenGL is toggled.
Comment on attachment 8758839 [details]
Bug 1277314 - Add ogl compositing feature in gfxplatform

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56962/diff/3-4/
Attachment #8758839 - Flags: review- → review?(dvander)
Attachment #8758839 - Flags: review?(dvander) → review+
Comment on attachment 8758839 [details]
Bug 1277314 - Add ogl compositing feature in gfxplatform

https://reviewboard.mozilla.org/r/56962/#review56004

Looks good, thanks!

::: gfx/thebes/gfxPlatform.cpp:2441
(Diff revision 4)
> +    openGLFeature.DisableByDefault(FeatureStatus::Unavailable, "Hardware compositing is disabled",
> +                           NS_LITERAL_CSTRING("FEATURE_FAILURE_OPENGL_NEED_HWCOMP"));
> +    return;
> +  }
> +
> +  #ifdef XP_WIN

nit: no indent on #if/#else/#endif directives
Comment on attachment 8758839 [details]
Bug 1277314 - Add ogl compositing feature in gfxplatform

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56962/diff/4-5/
Comment on attachment 8758839 [details]
Bug 1277314 - Add ogl compositing feature in gfxplatform

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56962/diff/5-6/
Comment on attachment 8758839 [details]
Bug 1277314 - Add ogl compositing feature in gfxplatform

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56962/diff/6-7/
fixed issues with windows build, if you have a chance could you try to land this?
Flags: needinfo?(bgirard)
actually, need to clarify something with you before you land.
Flags: needinfo?(bgirard)
last check-in request failed. ni?self to double check that this one goes through.
Flags: needinfo?(bgirard)
Pushed by b56girard@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/5d084a2d51ef
Add ogl compositing feature in gfxplatform r=BenWa,dvander
Flags: needinfo?(bgirard)
https://hg.mozilla.org/mozilla-central/rev/5d084a2d51ef
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
Please take a look at https://bugzilla.mozilla.org/show_bug.cgi?id=1002846#c16.  I posted it under the wrong patch.
Depends on: 1285629
You need to log in before you can comment on or make changes to this bug.