1323316 - Use ANGLE for WebRender on Windows

Status: RESOLVED FIXED

(Core :: Graphics: WebRender, defect)

Description

[Sotaro Ikeda [:sotaro]]

Use ANGLE for WebRender on windows.

Comment 1

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Use ANGLE for WebRender

Dirty wip patch. Addressed build and abort problem, but rendering does not work well.

Comment 2

[Sotaro Ikeda [:sotaro]]

(In reply to Sotaro Ikeda [:sotaro] from comment #1)
> Dirty wip patch. Addressed build and abort problem, but rendering does not
> work well.

Another problem is that gleam and webrender do not support dynamic switch between OpenGL and OpenGLES.

Comment 3

[Dzmitry Malyshau [:kvark]]

https://github.com/servo/gleam/issues/99

Comment 4

[Sotaro Ikeda [:sotaro]]

(In reply to Sotaro Ikeda [:sotaro] from comment #1)
> Created attachment 8818363 [details] [diff] [review]
> patch - Use ANGLE for WebRender
> 
> Dirty wip patch. Addressed build and abort problem, but rendering does not
> work well.

Same rendering problem happened also on android. See Bug 1323612 Comment 7. Current webrender might have a problem on GLES env.

Comment 5

[Sotaro Ikeda [:sotaro]]

Attachment: wip patch - Use ANGLE for WebRender

Comment 6

[Sotaro Ikeda [:sotaro]]

Attachment: wip patch

Comment 7

[Sotaro Ikeda [:sotaro]]

Attachment: wip patch

Addressed rendering problem.

Comment 8

[Sotaro Ikeda [:sotaro]]

Attachment: wip patch

Comment 9

[Sotaro Ikeda [:sotaro]]

Attachment: wip patch

Rebased.

Comment 10

[Sotaro Ikeda [:sotaro]]

Attachment: wip patch

Comment 11

[Sotaro Ikeda [:sotaro]]

Attachment: wip patch

Comment 12

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Attachment: Using ANGLE for webrender.

Quantum render is only enabled when using gpu process now. And the render thread turned on as well. So I rebased
the wip patch to make ANGLE works again. As you can see in the patch, there are some problems:

1. We don't have XPCOM service in GPU process, which means the directory service is not available. We need another way to load dll of EGL. This patch has been addressed this issue.
2. gfxPlatform is not available in GPU process, so we cannot get ScreenDepth which would be used by CreateConfig in EGL.
3. ThreadSafeGetFeatureStatus cannot be called in GPU process as well.

MozReview-Commit-ID: 38FsiJWL2h1

Comment 13

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

(In reply to Morris Tseng [:mtseng] [:Morris] from comment #12)
>...
> 2. gfxPlatform is not available in GPU process, so we cannot get ScreenDepth
> which would be used by CreateConfig in EGL.

gfxVars?

Comment 14

[Sotaro Ikeda [:sotaro]]

Attachment: wip patch

Comment 15

[Sotaro Ikeda [:sotaro]]

Attachment: wip patch

Comment 16

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Add support of using ANGLE for WebRender

Comment 17

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Use ANGLE for WebRender on Windows

Comment 18

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8850507 [details] [diff] [review]
patch - Use ANGLE for WebRender on Windows

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

This looks fine to me, but I don't know the code in GLLibraryEGL.cpp at all and would like somebody else to review that. Tagging jeff.

::: gfx/webrender_bindings/WebRenderAPI.cpp
@@ +47,5 @@
>  
> +    RefPtr<gl::GLContext> gl;
> +    if (gfxVars::UseWebRenderANGLE()) {
> +      gl = gl::GLContextProviderEGL::CreateForCompositorWidget(mCompositorWidget, true);
> +      if(!gl || !gl->IsANGLE()) {

nit: space before (

Also it might be nice to have an assertion or log something here if this fails, because otherwise the crash happens relatively far away from this point and it can be difficult to trace it back here.

Comment 19

[Kartikaya Gupta (email:[e-mail])]

FYI, I did a try push of all the windows tests with this patch and webrender force-enabled, just to see what would happen. It's looking pretty good so far: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b5c6b7eb72b8ded49b102af7240bfa295bf69598

Comment 20

[Kartikaya Gupta (email:[e-mail])]

Oh. I guess most of those tests are running with the GPU process disabled, which means webrender is going to be disabled as well :( The Rg jobs are reftests with GPU process, and they're crashing due to this:

https://treeherder.mozilla.org/logviewer.html#?job_id=86272653&repo=try&lineNumber=2606

Comment 21

[Kartikaya Gupta (email:[e-mail])]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #18)
> Also it might be nice to have an assertion or log something here if this
> fails, because otherwise the crash happens relatively far away from this
> point and it can be difficult to trace it back here.

Actually don't worry about this too much, I'll be adding logs in other places as well to catch WR startup failures, so I can do this one too.

Comment 22

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 8850507 [details] [diff] [review]
patch - Use ANGLE for WebRender on Windows

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

::: gfx/gl/GLLibraryEGL.cpp
@@ +103,5 @@
>  {
> +    PRLibSpec lspec;
> +    lspec.type = PR_LibSpec_Pathname;
> +    lspec.value.pathname = ToNewUTF8String(filename);
> +    return PR_LoadLibraryWithFlags(lspec, PR_LD_LAZY | PR_LD_LOCAL);

I'm concerned that this won't always work in cases where the old code did. See bug 516989

Comment 23

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 8850507 [details] [diff] [review]
patch - Use ANGLE for WebRender on Windows

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

Other than that the patch looks fine.

Comment 24

[Sotaro Ikeda [:sotaro]]

(In reply to Jeff Muizelaar [:jrmuizel] from comment #22)
> Comment on attachment 8850507 [details] [diff] [review]
> patch - Use ANGLE for WebRender on Windows
> 
> Review of attachment 8850507 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/gl/GLLibraryEGL.cpp
> @@ +103,5 @@
> >  {
> > +    PRLibSpec lspec;
> > +    lspec.type = PR_LibSpec_Pathname;
> > +    lspec.value.pathname = ToNewUTF8String(filename);
> > +    return PR_LoadLibraryWithFlags(lspec, PR_LD_LAZY | PR_LD_LOCAL);
> 
> I'm concerned that this won't always work in cases where the old code did.
> See bug 516989

Thanks for the info, but I am not sure why the patch became review-, since nsLocalFile::Load() uses PR_LoadLibraryWithFlags() on windows. If bug 516989 is a problem, it seems better to be addressed as a different problem than this bug.
 > https://dxr.mozilla.org/mozilla-central/source/xpcom/io/nsLocalFileWin.cpp#2369

Comment 25

[Sotaro Ikeda [:sotaro]]

:jrmuizel, with comment 24, I am not sure why PR_LoadLibraryWithFlags() usage caused review-. Can you explain more? Do you already have an idea how to address your comment other than addressing bug 516989?

Comment 26

[Nicolas Silva [:nical]]

Jeff, can you confirm that the bug number you linked is the intended one? It could be lack of context or caffeine, but I fail to see the relation between young-Bas's attempt at implementing condvars on Windows and library loading.

Comment 27

[Jeff Muizelaar [:jrmuizel]]

I meant bug 760323. Sorry.

Comment 28

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Use ANGLE for WebRender on Windows

Comment 29

[Sotaro Ikeda [:sotaro]]

Comment on attachment 8852317 [details] [diff] [review]
patch - Use ANGLE for WebRender on Windows

:jrmuizel, can you review again?

Comment 30

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8852317 [details] [diff] [review]
patch - Use ANGLE for WebRender on Windows

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

::: modules/libpref/init/all.js
@@ +823,5 @@
>  #ifdef MOZ_ENABLE_WEBRENDER
>  pref("gfx.webrender.enabled", true);
> +#ifdef XP_WIN
> +pref("gfx.webrender.force-angle", true);
> +#endif

I think this should be outside the MOZ_ENABLE_WEBRENDER, so that it's always true. Otherwise on a Nightly build people have to flip gfx.webrender.enabled as well as gfx.webrender.force-angle to get this turned on. If it's outside the ifdef then they only need to flip gfx.webrender.enabled. And looking at the code in InitWebRenderConfig it shouldn't make a difference.

I'll make this change and land the patch, let me know if you have a problem with this.

Comment 31

[Kartikaya Gupta (email:[e-mail])]

Don't know what happened to pulsebot.

https://hg.mozilla.org/projects/graphics/rev/69a8dd0935e25869d9f33440d92f317535843e09

Comment 32

[Kartikaya Gupta (email:[e-mail])]

https://hg.mozilla.org/mozilla-central/rev/69a8dd0935e2

Comment 33

[Sotaro Ikeda [:sotaro]]

Thanks!