1167504 - Clean up binding of WebGL objects

Status: RESOLVED FIXED

(Core :: Graphics: CanvasWebGL, defect)

Description

[u480271]

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.

Comment 1

[u480271]

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'.

Comment 2

[u480271]

Attachment: Part 1: Remove BindableName - Framebuffer.

Comment 3

[u480271]

Attachment: Part 2: Remove BindableName - Renderbuffer.

Comment 4

[u480271]

Attachment: Part 3: Remove BindableName - Sampler.

Comment 5

[u480271]

Attachment: Part 4: Remove BindableName - Texture.

Comment 6

[u480271]

Attachment: Part 5: Remove BindableName - Transform Feedback.

Comment 7

[u480271]

Attachment: Part 6: Remove BindableName - Vertex Array.

Comment 8

[u480271]

Attachment: Part 7: Remove BindableName - Buffer.

Comment 9

[u480271]

Attachment: Part 8: Remove WebGLBindableName.h.

Comment 10

[u480271]

Attachment: Part 9: Clean up buffer binding constraints.

Checked against http://www.khronos.org/registry/webgl/sdk/tests/conformance2/buffers/

Comment 11

[u480271]

Looks like Linux is why we can't have nice things. https://treeherder.mozilla.org/#/jobs?repo=try&revision=d96c3d09c072

Comment 12

[u480271]

Attachment: fIsVertexArray is borked for VAO emulation.

Comment 13

[Kelsey Gilbert [: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.

Comment 14

[Kelsey Gilbert [:jgilbert]]

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.

Comment 15

[Kelsey Gilbert [:jgilbert]]

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!

Comment 16

[u480271]

(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?)

Comment 17

[Kelsey Gilbert [:jgilbert]]

(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 18

[Kelsey Gilbert [:jgilbert]]

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.

Comment 19

[u480271]

Attachment: Work around Linux test slave driver bugs.

Comment 20

[u480271]

Attachment: Emulate fIsFramebuffer and fIsRenderbuffer for Android 2.3 emulator.

Comment 21

[u480271]

Try - https://treeherder.mozilla.org/#/jobs?repo=try&revision=f6e2c533b6f9

Comment 22

[u480271]

(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.

Comment 23

[Kelsey Gilbert [:jgilbert]]

(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 24

[Kelsey Gilbert [:jgilbert]]

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.

Comment 25

[Kelsey Gilbert [:jgilbert]]

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.

Comment 26

[u480271]

Attachment: Emulate fIsFramebuffer and fIsRenderbuffer for Android 2.3 emulator.

Comment 27

[u480271]

Greened up all GL mochitests - https://treeherder.mozilla.org/#/jobs?repo=try&revision=e7e54bd66eec

Comment 28

[u480271]

Attachment: Part 10: Mark previously failing tests as passing.

Comment 29

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/05ac4287dd79
https://hg.mozilla.org/integration/mozilla-inbound/rev/49332df2f830
https://hg.mozilla.org/integration/mozilla-inbound/rev/db0c0147c7fb
https://hg.mozilla.org/integration/mozilla-inbound/rev/8f4e5ac93a6b
https://hg.mozilla.org/integration/mozilla-inbound/rev/0696e1f508f3
https://hg.mozilla.org/integration/mozilla-inbound/rev/3b85d8441047
https://hg.mozilla.org/integration/mozilla-inbound/rev/d71cc076307e
https://hg.mozilla.org/integration/mozilla-inbound/rev/5756b0cdb987
https://hg.mozilla.org/integration/mozilla-inbound/rev/f8e64b23ba88
https://hg.mozilla.org/integration/mozilla-inbound/rev/8facb61cae2e
https://hg.mozilla.org/integration/mozilla-inbound/rev/a3943eb0e176
https://hg.mozilla.org/integration/mozilla-inbound/rev/46b3c9ba493f

Comment 30

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/05ac4287dd79
https://hg.mozilla.org/mozilla-central/rev/49332df2f830
https://hg.mozilla.org/mozilla-central/rev/db0c0147c7fb
https://hg.mozilla.org/mozilla-central/rev/8f4e5ac93a6b
https://hg.mozilla.org/mozilla-central/rev/0696e1f508f3
https://hg.mozilla.org/mozilla-central/rev/3b85d8441047
https://hg.mozilla.org/mozilla-central/rev/d71cc076307e
https://hg.mozilla.org/mozilla-central/rev/5756b0cdb987
https://hg.mozilla.org/mozilla-central/rev/f8e64b23ba88
https://hg.mozilla.org/mozilla-central/rev/8facb61cae2e
https://hg.mozilla.org/mozilla-central/rev/a3943eb0e176
https://hg.mozilla.org/mozilla-central/rev/46b3c9ba493f