1048731 - WebGL2 - Implement Buffer Objects

Status: RESOLVED FIXED

(Core :: Graphics: CanvasWebGL, defect)

Description

[u480271]

https://www.khronos.org/registry/webgl/specs/latest/2.0/#3.7.2

Comment 1

[u480271]

Attachment: [WebGL2] Implement new buffer binding targets

Comment 2

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8527426 [details] [diff] [review]
[WebGL2] Implement new buffer binding targets

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

MOZ_CRASH!

::: dom/canvas/WebGL1ContextBuffers.cpp
@@ +43,5 @@
> +        return true;
> +
> +    bool matchingBinding = !buffer->HasEverBeenBound() || buffer->Target() == target;
> +    if (!matchingBinding)
> +        ErrorInvalidOperation("%s: buffer already bound to a different target", info);

if (HasEverBeenBound() &&
    target != buffer->Target())

@@ +45,5 @@
> +    bool matchingBinding = !buffer->HasEverBeenBound() || buffer->Target() == target;
> +    if (!matchingBinding)
> +        ErrorInvalidOperation("%s: buffer already bound to a different target", info);
> +
> +    return matchingBinding;

I don't think all this merged return stuff is beneficial.

::: dom/canvas/WebGL2ContextBuffers.cpp
@@ +88,5 @@
> +        return;
> +    }
> +
> +    WebGLBufferRefPtr* readBufferSlot = GetBufferSlotByTarget(readTarget);
> +    WebGLBufferRefPtr* writeBufferSlot = GetBufferSlotByTarget(writeTarget);

Shouldn't this be `const WebGLBufferRefPtr&`?

::: dom/canvas/WebGLContextBuffers.cpp
@@ +22,5 @@
> +
> +    if (!buffer)
> +        return;
> +
> +    /* https://www.khronos.org/registry/webgl/specs/latest/2.0/#5.1 */

Include some text here, as well. The online specs change.

@@ +60,2 @@
>  
> +    UpdateBoundBuffer(target, buffer);

Why isn't this in WebGLContextUnchecked::BindBuffer?

@@ +434,5 @@
>  
>  WebGLBufferRefPtr*
>  WebGLContext::GetBufferSlotByTarget(GLenum target)
>  {
>      /* This function assumes that target has been validated for either WebGL1 or WebGL. */

If so, `default:` should be MOZ_CRASH.

@@ +452,5 @@
>  
>  WebGLBufferRefPtr*
>  WebGLContext::GetBufferSlotByTargetIndexed(GLenum target, GLuint index)
>  {
>      /* This function assumes that target has been validated for either WebGL1 or WebGL. */

If so, `default:` should be MOZ_CRASH.

::: dom/canvas/WebGLContextUnchecked.cpp
@@ +33,5 @@
> +    gl->fBindBufferBase(target, index, buffer ? buffer->GLName() : 0);
> +}
> +
> +void
> +WebGLContextUnchecked::BindBufferRange(GLenum target, GLuint index, WebGLBuffer* buffer, WebGLintptr offset, WebGLsizeiptr size)

Long line.

@@ +40,5 @@
> +    gl->fBindBufferRange(target, index, buffer ? buffer->GLName() : 0, offset, size);
> +}
> +
> +void
> +WebGLContextUnchecked::CopyBufferSubData(GLenum readTarget, GLenum writeTarget, GLintptr readOffset, GLintptr writeOffset, GLsizeiptr size)

Long line.

::: dom/canvas/WebGLContextUnchecked.h
@@ +27,5 @@
> +    // Buffer Objects
> +    void BindBuffer(GLenum target, WebGLBuffer* buffer);
> +    void BindBufferBase(GLenum target, GLuint index, WebGLBuffer* buffer);
> +    void BindBufferRange(GLenum taret, GLuint index, WebGLBuffer* buffer, WebGLintptr offset, WebGLsizeiptr size);
> +    void CopyBufferSubData(GLenum readTarget, GLenum writeTarget, GLintptr readOffset, GLintptr writeOffset, GLsizeiptr size);

These are long.

::: dom/canvas/WebGLContextValidate.cpp
@@ +212,5 @@
>      }
>  }
>  
> +bool
> +WebGLContext::ValidateDataOffsetSize(WebGLintptr offset, WebGLsizeiptr size, WebGLsizeiptr bufferSize, const char* info)

Spill an arg or two to the next line. This is long.

Comment 3

[u480271]

Attachment: Wrap lines at 90 chars.

Comment 4

[u480271]

Attachment: Review comments

Address review comments.

Comment 5

[u480271]

https://tbpl.mozilla.org/?tree=Try&rev=69b95c3d3aec

Comment 6

[u480271]

https://tbpl.mozilla.org/?tree=Try&rev=2870621d010c

Comment 7

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8532233 [details] [diff] [review]
Review comments

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

::: dom/canvas/WebGLContextBuffers.cpp
@@ +209,5 @@
>  
>      if (!ValidateBufferTarget(target, "bufferData"))
>          return;
>  
> +    WebGLRefPtr<WebGLBuffer>& bufferSlot = GetBufferSlotByTarget(target);

const this if you can.

@@ +482,4 @@
>  
>      default:
> +        MOZ_CRASH("Should not get here.");
> +        return mBoundArrayBuffer;

Don't bother returning.

Comment 8

[u480271]

(In reply to Jeff Gilbert [:jgilbert] from comment #7)
> Comment on attachment 8532233 [details] [diff] [review]
> Review comments
> 
> Review of attachment 8532233 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/canvas/WebGLContextBuffers.cpp
> @@ +209,5 @@
> >  
> >      if (!ValidateBufferTarget(target, "bufferData"))
> >          return;
> >  
> > +    WebGLRefPtr<WebGLBuffer>& bufferSlot = GetBufferSlotByTarget(target);
> 
> const this if you can.
> 
> @@ +482,4 @@
> >  
> >      default:
> > +        MOZ_CRASH("Should not get here.");
> > +        return mBoundArrayBuffer;
> 
> Don't bother returning.

Don't you have to when the function returns a reference?

Comment 9

[u480271]

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

Comment 10

[u480271]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c9c62f7fb8b2

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/c9c62f7fb8b2

Comment 12

[Florian Scholz (Open Web Docs)]

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