Closed Bug 1323316 Opened 3 years ago Closed 3 years ago

Use ANGLE for WebRender on Windows

Categories

(Core :: Graphics: WebRender, defect)

Unspecified
Windows
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: sotaro, Assigned: sotaro)

References

Details

Attachments

(2 files, 12 obsolete files)

24.49 KB, patch
Details | Diff | Splinter Review
12.47 KB, patch
jrmuizel
: review+
Details | Diff | Splinter Review
Use ANGLE for WebRender on windows.
OS: Unspecified → Windows
Blocks: webrender
Attached patch patch - Use ANGLE for WebRender (obsolete) — Splinter Review
Dirty wip patch. Addressed build and abort problem, but rendering does not work well.
(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.
See Also: → 1323612
(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.
Depends on: 1324376
Attachment #8818363 - Attachment is obsolete: true
Depends on: 1324648
Attached patch wip patch (obsolete) — Splinter Review
Attachment #8819807 - Attachment is obsolete: true
Attached patch wip patch (obsolete) — Splinter Review
Addressed rendering problem.
Attachment #8820584 - Attachment is obsolete: true
Attached patch wip patch (obsolete) — Splinter Review
Attachment #8820703 - Attachment is obsolete: true
Depends on: 1325911
Attached patch wip patch (obsolete) — Splinter Review
Rebased.
Attachment #8821017 - Attachment is obsolete: true
Attached patch wip patch (obsolete) — Splinter Review
Attachment #8827760 - Attachment is obsolete: true
Attached patch wip patch (obsolete) — Splinter Review
Attachment #8831304 - Attachment is obsolete: true
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
(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?
Depends on: 1347442
Depends on: 1347443
Depends on: 1348209
Attached patch wip patch (obsolete) — Splinter Review
Attachment #8831308 - Attachment is obsolete: true
Attached patch wip patch (obsolete) — Splinter Review
Attachment #8848440 - Attachment is obsolete: true
Depends on: 1349465
Assignee: nobody → sotaro.ikeda.g
Attachment #8848441 - Attachment is obsolete: true
Summary: Use ANGLE for WebRender → Add support of using ANGLE for WebRender
Summary: Add support of using ANGLE for WebRender → Using ANGLE for WebRender on Windows
Summary: Using ANGLE for WebRender on Windows → Use ANGLE for WebRender on Windows
Attachment #8850328 - Attachment is obsolete: true
Attachment #8850507 - Flags: review?(bugmail)
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.
Attachment #8850507 - Flags: review?(jmuizelaar)
Attachment #8850507 - Flags: review?(bugmail)
Attachment #8850507 - Flags: review+
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
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
(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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 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
Attachment #8850507 - Flags: review?(jmuizelaar) → review-
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.
See Also: → 516989
(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
: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?
Flags: needinfo?(jmuizelaar)
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.
I meant bug 760323. Sorry.
Flags: needinfo?(jmuizelaar)
Attachment #8850507 - Attachment is obsolete: true
Comment on attachment 8852317 [details] [diff] [review]
patch - Use ANGLE for WebRender on Windows

:jrmuizel, can you review again?
Attachment #8852317 - Flags: review?(jmuizelaar)
Attachment #8852317 - Flags: review?(jmuizelaar) → review+
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.
Don't know what happened to pulsebot.

https://hg.mozilla.org/projects/graphics/rev/69a8dd0935e25869d9f33440d92f317535843e09
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Thanks!
Depends on: 1366700
You need to log in before you can comment on or make changes to this bug.