WebGL2 - Implement Buffer Objects

RESOLVED FIXED in mozilla37

Status

()

defect
RESOLVED FIXED
5 years ago
3 years ago

People

(Reporter: kamidphish, Assigned: kamidphish)

Tracking

(Blocks 1 bug, {dev-doc-complete})

unspecified
mozilla37
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments)

Depends on: 1048668
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.
Attachment #8527426 - Flags: review?(jgilbert) → review+
Attachment #8532232 - Flags: review?(jgilbert)
Address review comments.
Attachment #8532233 - Flags: review?(jgilbert)
Attachment #8532232 - Flags: review?(jgilbert) → review+
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.
Attachment #8532233 - Flags: review?(jgilbert) → review+
(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?
https://hg.mozilla.org/mozilla-central/rev/c9c62f7fb8b2
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
You need to log in before you can comment on or make changes to this bug.