Closed Bug 919987 Opened 6 years ago Closed 6 years ago

Fix the mCurSurface vs mSurface situation in GLContextEGL


(Core :: Graphics, defect)

Not set





(Reporter: bjacob, Assigned: bjacob)



(Whiteboard: [qa-])


(1 file)

In GLContextEGL, mSurface is the EGLSurface we're rendering to, typically on Android where we have to get it renewed by RenewSurface when e.g. device orientation changes or we get re-focused.

Bug 716859 added another EGLSurface member, mCurSurface. It changed MakeCurrentImpl to use mCurSurface instead of mSurface, but RenewSurface still renews nSurface, not mCurSurface. So in fact, I have no idea how we are not broken at the moment. The only thing that I can see, that syncs the two surfaces, is MakeCurrent_EGLSurface, but that only seems to be called in SharedSurface_ANGLE.

Bug 900020 comment 69 says:

> We have to be able to switch out backing EGLSurfaces for the ShSurfANGLE
> path. (And later for the EGLStream path)
> I wrote that assuming that except in this case, the surface never changes.
> Just have RenewSurface update mCurSurface (since we MakeCurrent against
> mCurSurface) and everything should work. We should assert that
> GLContext->Screen() is null when this happens, though.

And bug 900020 comment 72 says:

> For the long term, unless there's a reason to keep mSurface and mCurSurface
> separate, we should merge them.
> If they need to be kept separate, we should formalize the difference between
> mBackingSurface (the surface considered to be the normal default
> framebuffer) and mCurSurface. (which is basically just an override)

Jeff, can you drive this?
Flags: needinfo?(jgilbert)
Assignee: nobody → jgilbert
Flags: needinfo?(jgilbert)
Blocks: 925608
Comment on attachment 830280 [details] [diff] [review]
Replace mCurSurface, which was redundant state, by a saner mSurfaceOverride that does nothing when it's EGL_NO_SURFACE

Review of attachment 830280 [details] [diff] [review]:

::: gfx/gl/GLContext.h
@@ +2458,5 @@
> +     *
> +     * If surf is null, this removes any previously set override, and makes the
> +     * context current again against its primary surface.
> +     */
> +    virtual void SetEGLSurfaceOverride(void* surf) {

Let's make this take `EGLSurface surf` instead of a void*. We have the EGL types in GLTypes.h now, so we don't have to skirt around using them.
Attachment #830280 - Flags: review?(jgilbert) → review+

Ouch, forgot to apply your review comment, will get back to it.
Assignee: jgilbert → bjacob
Target Milestone: --- → mozilla28
Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.