Closed
Bug 1390386
Opened 7 years ago
Closed 6 years ago
Call MakeCurrent implicitly
Categories
(Core :: Graphics: CanvasWebGL, enhancement, P1)
Core
Graphics: CanvasWebGL
Tracking
()
RESOLVED
FIXED
mozilla59
People
(Reporter: jgilbert, Assigned: jgilbert)
References
Details
(Whiteboard: gfx-noted)
Attachments
(8 files, 6 obsolete files)
59 bytes,
text/x-review-board-request
|
daoshengmu
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
daoshengmu
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
jrmuizel
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
daoshengmu
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
jrmuizel
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
jrmuizel
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
jrmuizel
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
daoshengmu
:
review+
|
Details |
Ensuring MakeCurrent for each GL command shouldn't hurt us in hot functions (draw calls, uniforms, etc), and makes life easier and safer otherwise. We can keep this opt-in so the compositor, which can handle MakeCurrent efficiently, can continue to ensure MakeCurrent minimally.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 7•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=04de301bf9fa5d1d91ceb53dab1a9592e501dab4
Assignee | ||
Comment 8•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=5e00747f1321ca100daddd0fd66d207030985f64
Comment hidden (mozreview-request) |
Comment 10•7 years ago
|
||
What's preventing us from moving from doing the MakeCurrent on every WebGL call to only doing it as needed? i.e. What prevents us from knowing when we're switching contexts?
Flags: needinfo?(jgilbert)
Comment 11•7 years ago
|
||
mozreview-review |
Comment on attachment 8897231 [details] Bug 1390386 - Allow null EGLSurfaces. - https://reviewboard.mozilla.org/r/168512/#review174232 LGTM
Attachment #8897231 -
Flags: review?(dmu) → review+
Comment 12•7 years ago
|
||
mozreview-review |
Comment on attachment 8897233 [details] Bug 1390386 - Remove old TLS current-context check. - https://reviewboard.mozilla.org/r/168516/#review174234
Attachment #8897233 -
Flags: review?(dmu) → review+
Assignee | ||
Comment 13•7 years ago
|
||
(NB: MakeCurrent(false) is already actually EnsureMakeCurrent) Context can change due to js calling a different webgl context, binary plugins (which are going away, but e.g. Flash will be around for a bit still), occasionally native toolkit calls (we've hit this on Mac before), and context loss. (calls against a lost context fails safely though) Also, what we do right now is try to call MakeCurrent at least once per WebGL command/use of GL. While we have DEBUG-build runtime asserts that check that the context is current before each GL call, we don't have static guarantees, and so it's easy to get a false-negative green try run even with a missing MakeCurrent.
Assignee | ||
Updated•7 years ago
|
Flags: needinfo?(jgilbert) → needinfo?(jmuizelaar)
Comment 14•7 years ago
|
||
mozreview-review |
Comment on attachment 8897235 [details] Bug 1390386 - mfbt/ThreadLocal should support pointers-to-const with the pthread impl. - https://reviewboard.mozilla.org/r/168520/#review174284 I guess getting all the template magic right to handle this without C-style casts would be painful, so this is OK. A test somewhere to verify that `ThreadLocal<const T>` worked would be great.
Attachment #8897235 -
Flags: review?(nfroyd) → review+
Assignee | ||
Comment 15•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8897235 [details] Bug 1390386 - mfbt/ThreadLocal should support pointers-to-const with the pthread impl. - https://reviewboard.mozilla.org/r/168520/#review174284 It's ThreadLocal<const T*>, which is a ThreadLocal<U>, not ThreadLocal<const U>.
Comment 16•7 years ago
|
||
mozreview-review |
Comment on attachment 8897232 [details] Bug 1390386 - Add GLContext::mImplicitMakeCurrent. - https://reviewboard.mozilla.org/r/168514/#review175008
Attachment #8897232 -
Flags: review?(jmuizelaar) → review+
Comment 17•7 years ago
|
||
mozreview-review |
Comment on attachment 8897234 [details] Bug 1390386 - IsCurrent should understand mImplicitMakeCurrent. - https://reviewboard.mozilla.org/r/168518/#review175012
Attachment #8897234 -
Flags: review?(jmuizelaar) → review+
Comment 18•7 years ago
|
||
mozreview-review |
Comment on attachment 8897578 [details] Bug 1390386 - Remove duplicate IsCurrent checks in MakeCurrentImpls. - https://reviewboard.mozilla.org/r/168846/#review175014
Attachment #8897578 -
Flags: review?(jmuizelaar) → review+
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 21•7 years ago
|
||
mozreview-review |
Comment on attachment 8898492 [details] Bug 1390386 - fGetError should use BEFORE_GL_CALL. - https://reviewboard.mozilla.org/r/169846/#review175112
Attachment #8898492 -
Flags: review?(jmuizelaar) → review+
Comment 22•7 years ago
|
||
Pushed by jgilbert@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/d1286d0d2c79 Make MakeCurrent const. - r=daoshengmu https://hg.mozilla.org/integration/mozilla-inbound/rev/9d16670edeb5 Add GLContext::mImplicitMakeCurrent. - r=jrmuizel https://hg.mozilla.org/integration/mozilla-inbound/rev/1314405cf812 Remove old TLS current-context check. - r=daoshengmu https://hg.mozilla.org/integration/mozilla-inbound/rev/ed6857bdc17b IsCurrent should understand mImplicitMakeCurrent. - r=jrmuizel https://hg.mozilla.org/integration/mozilla-inbound/rev/3364e6589731 mfbt/ThreadLocal should support pointers-to-const with the pthread impl. - r=froydnj https://hg.mozilla.org/integration/mozilla-inbound/rev/caffdbefd427 Remove duplicate IsCurrent checks in MakeCurrentImpls. - r=jrmuizel https://hg.mozilla.org/integration/mozilla-inbound/rev/22b11f9aedd3 fGetError should use BEFORE_GL_CALL. - r=jrmuizel
Comment 23•7 years ago
|
||
Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/fdbcfc9b7987 for Android crashes in testSettingsPages like https://treeherder.mozilla.org/logviewer.html#?job_id=124037538&repo=mozilla-inbound
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Attachment #8897232 -
Attachment is obsolete: true
Assignee | ||
Updated•7 years ago
|
Attachment #8897233 -
Attachment is obsolete: true
Assignee | ||
Updated•7 years ago
|
Attachment #8897234 -
Attachment is obsolete: true
Assignee | ||
Updated•7 years ago
|
Attachment #8897235 -
Attachment is obsolete: true
Assignee | ||
Updated•7 years ago
|
Attachment #8897578 -
Attachment is obsolete: true
Assignee | ||
Updated•7 years ago
|
Attachment #8898492 -
Attachment is obsolete: true
Updated•7 years ago
|
status-firefox57:
--- → wontfix
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 33•6 years ago
|
||
Seems green: https://treeherder.mozilla.org/#/jobs?repo=try&revision=af63f9680295ffc30184bd1c2f79dda98601bc5c
Comment 34•6 years ago
|
||
mozreview-review |
Comment on attachment 8931322 [details] Bug 1390386 - Add GLContext::mImplicitMakeCurrent. - https://reviewboard.mozilla.org/r/202458/#review207792 C/C++ static analysis found 1 defect in this patch. You can run this analysis locally with: `./mach static-analysis check path/to/file.cpp` ::: gfx/gl/GLContext.cpp:3117 (Diff revision 1) > + printf_stderr("[gl:%p] < %s [%s (0x%04x)]\n", this, funcName, > + GLErrorToString(err), err); > + } > + > + if (err != LOCAL_GL_NO_ERROR && > + !mLocalErrorScopeStack.size()) Warning: The 'empty' method should be used to check for emptiness instead of 'size' [clang-tidy: readability-container-size-empty] !mLocalErrorScopeStack.size()) ^ mLocalErrorScopeStack.empty()
Comment 35•6 years ago
|
||
mozreview-review |
Comment on attachment 8931321 [details] Bug 1390386 - Make MakeCurrent const. - https://reviewboard.mozilla.org/r/202456/#review208056 lgtm.
Attachment #8931321 -
Flags: review?(dmu) → review+
Comment 36•6 years ago
|
||
mozreview-review |
Comment on attachment 8931323 [details] Bug 1390386 - Remove old TLS current-context check. - https://reviewboard.mozilla.org/r/202460/#review208060 lgtm
Attachment #8931323 -
Flags: review?(dmu) → review+
Comment 37•6 years ago
|
||
How is this different from last time?
Flags: needinfo?(jmuizelaar) → needinfo?(jgilbert)
Assignee | ||
Comment 38•6 years ago
|
||
(In reply to Jeff Muizelaar [:jrmuizel] from comment #37) > How is this different from last time? I rebased it and it's green.
Flags: needinfo?(jgilbert)
Assignee | ||
Updated•6 years ago
|
Flags: needinfo?(jmuizelaar)
Comment 39•6 years ago
|
||
mozreview-review |
Comment on attachment 8931324 [details] Bug 1390386 - IsCurrent should understand mImplicitMakeCurrent. - https://reviewboard.mozilla.org/r/202462/#review208650
Attachment #8931324 -
Flags: review?(jmuizelaar) → review+
Comment 40•6 years ago
|
||
mozreview-review |
Comment on attachment 8931325 [details] Bug 1390386 - Remove duplicate IsCurrent checks in MakeCurrentImpls. - https://reviewboard.mozilla.org/r/202464/#review208652
Attachment #8931325 -
Flags: review?(jmuizelaar) → review+
Comment 41•6 years ago
|
||
mozreview-review |
Comment on attachment 8931326 [details] Bug 1390386 - fGetError should use BEFORE_GL_CALL. - https://reviewboard.mozilla.org/r/202466/#review208654
Attachment #8931326 -
Flags: review?(jmuizelaar) → review+
Comment 42•6 years ago
|
||
mozreview-review |
Comment on attachment 8931322 [details] Bug 1390386 - Add GLContext::mImplicitMakeCurrent. - https://reviewboard.mozilla.org/r/202458/#review208656
Attachment #8931322 -
Flags: review?(jmuizelaar) → review+
Comment hidden (mozreview-request) |
Comment 44•6 years ago
|
||
mozreview-review |
Comment on attachment 8931327 [details] Bug 1390386 - Use uintptr for the thread-local. - https://reviewboard.mozilla.org/r/202468/#review208740 lgtm
Attachment #8931327 -
Flags: review?(dmu) → review+
Comment 45•6 years ago
|
||
mozreview-review |
Comment on attachment 8931327 [details] Bug 1390386 - Use uintptr for the thread-local. - https://reviewboard.mozilla.org/r/202468/#review208718 More or less okay pretty much, but enough little changes here that it's worth a second look -- or at least a roadbump before landing, given mfbt code changes are always a bit finicky given far-flung users. ::: mfbt/ThreadLocal.h:79 (Diff revision 1) > // achieves that without penalizing the common case of ThreadLocals > // instantiated using a pointer type. > template<typename S> > struct Helper > { > - typedef uintptr_t Type; > + static inline void* ToVoidStar(const S x) { In this context -- on a function defined inside a class body -- "inline" is unnecessary. (Such functions are already implicitly inline. And of course the inline specifier doesn't *actually* inline, so there's that too.) So, remove the inline here and on the other functions like this? ::: mfbt/ThreadLocal.h:80 (Diff revision 1) > // instantiated using a pointer type. > template<typename S> > struct Helper > { > - typedef uintptr_t Type; > + static inline void* ToVoidStar(const S x) { > + const auto u = static_cast<uintptr_t>(x); Remove the const here, IMO -- and in the others. ::: mfbt/ThreadLocal.h:94 (Diff revision 1) > > -template<typename S> > -struct Helper<S *> > +template<typename T> > +struct Helper<T*> > { > - typedef S *Type; > + static inline void* ToVoidStar(T* const x) { > + return (void*)x; Use `static_cast<void*>(const_cast<typename RemoveConst<T>::Type*>(x))` instead here. And actually -- remove the const on this and the other function argument signatures? That isn't actually necessary (it would only have any effect *inside* these functions, but they don't modify the argument so it doesn't matter) because pointers are pass-by-copy. ::: mfbt/ThreadLocal.h:97 (Diff revision 1) > { > - typedef S *Type; > + static inline void* ToVoidStar(T* const x) { > + return (void*)x; > + } > + static inline T* FromVoidStar(void* const x) { > + return (T*)x; I'm pretty sure you can use `static_cast<T*>(x)` here.
Attachment #8931327 -
Flags: review-
Assignee | ||
Updated•6 years ago
|
Attachment #8931327 -
Flags: review-
Comment 46•6 years ago
|
||
Pushed by jgilbert@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/538dfa8c4c3b Make MakeCurrent const. - r=daoshengmu https://hg.mozilla.org/integration/mozilla-inbound/rev/6d4669e3d2da Add GLContext::mImplicitMakeCurrent. - r=jrmuizel https://hg.mozilla.org/integration/mozilla-inbound/rev/3ccf02d862c8 Remove old TLS current-context check. - r=daoshengmu https://hg.mozilla.org/integration/mozilla-inbound/rev/3b910891a85b IsCurrent should understand mImplicitMakeCurrent. - r=jrmuizel https://hg.mozilla.org/integration/mozilla-inbound/rev/b98d0d835d12 Remove duplicate IsCurrent checks in MakeCurrentImpls. - r=jrmuizel https://hg.mozilla.org/integration/mozilla-inbound/rev/d0c82eb2cea9 fGetError should use BEFORE_GL_CALL. - r=jrmuizel https://hg.mozilla.org/integration/mozilla-inbound/rev/1042caf2d4ce Allow null EGLSurfaces. - r=daoshengmu https://hg.mozilla.org/integration/mozilla-inbound/rev/a3d350120375 Use uintptr for the thread-local. - r=daoshengmu
Assignee | ||
Comment 47•6 years ago
|
||
I moved mfbt/ThreadLocal changes to bug 1421487.
Assignee | ||
Updated•6 years ago
|
Flags: needinfo?(jmuizelaar)
Comment 48•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/538dfa8c4c3b https://hg.mozilla.org/mozilla-central/rev/6d4669e3d2da https://hg.mozilla.org/mozilla-central/rev/3ccf02d862c8 https://hg.mozilla.org/mozilla-central/rev/3b910891a85b https://hg.mozilla.org/mozilla-central/rev/b98d0d835d12 https://hg.mozilla.org/mozilla-central/rev/d0c82eb2cea9 https://hg.mozilla.org/mozilla-central/rev/1042caf2d4ce https://hg.mozilla.org/mozilla-central/rev/a3d350120375
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox59:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
You need to log in
before you can comment on or make changes to this bug.
Description
•