Closed Bug 900188 Opened 6 years ago Closed 6 years ago

Correctly treat WebGL as being single buffered

Categories

(Core :: Graphics: Layers, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla25

People

(Reporter: mattwoodrow, Unassigned)

References

Details

Attachments

(1 file)

No description provided.
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.
Attachment #783975 - Flags: review?(ncameron)
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
Attachment #783975 - Flags: review?(ncameron) → review+
(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.
(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.
https://hg.mozilla.org/mozilla-central/rev/4aec4fb6cb96
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
Depends on: 904620
You need to log in before you can comment on or make changes to this bug.