Firefox WebGL: deleteTexture of a bound (but not active) texture makes the texture become the activeTexture after deletion

RESOLVED FIXED in Firefox 19

Status

()

defect
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: prouty, Assigned: jgilbert)

Tracking

({regression, testcase})

11 Branch
mozilla20
x86
All
Points:
---

Firefox Tracking Flags

(firefox18 wontfix, firefox19 fixed, firefox20 fixed)

Details

Attachments

(2 attachments)

(Reporter)

Description

6 years ago
User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_8_2) AppleWebKit/537.11 (KHTML, like Gecko) Chrome/23.0.1271.97 Safari/537.11

Steps to reproduce:

Reproduction:
var gl = canvas.getContext('moz-webgl');

var texture1 = gl.createTexture();
// Should be active by default, but for completeness:
gl.activeTexture(gl.TEXTURE0);
gl.bindTexture(gl.TEXTURE_2D, texture1);

// Verify TEXTURE0 is indeed the current texture.
assert(gl.getParameter(gl.ACTIVE_TEXTURE) == gl.TEXTURE0);

var texture2 = gl.createTexture();
gl.activeTexture(gl.TEXTURE15);
gl.bindTexture(gl.TEXTURE_2D, texture2);

//Revert to the active texture TEXTURE0 and verify.
gl.activeTexture(gl.TEXTURE0);
assert(gl.getParameter(gl.ACTIVE_TEXTURE) == gl.TEXTURE0);

// Delete a bound texture which is not the currently active texture.
gl.deleteTexture(texture2);

// Expect that the activeTexture is still TEXTURE0. This assertion fails; ACTIVE_TEXTURE becomes TEXTURE15.
assert(gl.getParameter(gl.ACTIVE_TEXTURE) == gl.TEXTURE0);


Actual results:

Create two or more textures. Bind both to different texture units. Delete a bound texture that is not bound to the active texture unit. The texture unit that was bound to that texture will now become the active texture.

In the example above, TEXTURE15 becomes the ACTIVE_TEXTURE immediately after the delete.
Create a new texture, bind the 2dTexture to something other than the
// previously active texture (by temporarily setting the active texture to
// something else and then reverting it back to the original value), and then
// delete the texture.
//
  // After the deleteTexture call, the activeTexture unexpectedly changes from
  // the active texture prior to the deleteTexture call to the texture index of
  // the bound texture. This is not specified in the WebGL spec and is a firefox
  // specific behavior.



Expected results:

The ACTIVE_TEXTURE should remain the active texture. In the example above, TEXTURE0 should have been the active texture immediately before and after the delete call.

The webgl standard does not specify any special behavior about a deleteTexture changing the active texture unit.

As a comparison, this same code example works correctly on all other browsers that support WebGL (Chrome, Safari, Opera). This behavior is a nuisance for developers and is not always easy to determine the source of error.
(Reporter)

Updated

6 years ago
OS: Mac OS X → All
Component: Untriaged → Canvas: WebGL
Product: Firefox → Core

Comment 1

6 years ago
Could you attach a minimal testcase (.html), please.
Flags: needinfo?(prouty)
(Reporter)

Comment 3

6 years ago
Thanks Loic for the quick reply. Cheers

Updated

6 years ago
Attachment #693031 - Attachment mime type: text/plain → text/html

Comment 4

6 years ago
Is it normal that the testcase loads a blank page? (in various versions of Firefox)
Is there something to see?
Flags: needinfo?(prouty)
(Reporter)

Comment 5

6 years ago
Yes, that is normal. The bug is about webgl state (and in this case does not require uploading any textures or drawing anything). If you inspect the console log (or step through the javascript, you should see the state inconsistently very clearly.
Flags: needinfo?(prouty)

Comment 6

6 years ago
It looks like there is a regression because in FF10-, I got:

[01:27:03,112] At start, ACTIVE_TEXTURE is 0: true
[01:27:03,115] before deleteTexture, ACTIVE_TEXTURE is 0: true
[01:27:03,117] after deleteTexture, ACTIVE_TEXTURE is 0: true

Regression range:

m-c
good=2011-12-05
bad=2011-12-06
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=1bd7482ad4d1&tochange=fafaf614791f

m-i
good=2011-12-04
bad=2011-12-05
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=2a15404c1d71&tochange=431962a6ac34

Suspected bug:
Bug 704839 - Refactor mutual ownership of WebGL objects
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: regression, testcase
Version: 17 Branch → 11 Branch
(Reporter)

Comment 7

6 years ago
That sounds about right. We first noticed the error back in FF12 in April.

The fix is rather simple. Sorry I don't have a FF environment set up to create a patch, but it's a two line fix. If someone could make the patch for me, that would be much appreciated!

source/content/canvas/src/WebGLContextGL.cpp:
1053 WebGLContext::DeleteTexture(WebGLTexture *tex)
1054 {
1055     if (!IsContextStable())
1056         return;
1057 
1058     if (!ValidateObjectAllowDeletedOrNull("deleteTexture", tex))
1059         return;
1060 
1061     if (!tex || tex->IsDeleted())
1062         return;
1063 
1064     if (mBoundFramebuffer)
1065         mBoundFramebuffer->DetachTexture(tex);
1066 
++++     WebGLuint activeTexture = mActiveTexture;
1067     for (int32_t i = 0; i < mGLMaxTextureUnits; i++) {
1068         if ((tex->Target() == LOCAL_GL_TEXTURE_2D && mBound2DTextures[i] == tex) ||
1069             (tex->Target() == LOCAL_GL_TEXTURE_CUBE_MAP && mBoundCubeMapTextures[i] == tex))
1070         {
1071             ActiveTexture(LOCAL_GL_TEXTURE0 + i);
1072             BindTexture(tex->Target(), static_cast<WebGLTexture*>(nullptr));
1073         }
1074     }
++++     ActiveTexture(LOCAL_GL_TEXTURE0 + activeTexture);
1076 
1077     tex->RequestDelete();
1078 }
(Assignee)

Comment 8

6 years ago
Indeed, this is a silly mistake. You'll probably notice that we actually leave the texture unit to the last texture unit which has a valid texture attached to it.

Hah, looks like you posted the same patch I had in mind.
(Assignee)

Comment 9

6 years ago
Posted patch patchSplinter Review
Assignee: nobody → jgilbert
Status: NEW → ASSIGNED
Attachment #693199 - Flags: review?(bjacob)
(Reporter)

Comment 10

6 years ago
Comment on attachment 693199 [details] [diff] [review]
patch

Great! Thanks for the quick fix. You rock!

Updated

6 years ago
Blocks: 704839
Comment on attachment 693199 [details] [diff] [review]
patch

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

Two things:

oops, and thanks.
Attachment #693199 - Flags: review?(bjacob) → review+
(Reporter)

Comment 13

6 years ago
Which milestone/release will this likely go out in (for Firefox)? Sorry for the noob questions!
(Assignee)

Comment 14

6 years ago
It'll hit Aurora 19, and probably Beta 18. Beta depends on release drivers, Aurora is a pretty sure thing, though.

18 will hit Release on (or around) Jan 6. 19 will hit Release in mid February. (Around the 15th, I think)
Indeed, it's WebGLuint, not WebGLUint.
(Assignee)

Comment 18

6 years ago
Try builds look like they're coming in fine:
https://tbpl.mozilla.org/?tree=Try&rev=c2a48b2f166a
https://hg.mozilla.org/mozilla-central/rev/a2a26dec9fc4
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
(Assignee)

Comment 20

6 years ago
Comment on attachment 693199 [details] [diff] [review]
patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 704839
User impact if declined: Bad behavior for WebGL when deleting textures. (Fairly common use-case)
Testing completed (on m-c, etc.): On m-c.
Risk to taking this patch (and alternatives if risky): None.
String or UUID changes made by this patch: None.
Attachment #693199 - Flags: approval-mozilla-beta?
Attachment #693199 - Flags: approval-mozilla-b2g18?
Attachment #693199 - Flags: approval-mozilla-aurora?
Comment on attachment 693199 [details] [diff] [review]
patch

Considering where we are close to release for FF18, a- this patch for beta.Low risk enough to approve on aurora 19.
Attachment #693199 - Flags: approval-mozilla-beta?
Attachment #693199 - Flags: approval-mozilla-beta-
Attachment #693199 - Flags: approval-mozilla-b2g18?
Attachment #693199 - Flags: approval-mozilla-b2g18-
Attachment #693199 - Flags: approval-mozilla-aurora?
Attachment #693199 - Flags: approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.