1274012 - use gfxConfig for WebGL prefs

Status: RESOLVED INCOMPLETE

(Core :: Graphics: CanvasWebGL, task, P5)

Comment 1

[Benoit Girard (:BenWa)]

Attachment: MozReview Request: Bug 1274012 - use gfxConfig for WebGL prefs. r=dvander

Review commit: https://reviewboard.mozilla.org/r/53712/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/53712/

Comment 2

[Benoit Girard (:BenWa)]

Comment on attachment 8754086 [details]
MozReview Request: Bug 1274012 - use gfxConfig for WebGL prefs. r=dvander

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53712/diff/1-2/

Comment 3

[Benoit Girard (:BenWa)]

Comment on attachment 8754086 [details]
MozReview Request: Bug 1274012 - use gfxConfig for WebGL prefs. r=dvander

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53712/diff/2-3/

Comment 4

[Benoit Girard (:BenWa)]

This is waiting on the API changes from bug 1274046 bug which I'll post once I get it building on windows. But it should be reviewable now since the API change is trivial.

Comment 5

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

Comment on attachment 8754086 [details]
MozReview Request: Bug 1274012 - use gfxConfig for WebGL prefs. r=dvander

https://reviewboard.mozilla.org/r/53712/#review51698

::: dom/canvas/WebGLContext.cpp:584
(Diff revision 3)
>  static already_AddRefed<gl::GLContext>
>  CreateGLWithEGL(const gl::SurfaceCaps& caps, gl::CreateContextFlags flags,
>                  WebGLContext* webgl, nsACString* const out_failReason,
>                  nsACString* const out_failureId)
>  {
> +    if (!gfxConfig::IsEnabled(Feature::WEBGL_OPENGL)) {

No out_failureId in this case?

::: gfx/thebes/gfxPlatform.cpp:2492
(Diff revision 3)
> +    if (InSafeMode()) {
> +      nsCString safeModeFailure = NS_LITERAL_CSTRING("FEATURE_FAILURE_WEBGL_SAFEMODE");
> +      webglOpenGLFeature.ForceDisable(FeatureStatus::Blocked, "Acceleration blocked by safe-mode",
> +                                      safeModeFailure);
> +      webglAngleFeature.ForceDisable(FeatureStatus::Blocked, "Acceleration blocked by safe-mode",
> +                                     safeModeFailure);

Return here if possible, or move the InSafeMode check to the end - ForceDisable sets the very last state so anything that comes after is effectively ignored. (It might technically show up in the about:support decision log, I guess.)

Comment 6

[Benoit Girard (:BenWa)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=02d7215cbfb8

Comment 7

[Benoit Girard (:BenWa)]

Comment on attachment 8754086 [details]
MozReview Request: Bug 1274012 - use gfxConfig for WebGL prefs. r=dvander

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53712/diff/3-4/

Comment 8

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

Ryan, can you check with David if we still want this, and see if we can land it if we do?

Comment 9

[Ryan Hunt [:rhunt]]

Attachment: remove-disable-wgl.patch

Currently this pref does nothing. I don't know if that is a mistake or if it should be updated to follow original intent.

For this bug, it's nice to just remove it. It can be added back later.

Comment 10

[Ryan Hunt [:rhunt]]

Attachment: use-gfxconfig.patch

This patch is similar to Benoit's patches, but updated to match changes to WebGL initialization.

This patch tries to keep the same observable behavior of error messages and warnings. All of the feature detections (env variables, prefs, blacklist, safemode) that were reported as failures, are moved into gfxPlatform/gfxConfig. The rest of the prefs that are about choosing which backend to try are left as is.

Comment 11

[Kelsey Gilbert [:jgilbert]]

Please hold off on optional fixes until 54, while we get WebGL 2 out the door.

Comment 12

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

Yes, let's land this in late January.

Comment 13

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8819951 [details] [diff] [review]
use-gfxconfig.patch

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

The rest of this looks ok.

I'm not clear on why we want to change this. Please explain why it's useful to transition to this new API.

::: dom/canvas/WebGLContext.cpp
@@ +748,2 @@
>  #ifdef XP_WIN
> +    const bool tryNativeGL = gfxPrefs::WebGLDisableANGLE() || PR_GetEnv("MOZ_WEBGL_FORCE_OPENGL") || useEGL;

Do not simplify this logic.

For readability, it's important it stays roughly how it was.

Comment 14

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8819944 [details] [diff] [review]
remove-disable-wgl.patch

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

If you're removing this, you *must* remove the all.js entry.

Comment 15

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

(In reply to Jeff Gilbert [:jgilbert] from comment #13)
> Comment on attachment 8819951 [details] [diff] [review]
> use-gfxconfig.patch
> 
> Review of attachment 8819951 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> The rest of this looks ok.
> 
> I'm not clear on why we want to change this. Please explain why it's useful
> to transition to this new API.

Bug 1254899.

Comment 16

[Ryan Hunt [:rhunt]]

Attachment: remove-disable-wgl.patch

Rebased and removed the all.js entry.

Comment 17

[Ryan Hunt [:rhunt]]

Attachment: webgl-gfxconfig.patch

Rebased and addressed review comments.

Comment 18

[Ryan Hunt [:rhunt]]

Comment on attachment 8828946 [details] [diff] [review]
webgl-gfxconfig.patch

Clearing review as I'm not working on this right now.

Comment 19

[Kelsey Gilbert [:jgilbert]]

It's all StaticPrefs now.