1390386 - Call MakeCurrent implicitly

Status: RESOLVED FIXED

(Core :: Graphics: CanvasWebGL, enhancement, P1)

Description

[Kelsey Gilbert [:jgilbert]]

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 1

[Kelsey Gilbert [:jgilbert]]

Attachment: Bug 1390386 - Allow null EGLSurfaces. -

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

Comment 2

[Kelsey Gilbert [:jgilbert]]

Attachment: Bug 1390386 - Add GLContext::mImplicitMakeCurrent. -

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

Comment 3

[Kelsey Gilbert [:jgilbert]]

Attachment: Bug 1390386 - Remove old TLS current-context check. -

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

Comment 4

[Kelsey Gilbert [:jgilbert]]

Attachment: Bug 1390386 - IsCurrent should understand mImplicitMakeCurrent. -

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

Comment 5

[Kelsey Gilbert [:jgilbert]]

Attachment: Bug 1390386 - mfbt/ThreadLocal should support pointers-to-const with the pthread impl. -

reinterpret_cast can't convert const T* to void*. Just use c-style casts.

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

Comment 7

[Kelsey Gilbert [:jgilbert]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=04de301bf9fa5d1d91ceb53dab1a9592e501dab4

Comment 8

[Kelsey Gilbert [:jgilbert]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5e00747f1321ca100daddd0fd66d207030985f64

Comment 9

[Kelsey Gilbert [:jgilbert]]

Attachment: Bug 1390386 - Remove duplicate IsCurrent checks in MakeCurrentImpls. -

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

Comment 10

[Jeff Muizelaar [:jrmuizel]]

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?

Comment 11

[Daosheng Mu[:daoshengmu]]

Comment on attachment 8897231 [details]
Bug 1390386 - Allow null EGLSurfaces. -

https://reviewboard.mozilla.org/r/168512/#review174232

LGTM

Comment 12

[Daosheng Mu[:daoshengmu]]

Comment on attachment 8897233 [details]
Bug 1390386 - Remove old TLS current-context check. -

https://reviewboard.mozilla.org/r/168516/#review174234

Comment 13

[Kelsey Gilbert [:jgilbert]]

(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.

Comment 14

[Nathan Froyd [:froydnj]]

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.

Comment 15

[Kelsey Gilbert [:jgilbert]]

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

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 8897232 [details]
Bug 1390386 - Add GLContext::mImplicitMakeCurrent. -

https://reviewboard.mozilla.org/r/168514/#review175008

Comment 17

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 8897234 [details]
Bug 1390386 - IsCurrent should understand mImplicitMakeCurrent. -

https://reviewboard.mozilla.org/r/168518/#review175012

Comment 18

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 8897578 [details]
Bug 1390386 - Remove duplicate IsCurrent checks in MakeCurrentImpls. -

https://reviewboard.mozilla.org/r/168846/#review175014

Comment 19

[Kelsey Gilbert [:jgilbert]]

Attachment: Bug 1390386 - fGetError should use BEFORE_GL_CALL. -

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

Comment 20

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8898492 [details]
Bug 1390386 - fGetError should use BEFORE_GL_CALL. -

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

Comment 21

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 8898492 [details]
Bug 1390386 - fGetError should use BEFORE_GL_CALL. -

https://reviewboard.mozilla.org/r/169846/#review175112

Comment 22

[Pulsebot]

Pushed by [e-mail]:
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

[Phil Ringnalda (:philor)]

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 24

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8897231 [details]
Bug 1390386 - Allow null EGLSurfaces. -

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

Comment 25

[Kelsey Gilbert [:jgilbert]]

Attachment: Bug 1390386 - Make MakeCurrent const. -

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

Comment 26

[Kelsey Gilbert [:jgilbert]]

Attachment: Bug 1390386 - Add GLContext::mImplicitMakeCurrent. -

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

Comment 27

[Kelsey Gilbert [:jgilbert]]

Attachment: Bug 1390386 - Remove old TLS current-context check. -

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

Comment 28

[Kelsey Gilbert [:jgilbert]]

Attachment: Bug 1390386 - IsCurrent should understand mImplicitMakeCurrent. -

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

Comment 29

[Kelsey Gilbert [:jgilbert]]

Attachment: Bug 1390386 - Remove duplicate IsCurrent checks in MakeCurrentImpls. -

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

Comment 30

[Kelsey Gilbert [:jgilbert]]

Attachment: Bug 1390386 - fGetError should use BEFORE_GL_CALL. -

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

Comment 31

[Kelsey Gilbert [:jgilbert]]

Attachment: Bug 1390386 - Use uintptr for the thread-local. -

Error was:
obj-firefox/dist/include/mozilla/ThreadLocal.h:153:15: error: reinterpret_cast from 'typename Helper<const GLContext *>::Type' (aka 'const mozilla::gl::GLContext *') to 'void *' casts away qualifiers

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

Comment 32

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8897231 [details]
Bug 1390386 - Allow null EGLSurfaces. -

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

Comment 33

[Kelsey Gilbert [:jgilbert]]

Seems green:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=af63f9680295ffc30184bd1c2f79dda98601bc5c

Comment 34

[Code Review Bot [:reviewbot]]

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

[Daosheng Mu[:daoshengmu]]

Comment on attachment 8931321 [details]
Bug 1390386 - Make MakeCurrent const. -

https://reviewboard.mozilla.org/r/202456/#review208056

lgtm.

Comment 36

[Daosheng Mu[:daoshengmu]]

Comment on attachment 8931323 [details]
Bug 1390386 - Remove old TLS current-context check. -

https://reviewboard.mozilla.org/r/202460/#review208060

lgtm

Comment 37

[Jeff Muizelaar [:jrmuizel]]

How is this different from last time?

Comment 38

[Kelsey Gilbert [:jgilbert]]

(In reply to Jeff Muizelaar [:jrmuizel] from comment #37)
> How is this different from last time?

I rebased it and it's green.

Comment 39

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 8931324 [details]
Bug 1390386 - IsCurrent should understand mImplicitMakeCurrent. -

https://reviewboard.mozilla.org/r/202462/#review208650

Comment 40

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 8931325 [details]
Bug 1390386 - Remove duplicate IsCurrent checks in MakeCurrentImpls. -

https://reviewboard.mozilla.org/r/202464/#review208652

Comment 41

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 8931326 [details]
Bug 1390386 - fGetError should use BEFORE_GL_CALL. -

https://reviewboard.mozilla.org/r/202466/#review208654

Comment 42

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 8931322 [details]
Bug 1390386 - Add GLContext::mImplicitMakeCurrent. -

https://reviewboard.mozilla.org/r/202458/#review208656

Comment 43

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8931327 [details]
Bug 1390386 - Use uintptr for the thread-local. -

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

Comment 44

[Daosheng Mu[:daoshengmu]]

Comment on attachment 8931327 [details]
Bug 1390386 - Use uintptr for the thread-local. -

https://reviewboard.mozilla.org/r/202468/#review208740

lgtm

Comment 45

[Jeff Walden [:Waldo]]

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.

Comment 46

[Pulsebot]

Pushed by [e-mail]:
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

Comment 47

[Kelsey Gilbert [:jgilbert]]

I moved mfbt/ThreadLocal changes to bug 1421487.

Comment 48

[Dorel Luca [:dluca]]

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