Closed Bug 1390386 Opened 7 years ago Closed 6 years ago

Call MakeCurrent implicitly

Categories

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

enhancement

Tracking

()

RESOLVED FIXED
mozilla59
Tracking Status
firefox57 --- wontfix
firefox59 --- fixed

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.
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 on attachment 8897231 [details]
Bug 1390386 - Allow null EGLSurfaces. -

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

LGTM
Attachment #8897231 - Flags: review?(dmu) → 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+
(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.
Flags: needinfo?(jgilbert) → needinfo?(jmuizelaar)
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+
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 on attachment 8897232 [details]
Bug 1390386 - Add GLContext::mImplicitMakeCurrent. -

https://reviewboard.mozilla.org/r/168514/#review175008
Attachment #8897232 - Flags: review?(jmuizelaar) → 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 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 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+
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
Attachment #8897232 - Attachment is obsolete: true
Attachment #8897233 - Attachment is obsolete: true
Attachment #8897234 - Attachment is obsolete: true
Attachment #8897235 - Attachment is obsolete: true
Attachment #8897578 - Attachment is obsolete: true
Attachment #8898492 - Attachment is obsolete: true
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 on attachment 8931321 [details]
Bug 1390386 - Make MakeCurrent const. -

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

lgtm.
Attachment #8931321 - Flags: review?(dmu) → 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)
(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)
Flags: needinfo?(jmuizelaar)
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 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 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 on attachment 8931322 [details]
Bug 1390386 - Add GLContext::mImplicitMakeCurrent. -

https://reviewboard.mozilla.org/r/202458/#review208656
Attachment #8931322 - Flags: review?(jmuizelaar) → 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 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-
Attachment #8931327 - Flags: review-
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
I moved mfbt/ThreadLocal changes to bug 1421487.
Flags: needinfo?(jmuizelaar)
Depends on: 1421960
Blocks: 1445941
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: