Status

()

defect
RESOLVED FIXED
5 years ago
3 years ago

People

(Reporter: kamidphish, Assigned: kamidphish)

Tracking

({dev-doc-complete})

Firefox Tracking Flags

(Not tracked)

Details

()

Attachments

(3 attachments)

Implement ClearBuffer family of functions for WebGL2.
Summary: WebGL2 - Implement ClearBufferXXX APIs → WebGL2 - Implement Multiple Render Targets
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.
Attachment #8467498 - Flags: review?(jgilbert) → review+
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.
(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.
(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 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.
Attachment #8469720 - Flags: review?(jgilbert) → review+
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
Attachment #8491624 - Flags: review?(jgilbert)
Comment on attachment 8491624 [details] [diff] [review]
clearbuffer-fixes

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

I like it.
Attachment #8491624 - Flags: review?(jgilbert) → review+
Dan, should we rename this bug to "implement clearbuffer" and close it?
Flags: needinfo?(dglastonbury)
Keywords: leave-open
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Flags: needinfo?(dglastonbury)
Resolution: --- → FIXED
Summary: WebGL2 - Implement Multiple Render Targets → WebGL2 - Implement clearbuffer
You need to log in before you can comment on or make changes to this bug.