900188 - Correctly treat WebGL as being single buffered

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Comment 1

[Matt Woodrow (:mattwoodrow)]

Attachment: single-buffer-webgl

We're not, and never were actually double buffered.

Ownership of the buffers is handled by the SurfaceStream, and we were passing the front buffer back and ignoring it.

This should be cleaner and easier to follow.

Comment 2

[Nick Cameron [:nrc]]

Comment on attachment 783975 [details] [diff] [review]
single-buffer-webgl

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

lgtm, presumably we often are double buffered but that is handled by the surface stream stuff rather than by layers?

::: gfx/layers/client/CanvasClient.cpp
@@ +127,5 @@
> +    SurfaceDescriptor *desc = mDeprecatedTextureClient->GetDescriptor();
> +    if (desc->type() != SurfaceDescriptor::TSurfaceStreamDescriptor ||
> +        desc->get_SurfaceStreamDescriptor().handle() != handle) {
> +      *desc = SurfaceStreamDescriptor(handle, false);
> +    

whitespace

::: gfx/layers/client/CanvasClient.h
@@ +72,5 @@
>  };
>  
>  // Used for GL canvases where we don't need to do any readback, i.e., with a
>  // GL backend.
> +class CanvasClientGL : public CanvasClient

Why change the name? I prefer CanvasClientWebGL as it means less chance of confusion with the OGL backend.

@@ +80,1 @@
>                      TextureFlags aFlags);

If you are going to change the name, please fix the indentation here and elsewhere.

::: gfx/layers/opengl/TextureHostOGL.h
@@ +732,5 @@
>    GLenum mTextureTarget;
>    GLuint mUploadTexture;
>    GLenum mWrapMode;
>    nsRefPtr<GLContext> mStreamGL;
> +  gfx::SurfaceStream *mStream; 

whitespace

Comment 3

[Matt Woodrow (:mattwoodrow)]

(In reply to Nick Cameron [:nrc] from comment #2)
> Comment on attachment 783975 [details] [diff] [review]
> single-buffer-webgl
> 
> Review of attachment 783975 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> lgtm, presumably we often are double buffered but that is handled by the
> surface stream stuff rather than by layers?

Right. As far as layers are concerned it's single buffered. It's probably triple-buffered from the surface stream's point of view.

> >  
> >  // Used for GL canvases where we don't need to do any readback, i.e., with a
> >  // GL backend.
> > +class CanvasClientGL : public CanvasClient
> 
> Why change the name? I prefer CanvasClientWebGL as it means less chance of
> confusion with the OGL backend.

Because it's not specific to WebGL, SkiaGL 2D canvas uses this too. I can do CanvasClientGLContext if you'd prefer. Or CanvasClientSurfaceStream.

Comment 4

[Nick Cameron [:nrc]]

(In reply to Matt Woodrow (:mattwoodrow) from comment #3)
> (In reply to Nick Cameron [:nrc] from comment #2)
> > Comment on attachment 783975 [details] [diff] [review]
> > single-buffer-webgl
> > 
> > Review of attachment 783975 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > lgtm, presumably we often are double buffered but that is handled by the
> > surface stream stuff rather than by layers?
> 
> Right. As far as layers are concerned it's single buffered. It's probably
> triple-buffered from the surface stream's point of view.
> 
> > >  
> > >  // Used for GL canvases where we don't need to do any readback, i.e., with a
> > >  // GL backend.
> > > +class CanvasClientGL : public CanvasClient
> > 
> > Why change the name? I prefer CanvasClientWebGL as it means less chance of
> > confusion with the OGL backend.
> 
> Because it's not specific to WebGL, SkiaGL 2D canvas uses this too. I can do
> CanvasClientGLContext if you'd prefer. Or CanvasClientSurfaceStream.

OK, makes sense. I think I prefer CanvasClientSurfaceStream.

Comment 5

[Ryan VanderMeulen [:RyanVM]]

Backed out for suspicion of causing OSX asserts.
https://hg.mozilla.org/integration/mozilla-inbound/rev/f38eafe79c95

https://tbpl.mozilla.org/php/getParsedLog.php?id=26056202&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=26056888&tree=Mozilla-Inbound

Comment 6

[Matt Woodrow (:mattwoodrow)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4aec4fb6cb96

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/4aec4fb6cb96