Closed Bug 1257593 Opened 5 years ago Closed 5 years ago

Handle webgl FramebufferTexture2D() with an unbound texture

Categories

(Core :: Canvas: WebGL, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox48 --- fixed

People

(Reporter: jerry, Assigned: jerry)

References

Details

Attachments

(2 files, 6 obsolete files)

+++ 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
Attachment #8731795 - Attachment is obsolete: true
Attachment #8731795 - Flags: review?(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`.
Attachment #8732080 - Flags: review?(jgilbert) → review+
(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()`.
Attachment #8732080 - Attachment is obsolete: true
Attachment #8732575 - Attachment is obsolete: true
Attachment #8732588 - Attachment is obsolete: true
I'm confused, comment 10 was a follow-up push to inbound to fix typo, not a backout.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/a41a84351d7c
https://hg.mozilla.org/mozilla-central/rev/ed173749281d
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
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.
Attachment #8736561 - Flags: review?(jgilbert) → review+
Attachment #8736561 - Attachment is obsolete: true
Attachment #8737055 - Attachment is obsolete: true
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.
Status: RESOLVED → REOPENED
Keywords: checkin-needed
Resolution: FIXED → ---
https://hg.mozilla.org/mozilla-central/rev/3481f843cf29
Status: REOPENED → RESOLVED
Closed: 5 years ago5 years ago
Resolution: --- → FIXED
Duplicate of this bug: 1228683
You need to log in before you can comment on or make changes to this bug.