Closed Bug 1370544 Opened 7 years ago Closed 7 years ago

[geckoview] No depth buffer in WebGL with e10s

Categories

(GeckoView :: Sandboxing, enhancement)

Product:
GeckoView
Component:
Sandboxing
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

(firefox57 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla57
Tracking Flags:
Tracking Status
firefox57 --- fixed

People

(Reporter: snorp, Assigned: rbarker)

Details

Attachments

(1 file, 1 obsolete file)

Bug 1370544 - Ensure EGLSurface is created with the same EGLConfig as the context
59 bytes, text/x-review-board-request
jgilbert
: review+
Details 
Since we're using the real FBO 0 now and the EGLContext we create does not have depth specified.
Assignee: nobody → rbarker
Attachment #8875010 - Attachment is obsolete: true
Component: Embedding: APIs → GeckoView
Product: Core → Firefox for Android
Version: unspecified → Trunk
Comment on attachment 8875092 [details]
Bug 1370544 - Ensure EGLSurface is created with the same EGLConfig as the context

https://reviewboard.mozilla.org/r/146466/#review151048

::: gfx/gl/GLContextEGL.h:138
(Diff revision 2)
>      static EGLSurface CreatePBufferSurfaceTryingPowerOfTwo(EGLConfig config,
>                                                             EGLenum bindToTextureFormat,
>                                                             gfx::IntSize& pbsize);
> +#if defined(MOZ_WIDGET_ANDROID)
> +public:
> +    EGLSurface CreateEGLSurface(void* aWindow);

CreateCompatibleSurface

::: gfx/gl/GLContextEGL.h:139
(Diff revision 2)
>                                                             EGLenum bindToTextureFormat,
>                                                             gfx::IntSize& pbsize);
> +#if defined(MOZ_WIDGET_ANDROID)
> +public:
> +    EGLSurface CreateEGLSurface(void* aWindow);
> +    static void DestroyEGLSurface(EGLSurface aSurface);

Leave this where it was.

::: gfx/gl/GLContextProviderEGL.cpp:764
(Diff revision 2)
>      MOZ_ASSERT(aWidget);
>      return GLContextEGLFactory::Create(GET_NATIVE_WINDOW_FROM_REAL_WIDGET(aWidget),
>                                         aWebRender);
>  }
>  
> -#if defined(ANDROID)
> +#if defined(MOZ_WIDGET_ANDROID)

Just remove the ifdef, since it doesn't look like we need it.

::: gfx/gl/GLContextProviderEGL.cpp:970
(Diff revision 2)
> +    // Using a headless context loses the SurfaceCaps
> +    // which can cause a loss of depth and/or stencil
> +    bool canOffscreenUseHeadless = false;

Don't duplicate the declaration across #ifs.
Move this below the IsANGLE block.

::: gfx/gl/GLContextProviderEGL.cpp:997
(Diff revision 2)
> -        gl = GLContextEGL::CreateEGLPBufferOffscreenContext(flags, size,
> +#if defined(MOZ_WIDGET_ANDROID)
> +        mozilla::gfx::IntSize pbsize(1, 1);
> +#else

What are you doing here?

::: gfx/gl/GLContextProviderImpl.h
(Diff revision 2)
>       */
>      static already_AddRefed<GLContext>
>      CreateWrappingExisting(void* aContext, void* aSurface);
>  
> -#if defined(ANDROID)
> -    static EGLSurface CreateEGLSurface(void* aWindow);

Give this an optional config param.
Attachment #8875092 - Flags: review?(jgilbert) → review-
Comment on attachment 8875092 [details]
Bug 1370544 - Ensure EGLSurface is created with the same EGLConfig as the context

https://reviewboard.mozilla.org/r/146466/#review151412

::: gfx/gl/GLContextEGL.h:139
(Diff revision 2)
>                                                             EGLenum bindToTextureFormat,
>                                                             gfx::IntSize& pbsize);
> +#if defined(MOZ_WIDGET_ANDROID)
> +public:
> +    EGLSurface CreateEGLSurface(void* aWindow);
> +    static void DestroyEGLSurface(EGLSurface aSurface);

I don't understand why these functions should be left in GLContextProviderEGL. They were Android specific to begin with (I added them originally). With this patch there would be nothing calling them. With nothing calling them they will rot. Additionally putting the DestroyEGLSurface back in the provider would require including both GLContextEGL.h and GLContextProviderEGL.h instead of just GLContextEGL.h. Finally, I'm not sure what the use case is for creating a surface that has a different EGLConfig from the context. This caused a significant amount of pain for both snorp and myself getting the surface to work correctly for features we are both working on. Leaving the static create function with an optional EGLConfig param seems to be asking for future problems.

::: gfx/gl/GLContextProviderEGL.cpp:997
(Diff revision 2)
> -        gl = GLContextEGL::CreateEGLPBufferOffscreenContext(flags, size,
> +#if defined(MOZ_WIDGET_ANDROID)
> +        mozilla::gfx::IntSize pbsize(1, 1);
> +#else

The pbuffer is only used to make the context current on creation. Previously, the code would always call CreateHeadlessContext which would hardcode the size to 16x16. Since the size isn't actually used on android, I'm just hardcoding it to 1x1 to prevent any one from creating a screen sized pbuffer and wasting memory on android. I #ifdef'd it so that other platforms will continue to work the way they did before the patch.
Comment on attachment 8875092 [details]
Bug 1370544 - Ensure EGLSurface is created with the same EGLConfig as the context

https://reviewboard.mozilla.org/r/146466/#review168580
Attachment #8875092 - Flags: review?(jgilbert) → review+
Pushed by rbarker@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/599b64980e2d
Ensure EGLSurface is created with the same EGLConfig as the context r=jgilbert
https://hg.mozilla.org/mozilla-central/rev/599b64980e2d
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox57: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
Product: Firefox for Android → GeckoView
Target Milestone: Firefox 57 → mozilla57

Moving some e10s bugs to the new GeckoView::Sandboxing component.

Component: General → Sandboxing
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: