1257593 - Handle webgl FramebufferTexture2D() with an unbound texture

Status: RESOLVED FIXED

(Core :: Graphics: CanvasWebGL, defect)

Description

[Jerry Shih[:jerry] (UTC+8) (inactive)]

+++ This bug was initially created as a clone of Bug #1252414 +++

If we have webgl code like:

  var texture = gl.createTexture(); // only createTexture(), but no bindBuffer()
  gl.framebufferTexture2D(gl.FRAMEBUFFER, gl.STENCIL_ATTACHMENT, gl.TEXTURE_CUBE_MAP_NEGATIVE_Z, texture, 0);

It will hit an assert at [1], since the texture is still an unbound texture.

I'm not sure how to handle this situation. So I try to return an InvalidOperation error for this case.

There is no clear item for this in webgl2 and opengles 3 spec.
https://www.khronos.org/opengles/sdk/docs/man32/html/glFramebufferTexture2D.xhtml
https://www.khronos.org/registry/webgl/specs/latest/2.0/

[1]
https://hg.mozilla.org/mozilla-central/annotate/5cfc10c14aaea2a449f74dcbb366179a45442dd6/dom/canvas/WebGLTexture.h#l306

Comment 1

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: Handle webgl FramebufferTexture2D() with an unbound texture. v1

Comment 2

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: Handle webgl FramebufferTexture2D() with an unbound texture. v2

fix typo.

Comment 3

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8732080 [details] [diff] [review]
Handle webgl FramebufferTexture2D() with an unbound texture. v2

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

::: dom/canvas/WebGLFramebuffer.cpp
@@ +623,5 @@
>      if (!mContext->ValidateObjectAllowNull("framebufferTexture2D: texture", tex))
>          return;
>  
>      if (tex) {
> +        if (!tex->IsTexture()) {

Just `tex->HasEverBeenBound()` or `tex->Target() == LOCAL_GL_NONE`.

Comment 4

[Kelsey Gilbert [:jgilbert]]

(In reply to Jeff Gilbert [:jgilbert] from comment #3)
> Comment on attachment 8732080 [details] [diff] [review]
> Handle webgl FramebufferTexture2D() with an unbound texture. v2
> 
> Review of attachment 8732080 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/canvas/WebGLFramebuffer.cpp
> @@ +623,5 @@
> >      if (!mContext->ValidateObjectAllowNull("framebufferTexture2D: texture", tex))
> >          return;
> >  
> >      if (tex) {
> > +        if (!tex->IsTexture()) {
> 
> Just `tex->HasEverBeenBound()` or `tex->Target() == LOCAL_GL_NONE`.

Er, `!tex->HasEveryBeenBound()`.

Comment 5

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: Handle webgl FramebufferTexture2D() with an unbound texture. v3. r=jgilbert

update for comment 4.

Comment 6

[Jerry Shih[:jerry] (UTC+8) (inactive)]

try link:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=15a296d686d6

Comment 7

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: Handle webgl FramebufferTexture2D() with an unbound texture. v4. r=jgilbert

fix typo.

Comment 8

[Jerry Shih[:jerry] (UTC+8) (inactive)]

try link:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=15a296d686d6

Please land the attachment 8732588 [details] [diff] [review] to m-c.

Comment 9

[Pulsebot]

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

Comment 10

[Pulsebot]

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

Comment 11

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: Handle webgl FramebufferTexture2D() with an unbound texture. v5. r=jgilbert

fix typo. :(

Comment 12

[Jerry Shih[:jerry] (UTC+8) (inactive)]

try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b9756b313c2f

Please land the attachment 8732713 [details] [diff] [review] to m-c.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

I'm confused, comment 10 was a follow-up push to inbound to fix typo, not a backout.

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/a41a84351d7c
https://hg.mozilla.org/mozilla-central/rev/ed173749281d

Comment 15

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: test case for webgl framebufferTexture2D(). v1

Comment 16

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8736561 [details] [diff] [review]
test case for webgl framebufferTexture2D(). v1

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

::: dom/canvas/test/webgl-mochitest/test_webgl_fuzzy_pattern.html
@@ +12,5 @@
> +  var canvas = document.createElement('canvas');
> +  canvas.id = 'webglCanvas';
> +  canvas.height = 512;
> +  canvas.width = 512;
> +  document.body.appendChild(canvas);

No need for anything but the `var canvas...` line here, so remove them.

Comment 17

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: test case for webgl framebufferTexture2D(). v2. r=jgilbert

Comment 18

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: test case for webgl framebufferTexture2D(). v3. r=jgilbert

rebase

Comment 19

[Jerry Shih[:jerry] (UTC+8) (inactive)]

try for attachment 8738945 [details] [diff] [review]
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c58ae61969a5

Comment 20

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Please land the attachment 8738945 [details] [diff] [review] to m-c after the attachment 8738944 [details] [diff] [review] in bug 1252414 landed.
mochitest for this bug.

Comment 21

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3481f843cf29

Comment 22

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/3481f843cf29