Closed Bug 919987 Opened 6 years ago Closed 6 years ago

Fix the mCurSurface vs mSurface situation in GLContextEGL

Categories

(Core :: Graphics, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla28

People

(Reporter: bjacob, Assigned: bjacob)

References

Details

(Whiteboard: [qa-])

Attachments

(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)
Sure.
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+
https://hg.mozilla.org/integration/b2g-inbound/rev/6a83dc3f7886

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.