Clean up binding of WebGL objects

RESOLVED FIXED in Firefox 41

Status

()

RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: kamidphish, Assigned: kamidphish)

Tracking

unspecified
mozilla41
Points:
---

Firefox Tracking Flags

(firefox41 fixed)

Details

(Whiteboard: [gfx-noted])

Attachments

(13 attachments, 1 obsolete attachment)

4.97 KB, patch
jgilbert
: review+
Details | Diff | Splinter Review
4.02 KB, patch
jgilbert
: review+
Details | Diff | Splinter Review
3.55 KB, patch
jgilbert
: review+
Details | Diff | Splinter Review
7.58 KB, patch
jgilbert
: review+
Details | Diff | Splinter Review
3.43 KB, patch
jgilbert
: review+
Details | Diff | Splinter Review
2.91 KB, patch
jgilbert
: review+
Details | Diff | Splinter Review
6.46 KB, patch
jgilbert
: review+
Details | Diff | Splinter Review
2.00 KB, patch
jgilbert
: review+
Details | Diff | Splinter Review
22.65 KB, patch
jgilbert
: review+
Details | Diff | Splinter Review
6.59 KB, patch
jgilbert
: review+
Details | Diff | Splinter Review
2.78 KB, patch
jgilbert
: review+
Details | Diff | Splinter Review
7.91 KB, patch
jgilbert
: review+
Details | Diff | Splinter Review
3.86 KB, patch
jgilbert
: review+
Details | Diff | Splinter Review
WebGL and GL objects are bound to binding points. WebGL2 spec contains language about what this means with the addition of new buffer binding points.
Remove WebGLBindableName. I created it for a situation that never really eventuated.

Also, fix up the buffer binding logic to handle `Undefined`, `Element Data`, and `Other Data'.
Created attachment 8609945 [details] [diff] [review]
Part 1: Remove BindableName - Framebuffer.
Attachment #8609945 - Flags: review?(jgilbert)
Created attachment 8609946 [details] [diff] [review]
Part 2: Remove BindableName - Renderbuffer.
Attachment #8609946 - Flags: review?(jgilbert)
Created attachment 8609947 [details] [diff] [review]
Part 3: Remove BindableName - Sampler.
Attachment #8609947 - Flags: review?(jgilbert)
Created attachment 8609948 [details] [diff] [review]
Part 4: Remove BindableName - Texture.
Attachment #8609948 - Flags: review?(jgilbert)
Created attachment 8609949 [details] [diff] [review]
Part 5: Remove BindableName - Transform Feedback.
Attachment #8609949 - Flags: review?(jgilbert)
Created attachment 8609951 [details] [diff] [review]
Part 6: Remove BindableName - Vertex Array.
Attachment #8609951 - Flags: review?(jgilbert)
Created attachment 8609952 [details] [diff] [review]
Part 7: Remove BindableName - Buffer.
Attachment #8609952 - Flags: review?(jgilbert)
Created attachment 8609953 [details] [diff] [review]
Part 8: Remove WebGLBindableName.h.
Attachment #8609953 - Flags: review?(jgilbert)
Created attachment 8609954 [details] [diff] [review]
Part 9: Clean up buffer binding constraints.

Checked against http://www.khronos.org/registry/webgl/sdk/tests/conformance2/buffers/
Attachment #8609954 - Flags: review?(jgilbert)
Looks like Linux is why we can't have nice things. https://treeherder.mozilla.org/#/jobs?repo=try&revision=d96c3d09c072
Created attachment 8610377 [details] [diff] [review]
fIsVertexArray is borked for VAO emulation.
Attachment #8610377 - Flags: review?(jgilbert)
Comment on attachment 8609946 [details] [diff] [review]
Part 2: Remove BindableName - Renderbuffer.

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

::: dom/canvas/WebGLContextGL.cpp
@@ +1718,5 @@
> +    if (rb->IsDeleted())
> +        return false;
> +
> +    MakeContextCurrent();
> +    return gl->fIsRenderbuffer(rb->PrimaryGLName());

Unfortunately:
https://bugzilla.mozilla.org/show_bug.cgi?id=1140459

Some drivers (including our test slaves!) don't give reasonable answers for IsRenderbuffer, maybe others.

What should we do?
This is a silly entrypoint anyway, so honestly leaving it broken on rarer platforms would probably be acceptable. That said, it's not a ton of work to fix.

I'm going to r+ this, but let's leave a comment with a reference to the bug (and bug number) I mentioned. We can see if the conformance test results are too unacceptable this way.
Attachment #8609946 - Flags: review?(jgilbert) → review+
Attachment #8609945 - Flags: review?(jgilbert) → review+
Comment on attachment 8609947 [details] [diff] [review]
Part 3: Remove BindableName - Sampler.

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

::: dom/canvas/WebGLSampler.h
@@ +37,5 @@
>  
>  private:
>      ~WebGLSampler();
> +
> +    GLuint mGLName;

Just make this public and const.
Attachment #8609947 - Flags: review?(jgilbert) → review+
Comment on attachment 8609948 [details] [diff] [review]
Part 4: Remove BindableName - Texture.

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

::: dom/canvas/WebGLTexture.h
@@ +42,5 @@
>  
> +    bool HasEverBeenBound() const { return mTarget != LOCAL_GL_NONE; }
> +    GLuint GLName() const { return mGLName; }
> +    GLenum Target() const { return mTarget; }
> +    

Whitespace!
Attachment #8609948 - Flags: review?(jgilbert) → review+
Attachment #8609949 - Flags: review?(jgilbert) → review+
Attachment #8609951 - Flags: review?(jgilbert) → review+
Attachment #8609952 - Flags: review?(jgilbert) → review+
Attachment #8609953 - Flags: review?(jgilbert) → review+
Attachment #8610377 - Flags: review?(jgilbert) → review+
(In reply to Jeff Gilbert [:jgilbert] from comment #13)
> Unfortunately:
> https://bugzilla.mozilla.org/show_bug.cgi?id=1140459
> 
> Some drivers (including our test slaves!) don't give reasonable answers for
> IsRenderbuffer, maybe others.

Yep. Linux also fails. This is why I said "Looks like Linux is why we can't have nice things" in comment 11. :-(

> What should we do?
> This is a silly entrypoint anyway, so honestly leaving it broken on rarer
> platforms would probably be acceptable. That said, it's not a ton of work to
> fix.

I'll fix it up, like I had to do for the FakeVAOs. I'll #ifdef it for linux and android (or is it gonk?)
(In reply to Dan Glastonbury :djg :kamidphish from comment #16)
> (In reply to Jeff Gilbert [:jgilbert] from comment #13)
> > Unfortunately:
> > https://bugzilla.mozilla.org/show_bug.cgi?id=1140459
> > 
> > Some drivers (including our test slaves!) don't give reasonable answers for
> > IsRenderbuffer, maybe others.
> 
> Yep. Linux also fails. This is why I said "Looks like Linux is why we can't
> have nice things" in comment 11. :-(
> 
> > What should we do?
> > This is a silly entrypoint anyway, so honestly leaving it broken on rarer
> > platforms would probably be acceptable. That said, it's not a ton of work to
> > fix.
> 
> I'll fix it up, like I had to do for the FakeVAOs. I'll #ifdef it for linux
> and android (or is it gonk?)

It's specifically the AndroidEmulator driver. Check the other bug.
Comment on attachment 8609954 [details] [diff] [review]
Part 9: Clean up buffer binding constraints.

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

::: dom/canvas/WebGL2ContextBuffers.cpp
@@ +118,5 @@
> +    WebGLBuffer::ContentKind readType = readBuffer->Content();
> +    WebGLBuffer::ContentKind writeType = writeBuffer->Content();
> +
> +    if (readType != WebGLBuffer::Undefined &&
> +        writeType != WebGLBuffer::Undefined &&

Can these really be Undefined that this point?

::: dom/canvas/WebGLBuffer.h
@@ +25,5 @@
>      , public WebGLContextBoundObject
>  {
>  public:
> +
> +    enum ContentKind {

Strong enums please. WebGLBuffer::ContentKind::Undefined is long, but makes far more sense than WebGLBuffer::Undefined.

We're probably fine with s/ContentKind/Kind/ this way, too.

::: dom/canvas/WebGLContextBuffers.cpp
@@ +417,5 @@
>  {
>      if (IsContextLost())
>          return false;
>  
> +    

Tame your editor!

@@ +452,5 @@
> +     * ELEMENT_ARRAY_BUFFER will generate an INVALID_OPERATION error,
> +     * and the state of the binding point will remain untouched.
> +     */
> +
> +    GLenum boundTo = IsBound(buffer);

It's weird that IsBound does not return bool.
Attachment #8609954 - Flags: review?(jgilbert) → review+
Created attachment 8611118 [details] [diff] [review]
Work around Linux test slave driver bugs.
Attachment #8611118 - Flags: review?(jgilbert)
Created attachment 8611120 [details] [diff] [review]
Emulate fIsFramebuffer and fIsRenderbuffer for Android 2.3 emulator.
Attachment #8611120 - Flags: review?(jgilbert)
(In reply to Jeff Gilbert [:jgilbert] from comment #18)
> Comment on attachment 8609954 [details] [diff] [review]
> Part 9: Clean up buffer binding constraints.
> 
> Review of attachment 8609954 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/canvas/WebGL2ContextBuffers.cpp
> @@ +118,5 @@
> > +    WebGLBuffer::ContentKind readType = readBuffer->Content();
> > +    WebGLBuffer::ContentKind writeType = writeBuffer->Content();
> > +
> > +    if (readType != WebGLBuffer::Undefined &&
> > +        writeType != WebGLBuffer::Undefined &&
> 
> Can these really be Undefined that this point?

According the conformance test suite, yes.

You can create a buffer, bind it to COPY_READ_BUFFER, bufferData to it and the contents is `undefined` until it's bound to a binding point that defines the contents type as `element data` or `other data`.
 
> ::: dom/canvas/WebGLBuffer.h
> @@ +25,5 @@
> >      , public WebGLContextBoundObject
> >  {
> >  public:
> > +
> > +    enum ContentKind {
> 
> Strong enums please. WebGLBuffer::ContentKind::Undefined is long, but makes
> far more sense than WebGLBuffer::Undefined.
> 
> We're probably fine with s/ContentKind/Kind/ this way, too.

OK.


> ::: dom/canvas/WebGLContextBuffers.cpp
> @@ +417,5 @@
> >  {
> >      if (IsContextLost())
> >          return false;
> >  
> > +    
> 
> Tame your editor!

Ugh. That's what I get for working on Windows.

> > +    GLenum boundTo = IsBound(buffer);
> 
> It's weird that IsBound does not return bool.

I was having a mental block on naming. How about `IsBoundTo` or `BoundTo`. I thought boundTo = BoundTo(...) was too repetitive.
(In reply to Dan Glastonbury :djg :kamidphish from comment #22)
> (In reply to Jeff Gilbert [:jgilbert] from comment #18)
> > Comment on attachment 8609954 [details] [diff] [review]
> > Part 9: Clean up buffer binding constraints.
> > 
> > Review of attachment 8609954 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: dom/canvas/WebGL2ContextBuffers.cpp
> > @@ +118,5 @@
> > > +    WebGLBuffer::ContentKind readType = readBuffer->Content();
> > > +    WebGLBuffer::ContentKind writeType = writeBuffer->Content();
> > > +
> > > +    if (readType != WebGLBuffer::Undefined &&
> > > +        writeType != WebGLBuffer::Undefined &&
> > 
> > Can these really be Undefined that this point?
> 
> According the conformance test suite, yes.
> 
> You can create a buffer, bind it to COPY_READ_BUFFER, bufferData to it and
> the contents is `undefined` until it's bound to a binding point that defines
> the contents type as `element data` or `other data`.
Oh, cool.

>  
> > ::: dom/canvas/WebGLBuffer.h
> > @@ +25,5 @@
> > >      , public WebGLContextBoundObject
> > >  {
> > >  public:
> > > +
> > > +    enum ContentKind {
> > 
> > Strong enums please. WebGLBuffer::ContentKind::Undefined is long, but makes
> > far more sense than WebGLBuffer::Undefined.
> > 
> > We're probably fine with s/ContentKind/Kind/ this way, too.
> 
> OK.
> 
> 
> > ::: dom/canvas/WebGLContextBuffers.cpp
> > @@ +417,5 @@
> > >  {
> > >      if (IsContextLost())
> > >          return false;
> > >  
> > > +    
> > 
> > Tame your editor!
> 
> Ugh. That's what I get for working on Windows.
Just VS! I actually use Geany for my basic editing needs, and it happily strips EOL whitespace. Notepad++ also does this.
> 
> > > +    GLenum boundTo = IsBound(buffer);
> > 
> > It's weird that IsBound does not return bool.
> 
> I was having a mental block on naming. How about `IsBoundTo` or `BoundTo`. I
> thought boundTo = BoundTo(...) was too repetitive.

Since it returns the bind point enum (if any), why not Get(Cur)BindPoint?
Comment on attachment 8611118 [details] [diff] [review]
Work around Linux test slave driver bugs.

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

::: dom/canvas/WebGLVertexArrayGL.cpp
@@ +61,5 @@
> +        gl->Vendor() == gl::GLVendor::VMware &&
> +        gl->Renderer() == gl::GLRenderer::GalliumLlvmpipe)
> +    {
> +        return mIsVAO;
> +    } else {

No need for else after return, so just fall through to the non-ifdef'd code.
Attachment #8611118 - Flags: review?(jgilbert) → review+
Comment on attachment 8611120 [details] [diff] [review]
Emulate fIsFramebuffer and fIsRenderbuffer for Android 2.3 emulator.

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

::: dom/canvas/WebGLContextGL.cpp
@@ +160,5 @@
>          gl->fBindFramebuffer(target, 0);
>      } else {
>          GLuint framebuffername = wfb->GLName();
>          gl->fBindFramebuffer(target, framebuffername);
> +#if ANDROID_VERSION >= 9 && ANDROID_VERSION < 11

This same conditional is all over, and *needs* to match. This seems fragile. I think we should make a define specifically for this, and just reference that define everywhere we need to.

@@ +1703,5 @@
> +    if (gl->WorkAroundDriverBugs() &&
> +        gl->Renderer() == GLRenderer::AndroidEmulator)
> +    {
> +        return fb->mIsFB;
> +    } else {

Again, no need for this block. Just let control fall out of the ifdef block to the normal code.
Attachment #8611120 - Flags: review?(jgilbert) → review-
Created attachment 8612086 [details] [diff] [review]
Emulate fIsFramebuffer and fIsRenderbuffer for Android 2.3 emulator.
Attachment #8611120 - Attachment is obsolete: true
Attachment #8612086 - Flags: review?(jgilbert)
Attachment #8612086 - Flags: review?(jgilbert) → review+
Created attachment 8613346 [details] [diff] [review]
Part 10: Mark previously failing tests as passing.
Attachment #8613346 - Flags: review?(jgilbert)
Attachment #8613346 - Flags: review?(jgilbert) → review+
You need to log in before you can comment on or make changes to this bug.