1250840 - Redefinition of "class mozilla::gl::GLContextProvider"

Status: RESOLVED WONTFIX

(Core :: Graphics, defect)

Description

[Tommy Kuo (away forever...)]

I think it should be wrapped by #define something in [1]. 

When we define "MOZ_GL_PROVIDER" to "GLContextProviderEGL" in [2]. It causes the redefinition error of "class GL_CONTEXT_PROVIDER_NAME" in GLContextProviderImpl.h.

[1]: https://dxr.mozilla.org/mozilla-central/source/gfx/gl/GLContextProvider.h#49-54
[2]: https://dxr.mozilla.org/mozilla-central/source/gfx/gl/GLContextProvider.h#65-70

Comment 1

[Tommy Kuo (away forever...)]

Attachment: Bug-1250840-hg.patch

Hi Jerry, can you give some feedback on this patch? Or, if you think there is someone suitable for feedback, can you forward it to him/her?

I found the define warp is removed in Bug 1191042. So I think we should add the wrap back. And I also add a define wrap for "GL_CONTEXT_PROVIDER_DEFAULT" in [1].

[1]: https://dxr.mozilla.org/mozilla-central/source/gfx/gl/GLContextProvider.h#69

Comment 2

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Comment on attachment 8722943 [details] [diff] [review]
Bug-1250840-hg.patch

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

::: gfx/gl/GLContextProvider.h
@@ +47,5 @@
>  #endif
>  
> +#if defined(ANDROID) || defined(XP_WIN)
> +  #define GL_CONTEXT_PROVIDER_NAME GLContextProviderEGL
> +  #include "GLContextProviderImpl.h"

If this is android platform and we define the MOZ_GL_PROVIDER as GLContextProviderEGL, we still create two class named GLContextProviderEGL.

Comment 3

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: handle the redefinition of gl context provider.

Comment 4

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Comment on attachment 8722983 [details] [diff] [review]
handle the redefinition of gl context provider.

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

There is a redefinition problem when we set MOZ_GL_PROVIDER as the same type as the provider we already have.
I assume we only have one provider. If we already have one, we will just ignore others.
Then, make MOZ_GL_PROVIDER as the first priority.

Comment 5

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 8722983 [details] [diff] [review]
handle the redefinition of gl context provider.

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

It looks like this stops us from defining both providers on X11. Is that intentional?

Comment 6

[Jerry Shih[:jerry] (UTC+8) (inactive)]

(In reply to Jeff Muizelaar [:jrmuizel] from comment #5)
> Comment on attachment 8722983 [details] [diff] [review]
> handle the redefinition of gl context provider.
> 
> Review of attachment 8722983 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> It looks like this stops us from defining both providers on X11. Is that
> intentional?

Yes, I assume we only have one provider.
Will we have multiple providers on X11(not including the default GLContextProviderNull provider)?

Comment 7

[Jeff Muizelaar [:jrmuizel]]

(In reply to Jerry Shih[:jerry] (UTC+8) from comment #6)
> (In reply to Jeff Muizelaar [:jrmuizel] from comment #5)
> > Comment on attachment 8722983 [details] [diff] [review]
> > handle the redefinition of gl context provider.
> > 
> > Review of attachment 8722983 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > It looks like this stops us from defining both providers on X11. Is that
> > intentional?
> 
> Yes, I assume we only have one provider.
> Will we have multiple providers on X11(not including the default
> GLContextProviderNull provider)?

I think we do have multiple providers on X11...

Comment 8

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: handle the redefinition of gl context provider. v2

Export current provider def in configure.in and check it to prevent the redefinition problem.

Comment 9

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Comment on attachment 8726645 [details] [diff] [review]
handle the redefinition of gl context provider. v2

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

::: gfx/gl/GLContextProvider.h
@@ +36,2 @@
>  #ifdef XP_WIN
> +  #ifndef MOZ_GLContextProviderWGL

Please check old-configure.in

Comment 10

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: handle the redefinition of gl context provider. v3

rebase.

Comment 11

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Comment on attachment 8732868 [details] [diff] [review]
handle the redefinition of gl context provider. v3

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

::: gfx/gl/GLContextProvider.h
@@ +34,2 @@
>  #ifdef XP_WIN
> +  #ifndef GL_PROVIDER_WGL

Use the def from [1] to prevent the redefinition problem when we set the provider as that we already have.

[1]
https://hg.mozilla.org/mozilla-central/annotate/f14898695ee0dd14615914f3e1401f17df57fdd7/toolkit/moz.configure#l180

Comment 12

[Kelsey Gilbert [:jgilbert]]

(In reply to Tommy Kuo [:KuoE0] from comment #0)
> I think it should be wrapped by #define something in [1]. 
> 
> When we define "MOZ_GL_PROVIDER" to "GLContextProviderEGL" in [2]. It causes
> the redefinition error of "class GL_CONTEXT_PROVIDER_NAME" in
> GLContextProviderImpl.h.

Why do you want to do this? It is preferable to not 'fix' anything here if there's no good reason to want to do this.

Comment 13

[Jerry Shih[:jerry] (UTC+8) (inactive)]

(In reply to Jeff Gilbert [:jgilbert] from comment #12)
> (In reply to Tommy Kuo [:KuoE0] from comment #0)
> > I think it should be wrapped by #define something in [1]. 
> > 
> > When we define "MOZ_GL_PROVIDER" to "GLContextProviderEGL" in [2]. It causes
> > the redefinition error of "class GL_CONTEXT_PROVIDER_NAME" in
> > GLContextProviderImpl.h.
> 
> Why do you want to do this? It is preferable to not 'fix' anything here if
> there's no good reason to want to do this.

If we have ac_option --with-gl-provider=EGL, we will hit the problem. I think it's fine to add the option even we already use that.

Comment 14

[Kelsey Gilbert [:jgilbert]]

I do not think that's useful enough to warrant the extra complexity going into this already-gross file.

Unless there's a reason to have this, I'm going to WONTFIX this with the solution of "don't do that then".

Please reopen if there is a better usecase.

Comment 15

[Tommy Kuo (away forever...)]

(In reply to Jeff Gilbert [:jgilbert] from comment #12)
> (In reply to Tommy Kuo [:KuoE0] from comment #0)
> > I think it should be wrapped by #define something in [1]. 
> > 
> > When we define "MOZ_GL_PROVIDER" to "GLContextProviderEGL" in [2]. It causes
> > the redefinition error of "class GL_CONTEXT_PROVIDER_NAME" in
> > GLContextProviderImpl.h.
> 
> Why do you want to do this? It is preferable to not 'fix' anything here if
> there's no good reason to want to do this.

Hi jgibert, the problem only happens on partner's build. And I think they already have workaround on it. If it is not an issue for us, I think WONTFIX is okay.

Comment 16

[Kelsey Gilbert [:jgilbert]]

(In reply to Tommy Kuo [:KuoE0] from comment #15)
> (In reply to Jeff Gilbert [:jgilbert] from comment #12)
> > (In reply to Tommy Kuo [:KuoE0] from comment #0)
> > > I think it should be wrapped by #define something in [1]. 
> > > 
> > > When we define "MOZ_GL_PROVIDER" to "GLContextProviderEGL" in [2]. It causes
> > > the redefinition error of "class GL_CONTEXT_PROVIDER_NAME" in
> > > GLContextProviderImpl.h.
> > 
> > Why do you want to do this? It is preferable to not 'fix' anything here if
> > there's no good reason to want to do this.
> 
> Hi jgibert, the problem only happens on partner's build. And I think they
> already have workaround on it. If it is not an issue for us, I think WONTFIX
> is okay.

Great, thanks!