Fix Firefox console warnings. #5939

bagnell · 2017-10-26T21:58:32Z

Only use pre-multiply alpha and y flip for DOM sources.
Initialized textures before using texSubImage2D.
- Use texImage2D when the sub image is the same size as the texture
- Use texImage2D to initialize to zero when the sub image is smaller.

…extures before using texSubImage.

cesium-concierge · 2017-10-26T21:58:32Z

@bagnell, thanks for the pull request! Maintainers, we have a signed CLA from @bagnell, so you can review this at any time.

I noticed that CHANGES.md has not been updated. If this change updates the public API in any way, fixes a bug, or makes any non-trivial update, please add a bullet point to CHANGES.md and comment on this pull request so we know it was updated. For more info, see the Pull Request Guidelines.

I am a bot who helps you make Cesium awesome! Thanks again.

mramato · 2017-10-27T00:19:22Z

Source/Renderer/CubeMap.js

@@ -121,25 +120,30 @@ define([
        gl.activeTexture(gl.TEXTURE0);
        gl.bindTexture(textureTarget, texture);

-        function createFace(target, sourceFace) {
+        function createFace(target, sourceFace, preMultiplyAlpha, flipY) {
+            // TODO: gl.pixelStorei(gl._UNPACK_ALIGNMENT, 4);


Did you mean to leave this TODO in?

Yes, that was in the original code. I just moved it.

pjcozzi · 2017-10-27T20:51:07Z

Source/Renderer/CubeMap.js

-        function createFace(target, sourceFace) {
+        function createFace(target, sourceFace, preMultiplyAlpha, flipY) {
+            // TODO: gl.pixelStorei(gl._UNPACK_ALIGNMENT, 4);
+            gl.pixelStorei(gl.UNPACK_PREMULTIPLY_ALPHA_WEBGL, false);


Are there duplicate pixelStorei calls if sourceFace.arrayBufferView below is not defined? If so, can we move these to the first if block?

Bump - I was about to leave this comment and then noticed @pjcozzi already mentioned it.

pjcozzi · 2017-10-27T20:54:44Z

Source/Renderer/CubeMapFace.js

-            gl.texSubImage2D(this._targetFace, 0, xOffset, yOffset, source.width, source.height, this._pixelFormat, this._pixelDatatype, source.arrayBufferView);
-        } else {
-            gl.texSubImage2D(this._targetFace, 0, xOffset, yOffset, this._pixelFormat, this._pixelDatatype, source);
+        var uploaded = false;


Here and below seems more complicated than it needs to be; I will not have the time to dive deep for awhile.

@lilleyse can you take a look?

mramato · 2017-10-31T19:19:39Z

@bagnell should we get this into tomorrow's release? Seems like we would want to (especially if it's a speed improvement).

pjcozzi · 2017-10-31T20:42:04Z

Please do not rush the review - would rather see this go into the following release.

mramato · 2017-11-01T18:08:43Z

Do you think this will help with #3888? The Firefox tab now crashes if you run the tests with WebGL validation.

bagnell · 2017-11-09T20:50:37Z

No, the tab still crashes for me.

thw0rted · 2018-02-16T13:41:44Z

Hey all, just curious if this fell off somebody's plate -- I'm still seeing the warnings I mentioned in #5912 .

hpinkos · 2018-02-16T16:10:12Z

Thanks for the reminder @thw0rted

@bagnell what's the status of this PR?

bagnell · 2018-02-16T19:15:44Z

Waiting for a review from @lilleyse.

lilleyse · 2018-02-16T20:53:52Z

Source/Renderer/CubeMap.js

-        function createFace(target, sourceFace) {
+        function createFace(target, sourceFace, preMultiplyAlpha, flipY) {
+            // TODO: gl.pixelStorei(gl._UNPACK_ALIGNMENT, 4);
+            gl.pixelStorei(gl.UNPACK_PREMULTIPLY_ALPHA_WEBGL, false);


Bump - I was about to leave this comment and then noticed @pjcozzi already mentioned it.

lilleyse · 2018-02-16T20:55:09Z

Source/Renderer/CubeMapFace.js

-        gl.pixelStorei(gl.UNPACK_PREMULTIPLY_ALPHA_WEBGL, this._preMultiplyAlpha);
-        gl.pixelStorei(gl.UNPACK_FLIP_Y_WEBGL, this._flipY);
+        gl.pixelStorei(gl.UNPACK_PREMULTIPLY_ALPHA_WEBGL, false);
+        gl.pixelStorei(gl.UNPACK_FLIP_Y_WEBGL, false);


Same comment here - there are some situations where gl.pixelStorei gets called twice.

lilleyse · 2018-02-20T13:52:48Z

Source/Renderer/CubeMapFace.js

+                }
+
+                var size = PixelFormat.componentsLength(this._pixelFormat) * this._size * this._size;
+                var bufferView = new constructor(size);


This could maybe be simplified with PixelFormat.textureSizeInBytes. And to round it out PixelFormat could have a function like PixelFormat.createTypedArray, like how we have ComponentDatatype.createTypedArray

lilleyse · 2018-02-20T13:54:58Z

Source/Renderer/CubeMap.js

+            createFace(gl.TEXTURE_CUBE_MAP_POSITIVE_Y, source.positiveY, preMultiplyAlpha, flipY);
+            createFace(gl.TEXTURE_CUBE_MAP_NEGATIVE_Y, source.negativeY, preMultiplyAlpha, flipY);
+            createFace(gl.TEXTURE_CUBE_MAP_POSITIVE_Z, source.positiveZ, preMultiplyAlpha, flipY);
+            createFace(gl.TEXTURE_CUBE_MAP_NEGATIVE_Z, source.negativeZ, preMultiplyAlpha, flipY);
        } else {
            gl.texImage2D(gl.TEXTURE_CUBE_MAP_POSITIVE_X, 0, pixelFormat, size, size, 0, pixelFormat, pixelDatatype, null);


Is it valid to pass in null? If the value is null does the texture still require another texImage2D call with an empty typed array? Could that step go here instead? If so, then I think all the initialized checks in CubeMapFace may not be needed.

I did it this way for the case where you create a texture and upload the entire texture later. This avoids creating an array buffer of all zeroes. The only time the array buffer of zeroes would be created would be when a texture is created and then a bunch of texSubImage2D's are called to fill out an atlas. If we don't care about that case or optimization, then Ill change this.

Also, a texture that is going to be rendered to cannot be set.

lilleyse · 2018-02-20T13:59:45Z

Source/Renderer/Texture.js

                // Source: ImageData, HTMLImageElement, HTMLCanvasElement, or HTMLVideoElement
                gl.texImage2D(textureTarget, 0, internalFormat, pixelFormat, pixelDatatype, source);
            }
        } else {
            gl.texImage2D(textureTarget, 0, internalFormat, width, height, 0, pixelFormat, pixelDatatype, null);
+            initialized = false;


Basically the same note as above, can we initialize the texture with an empty typed array now to avoid the initialized check in copyFrom?

Same comment here.

bagnell · 2018-02-28T21:07:41Z

@lilleyse This is ready for another look.

lilleyse · 2018-02-28T22:10:17Z

Source/Renderer/Texture.js

-        gl.pixelStorei(gl.UNPACK_PREMULTIPLY_ALPHA_WEBGL, this._preMultiplyAlpha);
-        gl.pixelStorei(gl.UNPACK_FLIP_Y_WEBGL, this._flipY);
+        gl.pixelStorei(gl.UNPACK_PREMULTIPLY_ALPHA_WEBGL, false);
+        gl.pixelStorei(gl.UNPACK_FLIP_Y_WEBGL, false);


Make the same changes as CubeMapFace so that pixelStorei isn't called multiple times in some cases

lilleyse · 2018-02-28T22:32:29Z

Tests pass and the Sandcastle demos I tried still work.

Here and below seems more complicated than it needs to be; I will not have the time to dive deep for awhile. @lilleyse can you take a look?

It seems mostly unavoidable to me based on this discussion #5939 (comment).

Do we want to release this with 1.43? @pjcozzi did you want to take a look?

pjcozzi · 2018-02-28T22:37:07Z

I don't have bandwidth to look at this for 1.43, but please merge if you are confident.

Revert #5939

markerikson · 2018-05-11T18:26:37Z

I'm still seeing a related warning with Cesium 1.45 and Firefox 52 (which we are unfortunately stuck on for our application):

Error: WebGL: texSubImage2D: Conversion requires pixel reformatting.

Was that supposed to be resolved by this PR?

hpinkos · 2018-05-11T18:31:50Z

Hi @markerikson, do you see that error in Cesium 1.44 as well? This PR was added in 1.43 so if you're seeing the error in 1.45 but not 1.44 it means that we might have introduced something in 1.45

markerikson · 2018-05-11T20:30:02Z

Loading up our app with 1.44, I actually see considerably more warnings:

Error: WebGL: texImage2D: Alpha-premult and y-flip are deprecated for non-DOM-Element uploads.
Error: WebGL: texImage2D: Conversion requires y-flip.
Error: WebGL: texSubImage2D: Texture has not been initialized prior to a partial upload, forcing the browser to clear it. This may be slow.
Error: WebGL: texSubImage2D: This operation requires zeroing texture data. This is slow.
Error: WebGL: texSubImage2D: Conversion requires pixel reformatting.

(All repeated quite a few times, just isolating the unique messages.)

In 1.45, I only see the "pixel formatting" message.

bagnell · 2018-05-11T20:43:41Z

I haven't seen that error, but I'm using Firefox 60. Do you see that in the Viewer or any Sandcastle example? If not, can you post an example so I can take a look?

markerikson · 2018-05-11T20:49:45Z

Yes, just loading up the main Sandcastle page in FF52 immediately shows the "pixel reformatting" warning in the console.

hpinkos · 2018-05-11T20:52:48Z

Hmm, I don't see it on FF59 either. It's unfortunate you're unable to upgrade

markerikson · 2018-05-11T21:07:10Z

Yeah, we have an internal app that targets a specific Firefox ESR version at a time, so we're still targeting FF52 for this dev cycle. We'll live with it, just was wanting to see if it was supposed to have been resolved already (and at least there's fewer warnings).

Only use pre-multiply alpha and y flip for DOM sources. Initialized t…

f7fac41

…extures before using texSubImage.

Update CHANGES.md.

95e3f9a

mramato reviewed Oct 27, 2017

View reviewed changes

Merge branch 'master' into firefox-warnings

40ab429

pjcozzi reviewed Oct 27, 2017

View reviewed changes

Merge branch 'master' into firefox-warnings

4ab51ce

bagnell added 3 commits December 14, 2017 17:15

Merge branch 'master' into firefox-warnings

67f9ac4

Merge branch 'master' into firefox-warnings

c1f3a04

Merge branch 'master' into firefox-warnings

19af263

lilleyse reviewed Feb 20, 2018

View reviewed changes

bagnell added 3 commits February 28, 2018 14:46

Merge branch 'master' into firefox-warnings

21cc4ec

Update pixelStorei from review.

d9f44d0

Have PixelFormat create an array buffer of the right size.

09720d0

lilleyse reviewed Feb 28, 2018

View reviewed changes

bagnell added 2 commits February 28, 2018 17:40

Don't call pixelStorei multiple times in Texture.copyFrom.

1bcac56

Fix remaining pixelStorei.

544d08e

lilleyse merged commit 0f92a1b into master Mar 1, 2018

lilleyse deleted the firefox-warnings branch March 1, 2018 15:28

hpinkos pushed a commit that referenced this pull request Mar 1, 2018

revert #5939

47a8b74

hpinkos mentioned this pull request Mar 1, 2018

Revert #5939 #6293

Merged

lilleyse added a commit that referenced this pull request Mar 1, 2018

Merge pull request #6293 from AnalyticalGraphicsInc/revert5939

0e84ac1

Revert #5939

bagnell mentioned this pull request Mar 1, 2018

Fix Firefox warnings #6297

Merged

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix Firefox console warnings. #5939

Fix Firefox console warnings. #5939

bagnell commented Oct 26, 2017

cesium-concierge commented Oct 26, 2017

mramato Oct 27, 2017

bagnell Oct 27, 2017

pjcozzi Oct 27, 2017

lilleyse Feb 16, 2018

pjcozzi Oct 27, 2017

mramato commented Oct 31, 2017

pjcozzi commented Oct 31, 2017

mramato commented Nov 1, 2017

bagnell commented Nov 9, 2017

thw0rted commented Feb 16, 2018

hpinkos commented Feb 16, 2018

bagnell commented Feb 16, 2018

lilleyse Feb 16, 2018

lilleyse Feb 16, 2018

lilleyse Feb 20, 2018

lilleyse Feb 20, 2018

bagnell Feb 28, 2018

bagnell Feb 28, 2018

lilleyse Feb 20, 2018

bagnell Feb 28, 2018

bagnell commented Feb 28, 2018

lilleyse Feb 28, 2018

bagnell Feb 28, 2018

lilleyse commented Feb 28, 2018

pjcozzi commented Feb 28, 2018

markerikson commented May 11, 2018

hpinkos commented May 11, 2018

markerikson commented May 11, 2018

bagnell commented May 11, 2018

markerikson commented May 11, 2018

hpinkos commented May 11, 2018

markerikson commented May 11, 2018

Fix Firefox console warnings. #5939

Fix Firefox console warnings. #5939

Conversation

bagnell commented Oct 26, 2017

cesium-concierge commented Oct 26, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mramato commented Oct 31, 2017

pjcozzi commented Oct 31, 2017

mramato commented Nov 1, 2017

bagnell commented Nov 9, 2017

thw0rted commented Feb 16, 2018

hpinkos commented Feb 16, 2018

bagnell commented Feb 16, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bagnell commented Feb 28, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lilleyse commented Feb 28, 2018

pjcozzi commented Feb 28, 2018

markerikson commented May 11, 2018

hpinkos commented May 11, 2018

markerikson commented May 11, 2018

bagnell commented May 11, 2018

markerikson commented May 11, 2018

hpinkos commented May 11, 2018

markerikson commented May 11, 2018