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

Status: RESOLVED FIXED

(Core :: Graphics: CanvasWebGL, defect)

Description

[Jeff Prouty]

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.

Comment 1

[Loic]

Could you attach a minimal testcase (.html), please.

Comment 2

[Jeff Prouty]

Attachment: deleteTexture of a bound texture that unexpectedly changes the ACTIVE_TEXTURE texture unit

Comment 3

[Jeff Prouty]

Thanks Loic for the quick reply. Cheers

Comment 4

[Loic]

Is it normal that the testcase loads a blank page? (in various versions of Firefox)
Is there something to see?

Comment 5

[Jeff Prouty]

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.

Comment 6

[Loic]

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

Comment 7

[Jeff Prouty]

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 }

Comment 8

[Kelsey Gilbert [:jgilbert]]

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.

Comment 9

[Kelsey Gilbert [:jgilbert]]

Attachment: patch

Comment 10

[Jeff Prouty]

Comment on attachment 693199 [details] [diff] [review]
patch

Great! Thanks for the quick fix. You rock!

Comment 11

[Benoit Jacob [:bjacob] (mostly away)]

Comment on attachment 693199 [details] [diff] [review]
patch

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

Two things:

oops, and thanks.

Comment 12

[Kelsey Gilbert [:jgilbert]]

https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=966b04dba626

Comment 13

[Jeff Prouty]

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

Comment 14

[Kelsey Gilbert [:jgilbert]]

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)

Comment 15

[Ed Morley [:emorley]]

Failed to build on any platform, backed out:
https://hg.mozilla.org/integration/mozilla-inbound/rev/dc08a7e9993a

Comment 16

[Benoit Jacob [:bjacob] (mostly away)]

Indeed, it's WebGLuint, not WebGLUint.

Comment 17

[Kelsey Gilbert [:jgilbert]]

https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=a2a26dec9fc4

Comment 18

[Kelsey Gilbert [:jgilbert]]

Try builds look like they're coming in fine:
https://tbpl.mozilla.org/?tree=Try&rev=c2a48b2f166a

Comment 19

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/a2a26dec9fc4

Comment 20

[Kelsey Gilbert [:jgilbert]]

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.

Comment 21

[bhavana bajaj [:bajaj]]

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.

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/54e4ebef9f6b