Closed Bug 1262265 Opened 3 years ago Closed 3 years ago

Cleanup GLContext symbol initialization

Categories

(Core :: Graphics, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox48 --- fixed

People

(Reporter: jgilbert, Assigned: jgilbert)

Details

Attachments

(7 files)

It's pretty messy, and could use a bit of a cleanup.

Particularly, it'd be nice to improve readability of code flow regarding which parts can cause context creation to fail.
From 9d86dbd546df9349a0a9f1c089b491a7524e9450 Mon Sep 17 00:00:00 2001
---
 dom/canvas/WebGLContext.cpp     |    2 +-
 gfx/gl/GLContext.cpp            | 1934 ++++++++++++++++++---------------------
 gfx/gl/GLContext.h              |   29 +-
 gfx/gl/GLContextCGL.h           |    2 +-
 gfx/gl/GLContextEAGL.h          |    2 +-
 gfx/gl/GLContextProviderCGL.mm  |    6 -
 gfx/gl/GLContextProviderEAGL.mm |    6 -
 gfx/gl/GLLibraryLoader.cpp      |    6 +-
 gfx/gl/GLLibraryLoader.h        |    4 +-
 9 files changed, 897 insertions(+), 1094 deletions(-)

Review commit: https://reviewboard.mozilla.org/r/44399/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/44399/
Attachment #8738264 - Flags: review?(jmuizelaar)
Comment on attachment 8738264 [details]
MozReview Request: Bug 1262265 - Cleanup InitWithPrefix. - r?jrmuizel

https://reviewboard.mozilla.org/r/44399/#review42077

It's probably worth adding gfxCriticalNote/gfxCriticalWarning's in the places that are not expected to fail but we still attempt to handle failure case.
Attachment #8738264 - Flags: review?(jmuizelaar)
(In reply to Jeff Muizelaar [:jrmuizel] from comment #2)
> Comment on attachment 8738264 [details]
> MozReview Request: Bug 1262265 - Cleanup InitWithPrefix. - r?jrmuizel
> 
> https://reviewboard.mozilla.org/r/44399/#review42077
> 
> It's probably worth adding gfxCriticalNote/gfxCriticalWarning's in the
> places that are not expected to fail but we still attempt to handle failure
> case.

e.g. when we get the renderer string etc.
Comment on attachment 8738264 [details]
MozReview Request: Bug 1262265 - Cleanup InitWithPrefix. - r?jrmuizel

https://reviewboard.mozilla.org/r/44399/#review42117
Attachment #8738264 - Flags: review+
Not just android, as it turns out: https://treeherder.mozilla.org/logviewer.html#?job_id=25562779&repo=mozilla-inbound
Flags: needinfo?(jgilbert)
From 30a8c48ef9afeb8cac5124244a1f4091d0a2238d Mon Sep 17 00:00:00 2001
---
 gfx/gl/GLContextProviderCGL.mm  | 19 +++++++------------
 gfx/gl/GLContextProviderEAGL.mm | 13 ++++++++-----
 gfx/gl/GLContextProviderGLX.cpp | 15 ++++-----------
 gfx/gl/GLContextProviderWGL.cpp | 20 +++-----------------
 4 files changed, 22 insertions(+), 45 deletions(-)

Review commit: https://reviewboard.mozilla.org/r/45915/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/45915/
Attachment #8740690 - Flags: review?(jmuizelaar)
Attachment #8740691 - Flags: review?(jmuizelaar)
Attachment #8740692 - Flags: review?(jmuizelaar)
Attachment #8740693 - Flags: review?(jmuizelaar)
From 6aa28e18fb9ff96e22945d130933ba6062161372 Mon Sep 17 00:00:00 2001
---
 gfx/gl/GLContextProviderEGL.cpp | 12 ++++--------
 1 file changed, 4 insertions(+), 8 deletions(-)

Review commit: https://reviewboard.mozilla.org/r/45917/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/45917/
From 6ea76a49d344a5e122709b4cffa75a5a618e7fe1 Mon Sep 17 00:00:00 2001
---
 gfx/gl/GLContextProviderEGL.cpp | 10 ++--------
 1 file changed, 2 insertions(+), 8 deletions(-)

Review commit: https://reviewboard.mozilla.org/r/45919/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/45919/
From 649a327d8a82cb3b7339223174a787b6957ae2b8 Mon Sep 17 00:00:00 2001
---
 gfx/gl/GLContextProviderCGL.mm  | 3 ++-
 gfx/gl/GLContextProviderEAGL.mm | 3 ++-
 gfx/gl/GLContextProviderGLX.cpp | 3 ++-
 gfx/gl/GLContextProviderWGL.cpp | 5 +++--
 4 files changed, 9 insertions(+), 5 deletions(-)

Review commit: https://reviewboard.mozilla.org/r/45921/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/45921/
From 7c9bbc1952be0da6c6d7b2d745edb4a05a26715a Mon Sep 17 00:00:00 2001
---
 gfx/gl/GLContext.cpp | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Review commit: https://reviewboard.mozilla.org/r/45923/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/45923/
Comment on attachment 8738264 [details]
MozReview Request: Bug 1262265 - Cleanup InitWithPrefix. - r?jrmuizel

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44399/diff/1-2/
Can't we just get rid of GetGlobalContext from the public GLContextProvider API?

There aren't any callers of it.

GLContextProviderCGL doesn't even implement it usefully, context sharing is never enabled for CGL [1] .

I doubt the other backends really need context sharing either.



[1] http://mxr.mozilla.org/mozilla-central/source/gfx/gl/GLContextProviderCGL.mm#235
(In reply to Matt Woodrow (:mattwoodrow) from comment #14)
> Can't we just get rid of GetGlobalContext from the public GLContextProvider
> API?
> 
> There aren't any callers of it.
> 
> GLContextProviderCGL doesn't even implement it usefully, context sharing is
> never enabled for CGL [1] .
> 
> I doubt the other backends really need context sharing either.
> 
> 
> 
> [1]
> http://mxr.mozilla.org/mozilla-central/source/gfx/gl/GLContextProviderCGL.
> mm#235

F+, but I think EAGL uses it actually.

Snorp: Does iOS rely on context sharing via GetGlobalContext?
Flags: needinfo?(jgilbert) → needinfo?(snorp)
Attachment #8740690 - Flags: review?(jmuizelaar) → review+
Comment on attachment 8740690 [details]
MozReview Request: Simplify and standardize GetGlobalContext. r?jrmuizel

https://reviewboard.mozilla.org/r/45915/#review43449
I backed out d3aab3c4eb5f (comment 17). Backout cset is:
 https://hg.mozilla.org/integration/mozilla-inbound/rev/1a1420dcc987

Backout was for GL-related fatal assertion failures like:
Assertion failure: internalFormat == 0x1908, at c:/builds/moz2_slave/m-in-w32-d-0000000000000000000/build/src/gfx/gl/GLContext.cpp:2925
Sample log: https://treeherder.mozilla.org/logviewer.html#?job_id=26164012&repo=mozilla-inbound

And there are more of these on the inbound treeherder run where this patch had landed:
https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=d3aab3c4eb5fae8b7768438577c95b0dc364e215
Flags: needinfo?(jgilbert)
From 34a6b4b4db28ebec0d0ceb021a418117a8c89444 Mon Sep 17 00:00:00 2001
---
 gfx/gl/GLContextProviderEGL.cpp | 4 ++++
 gfx/gl/GLLibraryEGL.h           | 3 +++
 2 files changed, 7 insertions(+)

Review commit: https://reviewboard.mozilla.org/r/47943/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/47943/
Comment on attachment 8738264 [details]
MozReview Request: Bug 1262265 - Cleanup InitWithPrefix. - r?jrmuizel

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44399/diff/2-3/
Comment on attachment 8740690 [details]
MozReview Request: Simplify and standardize GetGlobalContext. r?jrmuizel

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45915/diff/1-2/
Comment on attachment 8740691 [details]
MozReview Request: Centralize EnsureInitialized. r?jrmuizel

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45917/diff/1-2/
Comment on attachment 8740692 [details]
MozReview Request: Don't call Init() twice. r?jrmuizel

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45919/diff/1-2/
Comment on attachment 8740693 [details]
MozReview Request: Compiler says 'no'. r?jrmuizel

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45921/diff/1-2/
Comment on attachment 8740694 [details]
MozReview Request: Fix the conditional.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45923/diff/1-2/
Comment on attachment 8740691 [details]
MozReview Request: Centralize EnsureInitialized. r?jrmuizel

https://reviewboard.mozilla.org/r/45917/#review44899
Attachment #8740691 - Flags: review?(jmuizelaar) → review+
Attachment #8740692 - Flags: review?(jmuizelaar) → review+
Comment on attachment 8740692 [details]
MozReview Request: Don't call Init() twice. r?jrmuizel

https://reviewboard.mozilla.org/r/45919/#review44901
Comment on attachment 8740693 [details]
MozReview Request: Compiler says 'no'. r?jrmuizel

https://reviewboard.mozilla.org/r/45921/#review44903
Attachment #8740693 - Flags: review?(jmuizelaar) → review+
Attachment #8743624 - Flags: review?(jmuizelaar)
Flags: needinfo?(jgilbert)
Comment on attachment 8743624 [details]
MozReview Request: r?jrmuizel - Require initialization for egl.IsANGLE().

https://reviewboard.mozilla.org/r/47943/#review45033
Attachment #8743624 - Flags: review?(jmuizelaar) → review+
https://hg.mozilla.org/mozilla-central/rev/ca8fae075ef3
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
(In reply to Jeff Gilbert [:jgilbert] from comment #15)
> (In reply to Matt Woodrow (:mattwoodrow) from comment #14)
> > Can't we just get rid of GetGlobalContext from the public GLContextProvider
> > API?
> > 
> > There aren't any callers of it.
> > 
> > GLContextProviderCGL doesn't even implement it usefully, context sharing is
> > never enabled for CGL [1] .
> > 
> > I doubt the other backends really need context sharing either.
> > 
> > 
> > 
> > [1]
> > http://mxr.mozilla.org/mozilla-central/source/gfx/gl/GLContextProviderCGL.
> > mm#235
> 
> F+, but I think EAGL uses it actually.
> 
> Snorp: Does iOS rely on context sharing via GetGlobalContext?

Sorry for the late reply, but yes, it does use that.
Flags: needinfo?(snorp)
You need to log in before you can comment on or make changes to this bug.