Closed Bug 1451183 Opened Last year Closed Last year

Add capability of resetting GLLibraryEGL state

Categories

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

enhancement

Tracking

()

RESOLVED FIXED

People

(Reporter: sotaro, Assigned: sotaro)

References

Details

(Whiteboard: [keep-open])

Attachments

(2 files, 17 obsolete files)

11.96 KB, patch
sotaro
: review+
Details | Diff | Splinter Review
54.58 KB, patch
sotaro
: review+
Details | Diff | Splinter Review
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.
Blocks: 1364504
Assignee: nobody → sotaro.ikeda.g
Attachment #8964800 - Flags: review?(jgilbert)
Attachment #8964800 - Flags: review?(jgilbert)
Attachment #8964803 - Flags: review?(jgilbert)
Attachment #8964803 - Flags: review?(jgilbert) → review+
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?
Attachment #8964803 - Flags: review+ → review?(jgilbert)
In current implementation, GL context existence is checked by client side by Bug 1364504. What is a better way to check it?
I am going to update the patch to check if GL context does not exist.
Attachment #8965624 - Flags: review?(jgilbert)
: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?
Flags: needinfo?(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 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.
Attachment #8965624 - Flags: review?(jgilbert)
Flags: needinfo?(jgilbert)
Attached patch patch - GLLibraryEGL::Shutdown() (obsolete) — Splinter Review
Applied the comment.
Attachment #8965624 - Attachment is obsolete: true
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 on attachment 8973927 [details] [diff] [review]
patch - GLLibraryEGL::Shutdown()

:jgilbert, can you review the patch again?
Attachment #8973927 - Flags: review?(jgilbert)
:jgilbert, can you review the patch?
Flags: needinfo?(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.
Attachment #8973927 - Flags: review?(jgilbert) → review-
(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.
Attached patch patch - GLLibraryEGL::Shutdown() (obsolete) — Splinter Review
One comment could not be applied since sEGLLibrary is defined as global object.
Is there a way to destroy global object?
Attachment #8973927 - Attachment is obsolete: true
Attached patch patch - GLLibraryEGL::Shutdown() (obsolete) — Splinter Review
Update nits.
Attachment #8979475 - Attachment is obsolete: true
Attachment #8979478 - Flags: review?(jgilbert)
Attachment #8979478 - Flags: review?(jgilbert)
Attachment #8979872 - Flags: review?(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.
Attachment #8979872 - Flags: review?(jgilbert) → review-
Flags: needinfo?(jgilbert)
(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.
Applied the comment.
Attachment #8979872 - Attachment is obsolete: true
Comment on attachment 8980196 [details] [diff] [review]
patch - Make EGLLibrary destroyable

:jgilbert, can you review the patch again? Thanks!
Attachment #8980196 - Flags: review?(jgilbert)
Attachment #8980196 - Flags: review?(jgilbert)
I changed my mind. I am going to use RefPtr instead of ThreadSafeWeakPtr.
Attachment #8980196 - Attachment is obsolete: true
Comment on attachment 8980199 [details] [diff] [review]
patch - Make EGLLibrary destroyable

:jgilbert, can you review the patch again?
Attachment #8980199 - Flags: review?(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
Attachment #8980199 - Flags: review?(jgilbert) → review+
Applied the comment.
Attachment #8980199 - Attachment is obsolete: true
Attachment #8980499 - Flags: review+
Whiteboard: [keep-open]
Rebased.
Attachment #8979478 - Attachment is obsolete: true
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.
Attachment #8980518 - Attachment description: patch - GLLibraryEGL::Shutdown() → patch - Add GLLibraryEGL::Shutdown()
(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().
Add calling GLLibraryEGL::Shutdown() in GLContextProviderEGL::Shutdown().
Attachment #8980518 - Attachment is obsolete: true
Attachment #8980580 - Flags: review?(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
Attachment #8980580 - Flags: review?(jgilbert) → review-
(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.
Apply the comment.
Attachment #8980580 - Attachment is obsolete: true
Update nits.
Attachment #8981782 - Attachment is obsolete: true
Ah, if we remove GLContextEGL::Destroy(), we do not need GLLibraryEGL::mGLContextEGLs.
Removed GLLibraryEGL::mGLContextEGLs.
Attachment #8981791 - Attachment is obsolete: true
Comment on attachment 8981793 [details] [diff] [review]
patch - Add GLLibraryEGL::Shutdown()

:jgilbert, can you review the patch again?
Attachment #8981793 - Flags: review?(jgilbert)
Update nits.
Attachment #8981793 - Attachment is obsolete: true
Attachment #8981793 - Flags: review?(jgilbert)
Attachment #8981796 - Flags: review?(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!
Attachment #8981796 - Flags: review?(jgilbert) → review+
Thank you for your patience with all the reviews!
Apply the comment.
Attachment #8981796 - Attachment is obsolete: true
Attachment #8982854 - Flags: review+
Attachment #8980499 - Attachment description: patch - Make EGLLibrary destroyable → patch part 1 - Make EGLLibrary destroyable
Attachment #8982854 - Attachment description: patch - Add GLLibraryEGL::Shutdown() → patch part 2 - Add GLLibraryEGL::Shutdown()
Rebased.
Attachment #8980499 - Attachment is obsolete: true
Attachment #8982858 - Flags: review+
Status: NEW → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Depends on: 1466778
You need to log in before you can comment on or make changes to this bug.