1451183 - Add capability of resetting GLLibraryEGL state

Status: RESOLVED FIXED

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

Description

[Sotaro Ikeda [:sotaro]]

This bug is created based on Bug 1438456 Comment 10. If we want to create a new EGLDisplay, we should completely reset to our starting state.

Comment 1

[Sotaro Ikeda [:sotaro]]

Attachment: patch - GLLibraryEGL::Shutdown()

Comment 2

[Sotaro Ikeda [:sotaro]]

Attachment: patch - GLLibraryEGL::Shutdown()

Comment 3

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8964803 [details] [diff] [review]
patch - GLLibraryEGL::Shutdown()

Review of attachment 8964803 [details] [diff] [review]:
-----------------------------------------------------------------

Wait, does this work? Don't we have a bunch of GL contexts potenntially floating around?

Comment 4

[Sotaro Ikeda [:sotaro]]

In current implementation, GL context existence is checked by client side by Bug 1364504. What is a better way to check it?

Comment 5

[Sotaro Ikeda [:sotaro]]

I am going to update the patch to check if GL context does not exist.

Comment 6

[Sotaro Ikeda [:sotaro]]

Attachment: patch - GLLibraryEGL::Shutdown()

Added a check if egl is still used by GLContext.

Comment 7

[Sotaro Ikeda [:sotaro]]

:jgilbert, what is a better way to handle the problem? Is attachment attachment 8965624 [details] [diff] [review] OK for it? Or is there another better way to handle it?

Comment 8

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8965624 [details] [diff] [review]
patch - GLLibraryEGL::Shutdown()

Review of attachment 8965624 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/gl/GLLibraryEGL.cpp
@@ +664,5 @@
>  #undef SYMBOL
>  #undef END_OF_SYMBOLS
>  
> +bool
> +GLLibraryEGL::Shutdown()

Is it actually OK if Shutdown is fallible like this?

@@ +673,5 @@
> +    }
> +
> +    if (mGLContextCount > 0) {
> +        // GLContexts still use EGL
> +        return false;

You probably want to destroy all outstanding contexts and mark them as context-lost (which we're supposed to handle correctly), rather than handling a shutdown failure.

@@ +685,5 @@
> +    }
> +    mSymbols = {};
> +    if (mEGLLibrary) {
> +      PR_UnloadLibrary(mEGLLibrary);
> +      mEGLLibrary = nullptr;

4-space

::: gfx/gl/GLLibraryEGL.h
@@ +368,5 @@
>  
>      bool EnsureInitialized(bool forceAccel, nsACString* const out_failureId);
>  
> +    bool IsInitialized() const {
> +        return mInitialized;

This appears to be unused.

@@ +374,5 @@
> +
> +    bool Shutdown();
> +
> +    void IncGLContext() {
> +        mGLContextCount++;

Since these mutator methods are public, just make the member var public and drop the getter/setters.

@@ +517,5 @@
>      RefPtr<GLContext> mReadbackGL;
>  
>      bool mIsANGLE;
>      bool mIsWARP;
> +    Atomic<int> mGLContextCount;

If it's never supposed to go negative, prefer unsigned types. uint32_t is fine here.

Comment 9

[Sotaro Ikeda [:sotaro]]

Comment on attachment 8965624 [details] [diff] [review]
patch - GLLibraryEGL::Shutdown()

Review of attachment 8965624 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/gl/GLLibraryEGL.cpp
@@ +664,5 @@
>  #undef SYMBOL
>  #undef END_OF_SYMBOLS
>  
> +bool
> +GLLibraryEGL::Shutdown()

If we destroy all outstanding contexts and mark them as context-lost, we do not need fallible.

@@ +673,5 @@
> +    }
> +
> +    if (mGLContextCount > 0) {
> +        // GLContexts still use EGL
> +        return false;

Yea, it looks better to destroy them.

Comment 10

[Sotaro Ikeda [:sotaro]]

Attachment: patch - GLLibraryEGL::Shutdown()

Applied the comment.

Comment 11

[Sotaro Ikeda [:sotaro]]

To destroy all outstanding contexts, attachment 8973927 [details] [diff] [review] uses nsTHashtable<nsPtrHashKey<GLContextEGL>> for holding all contexts. And it changed GLContextEGL::mContext to non const.

Comment 12

[Sotaro Ikeda [:sotaro]]

Comment on attachment 8973927 [details] [diff] [review]
patch - GLLibraryEGL::Shutdown()

:jgilbert, can you review the patch again?

Comment 13

[Sotaro Ikeda [:sotaro]]

:jgilbert, can you review the patch?

Comment 14

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8973927 [details] [diff] [review]
patch - GLLibraryEGL::Shutdown()

Review of attachment 8973927 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/gl/GLContextEGL.h
@@ +94,5 @@
>      EGLSurface GetEGLSurface() const {
>          return mSurface;
>      }
>  
> +    EGLContext GetEGLContext() const {

Context() is better.

::: gfx/gl/GLLibraryEGL.cpp
@@ +672,5 @@
> +        return;
> +    }
> +
> +    {
> +        MutexAutoLock lock(mGLContextEGLsLock);

This lock needs to cover the whole function to prevent racing teardown/re-init, doesn't it?

@@ +674,5 @@
> +
> +    {
> +        MutexAutoLock lock(mGLContextEGLsLock);
> +        for (auto it = mGLContextEGLs.Iter(); !it.Done(); it.Next()) {
> +            it.Get()->GetKey()->Destroy();

Just use a std::unordered_map please.

@@ +691,5 @@
> +        mEGLLibrary = nullptr;
> +    }
> +    mIsANGLE = false;
> +    mIsWARP = false;
> +    mInitialized = false;

This is fragile to have to list member resets here. Why don't we actually destroy the GLLibraryEGL object?

::: gfx/gl/GLLibraryEGL.h
@@ +376,5 @@
> +    void AddGLContextEGL(GLContextEGL* aContext);
> +
> +    void RemoveGLContextEGL(GLContextEGL* aContext);
> +
> +    uint32_t GetGLContextCount();

This is unused, please remove.

Comment 15

[Sotaro Ikeda [:sotaro]]

(In reply to Jeff Gilbert [:jgilbert] from comment #14)
> Comment on attachment 8973927 [details] [diff] [review]
> patch - GLLibraryEGL::Shutdown()
> 
> Review of attachment 8973927 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/gl/GLContextEGL.h
> @@ +94,5 @@
> >      EGLSurface GetEGLSurface() const {
> >          return mSurface;
> >      }
> >  
> > +    EGLContext GetEGLContext() const {
> 
> Context() is better.

I will update in a next patch.

> 
> ::: gfx/gl/GLLibraryEGL.cpp
> @@ +672,5 @@
> > +        return;
> > +    }
> > +
> > +    {
> > +        MutexAutoLock lock(mGLContextEGLsLock);
> 
> This lock needs to cover the whole function to prevent racing
> teardown/re-init, doesn't it?

Yea, I will update in a next patch.

> 
> @@ +674,5 @@
> > +
> > +    {
> > +        MutexAutoLock lock(mGLContextEGLsLock);
> > +        for (auto it = mGLContextEGLs.Iter(); !it.Done(); it.Next()) {
> > +            it.Get()->GetKey()->Destroy();
> 
> Just use a std::unordered_map please.

I will update in a next patch.

> 
> @@ +691,5 @@
> > +        mEGLLibrary = nullptr;
> > +    }
> > +    mIsANGLE = false;
> > +    mIsWARP = false;
> > +    mInitialized = false;
> 
> This is fragile to have to list member resets here. Why don't we actually
> destroy the GLLibraryEGL object?

It is because, sEGLLibrary is defined as global object. Is there a way to destroy global object?
  https://dxr.mozilla.org/mozilla-central/source/gfx/gl/GLLibraryEGL.cpp#46
  https://dxr.mozilla.org/mozilla-central/source/gfx/gl/GLLibraryEGL.h#506

> 
> ::: gfx/gl/GLLibraryEGL.h
> @@ +376,5 @@
> > +    void AddGLContextEGL(GLContextEGL* aContext);
> > +
> > +    void RemoveGLContextEGL(GLContextEGL* aContext);
> > +
> > +    uint32_t GetGLContextCount();
> 
> This is unused, please remove.

I will remove it in a next patch.

Comment 16

[Sotaro Ikeda [:sotaro]]

Attachment: patch - GLLibraryEGL::Shutdown()

One comment could not be applied since sEGLLibrary is defined as global object.
Is there a way to destroy global object?

Comment 17

[Sotaro Ikeda [:sotaro]]

Attachment: patch - GLLibraryEGL::Shutdown()

Update nits.

Comment 18

[Sotaro Ikeda [:sotaro]]

Attachment: patch - GLLibraryEGL::Shutdown()

Update nits.

Comment 19

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Make EGLLibrary destroyable

Comment 20

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8979872 [details] [diff] [review]
patch - Make EGLLibrary destroyable

Review of attachment 8979872 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/gl/EGLUtils.h
@@ +25,5 @@
>  public:
>      static EGLImageWrapper* Create(GLContext* gl, GLuint tex);
>  
>  private:
> +    GLLibraryEGL* mLibrary;

WeakPtr or RefPtr

::: gfx/gl/GLBlitHelperD3D.cpp
@@ +39,1 @@
>                                                                                stream,

Fix indents.

::: gfx/gl/GLLibraryEGL.cpp
@@ +42,5 @@
>  namespace mozilla {
>  namespace gl {
>  
>  StaticMutex GLLibraryEGL::sMutex;
> +StaticAutoPtr<GLLibraryEGL> GLLibraryEGL::sEGLLibrary;

If GLLibraryEGL isn't SupportsWeakPtr, this needs to be StaticRefPtr.

Comment 21

[Sotaro Ikeda [:sotaro]]

(In reply to Jeff Gilbert [:jgilbert] from comment #20)
> Comment on attachment 8979872 [details] [diff] [review]
> patch - Make EGLLibrary destroyable
> 
> Review of attachment 8979872 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/gl/EGLUtils.h
> @@ +25,5 @@
> >  public:
> >      static EGLImageWrapper* Create(GLContext* gl, GLuint tex);
> >  
> >  private:
> > +    GLLibraryEGL* mLibrary;
> 
> WeakPtr or RefPtr

I am going to change it to ThreadSafeWeakPtr.

> 
> ::: gfx/gl/GLBlitHelperD3D.cpp
> @@ +39,1 @@
> >                                                                                stream,
> 
> Fix indents.

I will fix in a next patch.

> 
> ::: gfx/gl/GLLibraryEGL.cpp
> @@ +42,5 @@
> >  namespace mozilla {
> >  namespace gl {
> >  
> >  StaticMutex GLLibraryEGL::sMutex;
> > +StaticAutoPtr<GLLibraryEGL> GLLibraryEGL::sEGLLibrary;
> 
> If GLLibraryEGL isn't SupportsWeakPtr, this needs to be StaticRefPtr.

I am going to use StaticRefPtr.

Comment 22

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Make EGLLibrary destroyable

Applied the comment.

Comment 23

[Sotaro Ikeda [:sotaro]]

Comment on attachment 8980196 [details] [diff] [review]
patch - Make EGLLibrary destroyable

:jgilbert, can you review the patch again? Thanks!

Comment 24

[Sotaro Ikeda [:sotaro]]

I changed my mind. I am going to use RefPtr instead of ThreadSafeWeakPtr.

Comment 25

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Make EGLLibrary destroyable

Comment 26

[Sotaro Ikeda [:sotaro]]

Comment on attachment 8980199 [details] [diff] [review]
patch - Make EGLLibrary destroyable

:jgilbert, can you review the patch again?

Comment 27

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8980199 [details] [diff] [review]
patch - Make EGLLibrary destroyable

Review of attachment 8980199 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/gl/EGLUtils.h
@@ +25,5 @@
>  public:
>      static EGLImageWrapper* Create(GLContext* gl, GLuint tex);
>  
>  private:
> +    RefPtr<GLLibraryEGL> mLibrary;

const RefPtr

Comment 28

[Sotaro Ikeda [:sotaro]]

Attachment: patch part 1 - Make EGLLibrary destroyable

Applied the comment.

Comment 29

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Add GLLibraryEGL::Shutdown()

Rebased.

Comment 30

[Sotaro Ikeda [:sotaro]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2ae1f002df217fac9b3a3bc7682adbd6e69e33f3

Hmm, there is a case that attachment 8980499 [details] [diff] [review] was detected as leaking memory. Its memory usage is same but, it changed its allocation from global to normal heap.

Comment 31

[Sotaro Ikeda [:sotaro]]

(In reply to Sotaro Ikeda [:sotaro] from comment #30)
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=2ae1f002df217fac9b3a3bc7682adbd6e69e33f3
> 
> Hmm, there is a case that attachment 8980499 [details] [diff] [review] was
> detected as leaking memory. Its memory usage is same but, it changed its
> allocation from global to normal heap.

It seems to be addressed by calling GLLibraryEGL::Shutdown() in GLContextProviderEGL::Shutdown().

Comment 32

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Add GLLibraryEGL::Shutdown()

Add calling GLLibraryEGL::Shutdown() in GLContextProviderEGL::Shutdown().

Comment 33

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8980580 [details] [diff] [review]
patch - Add GLLibraryEGL::Shutdown()

Review of attachment 8980580 [details] [diff] [review]:
-----------------------------------------------------------------

Don't we get this destroy semantic for free by just killing the context and having GL/ANGLE start generating its normal errors? I suspect it's simpler if we don't try to handle it ourselves. Contexts have to handle random context loss anyway.

::: gfx/gl/GLContextProviderEGL.cpp
@@ +322,5 @@
> +}
> +
> +void
> +GLContextEGL::Destroy()
> +{

This weird half-alive state breaks RAII rules, which is dangerous. I really don't want to have to think about this as an extra failure case. Don't bother tearing it down manually if we don't have to.

Generally speaking we want to design to prevent these zombie object states.

@@ +336,5 @@
>  #endif
>  
> +    if (mContext) {
> +        mEgl->fDestroyContext(EGL_DISPLAY(), mContext);
> +        mContext = EGL_NO_CONTEXT;

Do you have to zero this? Please just leave it const and leave it alone.

::: gfx/gl/GLLibraryEGL.cpp
@@ +376,5 @@
>  bool
>  GLLibraryEGL::DoEnsureInitialized(bool forceAccel, nsACString* const out_failureId)
>  {
> +    if (mDestroyed) {
> +        *out_failureId = NS_LITERAL_CSTRING("FEATURE_FAILURE_EGL_DESTROYED");

This should assert.

@@ +685,5 @@
>  #undef SYMBOL
>  #undef END_OF_SYMBOLS
>  
> +/* static */ void
> +GLLibraryEGL::Shutdown()

This has one caller, it's trivial, and it's confusingly named next to DoShutdown. Just remove this.

@@ +699,5 @@
> +GLLibraryEGL::DoShutdown()
> +{
> +    MutexAutoLock lock(mGLContextEGLsLock);
> +    mDestroyed = true;
> +    for (auto iter = mGLContextEGLs.begin(); iter != mGLContextEGLs.end(); iter++) {

for (auto& cur : mGLContextEGLs) {

@@ +708,5 @@
> +        mEGLDisplay = EGL_NO_DISPLAY;
> +    }
> +    mSymbols = {};
> +    if (mEGLLibrary) {
> +        PR_UnloadLibrary(mEGLLibrary);

Why do you try to unload this?

::: gfx/gl/GLLibraryEGL.h
@@ +383,5 @@
>      static bool EnsureInitialized(bool forceAccel, nsACString* const out_failureId);
>  
> +    static void Shutdown();
> +
> +    void AddGLContextEGL(GLContextEGL* aContext);

const method

@@ +524,5 @@
>      bool mIsANGLE;
>      bool mIsWARP;
> +
> +    Mutex mGLContextEGLsLock;
> +    std::unordered_set<GLContextEGL*> mGLContextEGLs;

mutable

Comment 34

[Sotaro Ikeda [:sotaro]]

(In reply to Jeff Gilbert [:jgilbert] from comment #33)
> Comment on attachment 8980580 [details] [diff] [review]
> patch - Add GLLibraryEGL::Shutdown()
> 
> Review of attachment 8980580 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Don't we get this destroy semantic for free by just killing the context and
> having GL/ANGLE start generating its normal errors? I suspect it's simpler
> if we don't try to handle it ourselves. Contexts have to handle random
> context loss anyway.
> 
> ::: gfx/gl/GLContextProviderEGL.cpp
> @@ +322,5 @@
> > +}
> > +
> > +void
> > +GLContextEGL::Destroy()
> > +{
> 
> This weird half-alive state breaks RAII rules, which is dangerous. I really
> don't want to have to think about this as an extra failure case. Don't
> bother tearing it down manually if we don't have to.
> 
> Generally speaking we want to design to prevent these zombie object states.

I am going to remove GLContextEGL::Destroy() in a next patch.

> 
> @@ +336,5 @@
> >  #endif
> >  
> > +    if (mContext) {
> > +        mEgl->fDestroyContext(EGL_DISPLAY(), mContext);
> > +        mContext = EGL_NO_CONTEXT;
> 
> Do you have to zero this? Please just leave it const and leave it alone.

I am going to update it in a next patch.

> 
> ::: gfx/gl/GLLibraryEGL.cpp
> @@ +376,5 @@
> >  bool
> >  GLLibraryEGL::DoEnsureInitialized(bool forceAccel, nsACString* const out_failureId)
> >  {
> > +    if (mDestroyed) {
> > +        *out_failureId = NS_LITERAL_CSTRING("FEATURE_FAILURE_EGL_DESTROYED");
> 
> This should assert.

I am going to add assert in a next patch.

> 
> @@ +685,5 @@
> >  #undef SYMBOL
> >  #undef END_OF_SYMBOLS
> >  
> > +/* static */ void
> > +GLLibraryEGL::Shutdown()
> 
> This has one caller, it's trivial, and it's confusingly named next to
> DoShutdown. Just remove this.

I am going to remove it.

> 
> @@ +699,5 @@
> > +GLLibraryEGL::DoShutdown()
> > +{
> > +    MutexAutoLock lock(mGLContextEGLsLock);
> > +    mDestroyed = true;
> > +    for (auto iter = mGLContextEGLs.begin(); iter != mGLContextEGLs.end(); iter++) {
> 
> for (auto& cur : mGLContextEGLs) {

GLContextEGL::Destroy() will be removed, then its iteration is not necessary anymore.

> 
> @@ +708,5 @@
> > +        mEGLDisplay = EGL_NO_DISPLAY;
> > +    }
> > +    mSymbols = {};
> > +    if (mEGLLibrary) {
> > +        PR_UnloadLibrary(mEGLLibrary);
> 
> Why do you try to unload this?

We could skip the unloading. I am going to remove it in a next patch.

> 
> ::: gfx/gl/GLLibraryEGL.h
> @@ +383,5 @@
> >      static bool EnsureInitialized(bool forceAccel, nsACString* const out_failureId);
> >  
> > +    static void Shutdown();
> > +
> > +    void AddGLContextEGL(GLContextEGL* aContext);
> 
> const method

I am going to update in a next patch.

> 
> @@ +524,5 @@
> >      bool mIsANGLE;
> >      bool mIsWARP;
> > +
> > +    Mutex mGLContextEGLsLock;
> > +    std::unordered_set<GLContextEGL*> mGLContextEGLs;
> 
> mutable

I am going to apply it.

Comment 35

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Add GLLibraryEGL::Shutdown()

Apply the comment.

Comment 36

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Add GLLibraryEGL::Shutdown()

Update nits.

Comment 37

[Sotaro Ikeda [:sotaro]]

Ah, if we remove GLContextEGL::Destroy(), we do not need GLLibraryEGL::mGLContextEGLs.

Comment 38

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Add GLLibraryEGL::Shutdown()

Removed GLLibraryEGL::mGLContextEGLs.

Comment 39

[Sotaro Ikeda [:sotaro]]

Comment on attachment 8981793 [details] [diff] [review]
patch - Add GLLibraryEGL::Shutdown()

:jgilbert, can you review the patch again?

Comment 40

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Add GLLibraryEGL::Shutdown()

Update nits.

Comment 41

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8981796 [details] [diff] [review]
patch - Add GLLibraryEGL::Shutdown()

Review of attachment 8981796 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/gl/GLContextEGL.h
@@ +96,5 @@
>      }
>  
> +    EGLContext Context() const {
> +        return mContext;
> +    }

Since mContext is const again, it can be public, and so we should drop the getter.

@@ -119,3 @@
>      EGLSurface mSurface;
>      const EGLSurface mFallbackSurface;
> -public:

Keep this public.

::: gfx/gl/GLContextProviderEGL.cpp
@@ +1196,5 @@
>  /*static*/ void
>  GLContextProviderEGL::Shutdown()
>  {
> +    if (GLLibraryEGL::Get()) {
> +        GLLibraryEGL::Get()->Shutdown();

Never call Get() twice. Grab a ref to it and use that.

::: gfx/gl/GLLibraryEGL.cpp
@@ +375,5 @@
>  
>  bool
>  GLLibraryEGL::DoEnsureInitialized(bool forceAccel, nsACString* const out_failureId)
>  {
> +    if (mDestroyed) {

You can actually know that this is destroyed by using `mInitialized && !mSymbols.fTerminate`. It'd be one less state variable, which is usually good!

Comment 42

[Kelsey Gilbert [:jgilbert]]

Thank you for your patience with all the reviews!

Comment 43

[Sotaro Ikeda [:sotaro]]

Attachment: patch part 2 - Add GLLibraryEGL::Shutdown()

Apply the comment.

Comment 44

[Sotaro Ikeda [:sotaro]]

Attachment: patch part 1 - Make EGLLibrary destroyable

Rebased.

Comment 45

[Sotaro Ikeda [:sotaro]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=277369493093fce85b47180a0718c98d4773aab5

Comment 46

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/db10936e9f8d
Make EGLLibrary destroyable r=jgilbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/886f2a6dc76d
Add GLLibraryEGL::Shutdown() r=jgilbert

Comment 47

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/db10936e9f8d
https://hg.mozilla.org/mozilla-central/rev/886f2a6dc76d