1048666 - WebGL2 - Implement clearbuffer

Status: RESOLVED FIXED

(Core :: Graphics: CanvasWebGL, defect)

Description

[u480271]

Implement ClearBuffer family of functions for WebGL2.

Comment 1

[u480271]

Attachment: WebGL2 - Implement ClearBufferXXX APIs

Comment 2

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8467498 [details] [diff] [review]
WebGL2 - Implement ClearBufferXXX APIs

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

::: dom/canvas/WebGL2ContextMRTs.cpp
@@ +24,5 @@
> +    case LOCAL_GL_STENCIL:
> +        return 1;
> +
> +    default:
> +        return 0;

MOZ_CRASH

@@ +95,5 @@
> +    ClearBufferuiv_base(buffer, drawbuffer, value.Elements());
> +}
> +
> +void
> +WebGL2Context::ClearBufferfv_base(GLenum buffer, GLint drawbuffer, const GLfloat* value)

I'd rather you stick these _base functions next to eachother.

::: gfx/gl/GLContext.h
@@ +879,5 @@
>          AfterGLDrawCall();
>      }
>  
> +    void fClearBufferfi(GLenum buffer, GLint drawbuffer, GLfloat depth, GLint stencil)
> +    {

Inlined member funcs should keep the { on the same line, unless decl is split across multiple lines.

::: gfx/gl/GLContextSymbols.h
@@ +66,5 @@
>      typedef void (GLAPIENTRY * PFNGLCLEARPROC) (GLbitfield);
>      PFNGLCLEARPROC fClear;
> +    typedef void (GLAPIENTRY * PFNGLCLEARBUFFERFIPROC) (GLenum buffer, GLint drawbuffer, GLfloat depth, GLint stencil);
> +    PFNGLCLEARBUFFERFIPROC fClearBufferfi;
> +    typedef void (GLAPIENTRY * PFNGLCLEARBUFFERFVPROC) (GLenum buffer, GLint drawbuffer, const GLfloat *value);

Star to left, even if the GL headers have it to the right.

Comment 3

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8467498 [details] [diff] [review]
WebGL2 - Implement ClearBufferXXX APIs

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

::: gfx/gl/GLContextFeatures.cpp
@@ +74,5 @@
>      {
> +        "clear_buffers",
> +        300, // OpenGL version
> +        300, // OpenGL ES version
> +        {

You forgot `GLContext::Extension_None,` here.

Comment 4

[u480271]

(In reply to Jeff Gilbert [:jgilbert] from comment #3)
> Comment on attachment 8467498 [details] [diff] [review]
> WebGL2 - Implement ClearBufferXXX APIs
> 
> Review of attachment 8467498 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/gl/GLContextFeatures.cpp
> @@ +74,5 @@
> >      {
> > +        "clear_buffers",
> > +        300, // OpenGL version
> > +        300, // OpenGL ES version
> > +        {
> 
> You forgot `GLContext::Extension_None,` here.

Strange. It's there in the change on my local disk.

Comment 5

[u480271]

(In reply to Jeff Gilbert [:jgilbert] from comment #2)
> Comment on attachment 8467498 [details] [diff] [review]
> WebGL2 - Implement ClearBufferXXX APIs
> 
> Review of attachment 8467498 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/canvas/WebGL2ContextMRTs.cpp
> @@ +24,5 @@
> MOZ_CRASH

Scary, but done. ;-)
 
> @@ +95,5 @@
> I'd rather you stick these _base functions next to eachother.

Done.

> ::: gfx/gl/GLContext.h
> Inlined member funcs should keep the { on the same line, unless decl is
> split across multiple lines.

Done.
 
> ::: gfx/gl/GLContextSymbols.h
> Star to left, even if the GL headers have it to the right.

Done.

Comment 6

[u480271]

Attachment: Review Comments 1

Comment 7

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8469720 [details] [diff] [review]
Review Comments 1

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

Cool, not a bad way to handle minor review updates.

Comment 8

[u480271]

https://hg.mozilla.org/users/dglastonbury_mozilla.com/mozilla-central/rev/3fdf50d41975
https://hg.mozilla.org/users/dglastonbury_mozilla.com/mozilla-central/rev/c1b4ad28ad33

Comment 9

[u480271]

https://hg.mozilla.org/users/dglastonbury_mozilla.com/mozilla-central/rev/fd773ecc59cf
https://hg.mozilla.org/users/dglastonbury_mozilla.com/mozilla-central/rev/1577ccf078df

Comment 10

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: clearbuffer-fixes

I looked at this patch and the spec, and it's almost good to land but I found some issues which are fixed by this patch.
 - don't MOZ_CRASH on bad inputs
 - validate the buffer parameter also in clearBufferfi
 - validate the GLint drawbuffer parameter
 - share identical validation code between different entry points
 - get a bit closer to Gecko coding style in switch statement even if we're 4-space indenting

Comment 11

[u480271]

Comment on attachment 8491624 [details] [diff] [review]
clearbuffer-fixes

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

I like it.

Comment 12

[u480271]

https://tbpl.mozilla.org/?tree=Try&rev=78da773f9049

Comment 13

[u480271]

https://hg.mozilla.org/integration/mozilla-inbound/rev/952adddb379b

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/952adddb379b

Comment 15

[Benoit Jacob [:bjacob] (mostly away)]

Dan, should we rename this bug to "implement clearbuffer" and close it?

Comment 16

[Florian Scholz (Open Web Docs)]

https://developer.mozilla.org/en-US/docs/Web/API/WebGL2RenderingContext/clearBuffer