Call MakeCurrent implicitly

RESOLVED FIXED in Firefox 59

Status

()

enhancement
P1
normal
RESOLVED FIXED
2 years ago
Last year

People

(Reporter: jgilbert, Assigned: jgilbert)

Tracking

unspecified
mozilla59
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox57 wontfix, firefox59 fixed)

Details

(Whiteboard: gfx-noted)

Attachments

(8 attachments, 6 obsolete attachments)

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
Assignee

Description

2 years ago
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

Updated

2 years ago
Duplicate of this bug: 999713
Comment hidden (mozreview-request)
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

2 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

2 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

2 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

2 years ago
Flags: needinfo?(jgilbert) → needinfo?(jmuizelaar)

Comment 14

2 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

2 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

2 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

2 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

2 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

2 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

2 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 hidden (mozreview-request)
Assignee

Updated

2 years ago
Attachment #8897232 - Attachment is obsolete: true
Assignee

Updated

2 years ago
Attachment #8897233 - Attachment is obsolete: true
Assignee

Updated

2 years ago
Attachment #8897234 - Attachment is obsolete: true
Assignee

Updated

2 years ago
Attachment #8897235 - Attachment is obsolete: true
Assignee

Updated

2 years ago
Attachment #8897578 - Attachment is obsolete: true
Assignee

Updated

2 years ago
Attachment #8898492 - Attachment is obsolete: true
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 34

2 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

2 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

2 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+
How is this different from last time?
Flags: needinfo?(jmuizelaar) → needinfo?(jgilbert)
Assignee

Comment 38

2 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

2 years ago
Flags: needinfo?(jmuizelaar)

Comment 39

2 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

2 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

2 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

2 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

2 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

2 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

2 years ago
Attachment #8931327 - Flags: review-

Comment 46

2 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

2 years ago
I moved mfbt/ThreadLocal changes to bug 1421487.
Assignee

Updated

2 years ago
Flags: needinfo?(jmuizelaar)
Depends on: 1421960
Assignee

Updated

Last year
Blocks: 1445941
You need to log in before you can comment on or make changes to this bug.