1380843 - Add gfx preference so GLScreenBuffer may be toggled between SurfaceFactory_EGLImage and SurfaceFactory_SurfaceTexture

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Toolbar, enhancement, P3)

Description

[Randall Barker [:rbarker]]

Currently, GLScreenBuffer on Android only uses SurfaceFactory_SurfaceTexture when e10s is enabled. The preference will allow SurfaceFactory_SurfaceTexture to be used even when e10s is not enabled so that it may be tested in Fennec.

Comment 1

[Randall Barker [:rbarker]]

Attachment: Bug 1380843 - Add gfx preference so GLScreenBuffer may be toggled between SurfaceFactory_EGLImage and SurfaceFactory_SurfaceTexture

Review commit: https://reviewboard.mozilla.org/r/157212/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/157212/

Comment 2

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8886416 [details]
Bug 1380843 - Add gfx preference so GLScreenBuffer may be toggled between SurfaceFactory_EGLImage and SurfaceFactory_SurfaceTexture

https://reviewboard.mozilla.org/r/157212/#review163284

::: gfx/gl/GLScreenBuffer.cpp:94
(Diff revision 1)
>                  if (sGLXLibrary.UseTextureFromPixmap())
>                    factory = SurfaceFactory_GLXDrawable::Create(gl, caps, ipcChannel, flags);
>  #elif defined(MOZ_WIDGET_UIKIT)
>                  factory = MakeUnique<SurfaceFactory_GLTexture>(mGLContext, caps, ipcChannel, mFlags);
>  #elif defined(MOZ_WIDGET_ANDROID)
> -                if (XRE_IsParentProcess()) {
> +                if (XRE_IsParentProcess() && !gfxPrefs::WebGLSurfaceTextureEnabled()) {

Shouldn't this change to:
if (SurfTexEnabled())
  factory = Fact_SurfTex
else if (IsParentProcess())
  factory = Fact_EGLImage
  
Otherwise enable-surface-texture/WebGLSurfaceTextureEnabled are misleading.

::: gfx/thebes/gfxPrefs.h:729
(Diff revision 1)
>    DECL_GFX_PREF(Live, "webgl.allow-fb-invalidation",           WebGLFBInvalidation, bool, false);
>  
>    DECL_GFX_PREF(Live, "webgl.perf.max-warnings",                    WebGLMaxPerfWarnings, int32_t, 0);
>    DECL_GFX_PREF(Live, "webgl.perf.max-acceptable-fb-status-invals", WebGLMaxAcceptableFBStatusInvals, int32_t, 0);
>  
> +

You added a newline here on accident.

Comment 3

[Randall Barker [:rbarker]]

Comment on attachment 8886416 [details]
Bug 1380843 - Add gfx preference so GLScreenBuffer may be toggled between SurfaceFactory_EGLImage and SurfaceFactory_SurfaceTexture

https://reviewboard.mozilla.org/r/157212/#review163906

::: gfx/gl/GLScreenBuffer.cpp:94
(Diff revision 1)
>                  if (sGLXLibrary.UseTextureFromPixmap())
>                    factory = SurfaceFactory_GLXDrawable::Create(gl, caps, ipcChannel, flags);
>  #elif defined(MOZ_WIDGET_UIKIT)
>                  factory = MakeUnique<SurfaceFactory_GLTexture>(mGLContext, caps, ipcChannel, mFlags);
>  #elif defined(MOZ_WIDGET_ANDROID)
> -                if (XRE_IsParentProcess()) {
> +                if (XRE_IsParentProcess() && !gfxPrefs::WebGLSurfaceTextureEnabled()) {

Yes, that's exactly what it should be. I'm on PTO right now but will try and update the patch when I get a moment.

Comment 4

[Randall Barker [:rbarker]]

(In reply to Jeff Gilbert [:jgilbert] from comment #2)
> Comment on attachment 8886416 [details]
> Bug 1380843 - Add gfx preference so GLScreenBuffer may be toggled between
> SurfaceFactory_EGLImage and SurfaceFactory_SurfaceTexture
> 
> https://reviewboard.mozilla.org/r/157212/#review163284
> 
> ::: gfx/gl/GLScreenBuffer.cpp:94
> (Diff revision 1)
> >                  if (sGLXLibrary.UseTextureFromPixmap())
> >                    factory = SurfaceFactory_GLXDrawable::Create(gl, caps, ipcChannel, flags);
> >  #elif defined(MOZ_WIDGET_UIKIT)
> >                  factory = MakeUnique<SurfaceFactory_GLTexture>(mGLContext, caps, ipcChannel, mFlags);
> >  #elif defined(MOZ_WIDGET_ANDROID)
> > -                if (XRE_IsParentProcess()) {
> > +                if (XRE_IsParentProcess() && !gfxPrefs::WebGLSurfaceTextureEnabled()) {
> 
> Shouldn't this change to:
> if (SurfTexEnabled())
>   factory = Fact_SurfTex
> else if (IsParentProcess())
>   factory = Fact_EGLImage
>   
> Otherwise enable-surface-texture/WebGLSurfaceTextureEnabled are misleading.
> 

Actually, on second thought, I think what I have is correct. For Fennec and non-e10s geckoview we want to run with SurfaceFactory_GLTexture because SurfaceFactory_SurfaceTextures will assert some times when switching tabs. However with e10s geckoview, GLTexture will not work and we have to use SurfaceTextures. That was how it was working. But with WebVR in non-e10s GeckoView I need to use SurfaceTextures as well. So I added the gfxPref so that I could switch it on for that case only. I believe that is what my patch does. Eventually I think we just want to use SurfaceTextures but there are still a few issues that need to be fixed before switching everything over.

Comment 5

[Kartikaya Gupta (email:[e-mail])]

jgilbert, review ping (see comment 4)

Comment 6

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8886416 [details]
Bug 1380843 - Add gfx preference so GLScreenBuffer may be toggled between SurfaceFactory_EGLImage and SurfaceFactory_SurfaceTexture

https://reviewboard.mozilla.org/r/157212/#review168582

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2e6dbfb4f07a
Add gfx preference so GLScreenBuffer may be toggled between SurfaceFactory_EGLImage and SurfaceFactory_SurfaceTexture r=jgilbert

Comment 8

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/2e6dbfb4f07a