Fix non power-of-2 textures being flipped when resized #15904

merwaaan · 2019-03-04T19:10:35Z

Fix for #15896

Uses an OffscreenCanvas only when document is not available (was the default choice before)
Flips the texture vertically in this case

mrdoob · 2019-03-05T01:08:21Z

@Mugen87 does this look right to you?

Mugen87 · 2019-03-05T10:26:25Z

I've tested the PR locally with the code from my test fiddle. Looks good now.

mrdoob · 2019-03-05T11:18:51Z

src/renderers/webgl/WebGLTextures.js

 	//

-	var useOffscreenCanvas = typeof OffscreenCanvas !== 'undefined';
+	var useOffscreenCanvas = typeof document === 'undefined';


Why are we changing this?

OffscreenCanvas is also available in a non worker scenario. So testing for document seems more robust since it's definitely missing in a worker.

I see... How about changing the name of the variable then?

var isWorker = typeof document === 'undefined';

Thanks!

However, my original question still stands. We are no longer going to use OffscreenCanvas for resizing in non-worker scenarios?

I mean, the problem seems to be that ImageBitmap needs flipping, but I do not understand why this PR also reverts to not using OffscreenCanvas in non-worker scenarios.

#15805 should enable texture resizing in workers. I though it's better to ensure the new code path is exclusively used in workers so the previous behavior of resizeImage() is not affected.

Well, maybe I'm too careful in that regard^^. I'm just not 100% sure of side effects if the method always returns resized images of type ImageBitmap when OffscreenCanvas is available.

I see. Yeah, lets not give up on OffscreenCanvas just yet. If we find problems with it we can report to browsers. Lets just flip the image for now.

@mrdoob Ok, the latest commit switches back to OffscreenCanvas by default, and flips the texture in this case, which was the initial issue. Is that ok?

mrdoob · 2019-03-06T07:36:51Z

src/renderers/webgl/WebGLTextures.js

 	//

-	var useOffscreenCanvas = typeof OffscreenCanvas !== 'undefined';
+	var useOffscreenCanvas  = typeof OffscreenCanvas  !== 'undefined';


Minor: Extra spaces added.

mrdoob · 2019-03-11T18:26:38Z

I did more research on this over the weekend. Also in conversations with the WebGL team at Chrome. It's not clear if ImageBitmap being upside down is correct or not. Maybe it's a big in Chrome.

Until that is resolved, I realised we can just pass the OffscreenCanvas to gl directly.

Can you change this PR so it just changes this line from this:

return useOffscreenCanvas ? canvas.transferToImageBitmap() : canvas;

To this?

return canvas;

merwaaan · 2019-03-11T18:41:27Z

@mrdoob Sure, I'll do that.

Is the conversation about ImageBitmap being upside down taking place in a public forum/issue tracker?

merwaaan · 2019-03-11T19:11:15Z

@mrdoob: I just tested @Mugen87 's fiddle again to make sure verything works and it seems like the image is not upside down anymore. Maybe fixed by a Chrome update?

mrdoob · 2019-03-11T19:15:52Z

Is the conversation about ImageBitmap being upside down taking place in a public forum/issue tracker?

Not yet.

@mrdoob: I just tested @Mugen87 's fiddle again to make sure verything works and it seems like the image is not upside down anymore. Maybe fixed by a Chrome update?

What browser/os did you try that on? I can still reproduce in Chrome/OSX.

mrdoob · 2019-03-11T19:17:11Z

src/renderers/webgl/WebGLTextures.js

+
+				// ImageBitmap is flipped vertically
+
+				if ( useOffscreenCanvas ) {


This block of code is not needed now because we're are now returning the canvas.

Please ignore what I said about the issue disappearing. I got confused. What fixed it was just returning the canvas instead of the ImageBitmap.

mrdoob · 2019-03-11T19:27:34Z

Thanks!

takahirox · 2019-03-11T19:51:18Z

I haven't perfectly read through the conversation here and in #15896 but I just want to comment that returning canvas may be safer as this PR finally goes than converting to ImageBitmap.

As I'm adding a note to ImageBitmapLoader document #15945, ImageBitmap flipY and premultiplyAlpha configuration need to be specified on bitmap creation, not on uploading to GPU. This means, texture.flipY/.premultiplyAlpha will be ignored. It may be confusing to users. If non pot regular image is implicitly converted to ImageBitmap inside of renderer, users will be surprised to see that changing texture.flipY/.premultiplyAlpha and then setting texture.needsUpdate=true has no effect.

The upside down problem mentioned above might come from this? I guess OffscreenCanvas.transferToImageBitmap() might generate ImageBitmap with imageOrientation = 'none' (non flipY) so if texture.flipY is true the texture will be upside down?

mrdoob · 2019-03-11T20:48:36Z

@takahirox That is indeed the problem. Thanks for confirming and clarifying 🙏

takahirox · 2019-03-11T21:13:30Z

Similarly saying, internally converting ImageBitmap to canvas might be confusing to users because of different texture.flipY/.premultiplyAlpha behaviors.

BTW, how ImageBitmap created with imageOrientation = 'flipY' is rendered to 2D canvas? Upside down? Or not? I need to confirm the specification but encouraging POT ImageBitmap maybe sounds reasonable and less problematic so far...

Update: perhaps it should be rendered upside down because ImageBitmap and its option isn't specific to WebGL. I'll look into more...

mrdoob · 2019-03-11T21:37:55Z

@takahirox Just to clarify... The original reason we added OffscreenCanvas was to support Workers. No document in Workers.

takahirox · 2019-03-12T07:46:56Z

Yes, I know. I mean, the issue that non-POT ImageBitmap is implicitly converted to canvas in renderer and can be upside down has been around since before we introduced OffscreenCanvas for resizing in Worker. I'll clarify the problem and open a new issue.

takahirox · 2019-04-03T22:16:21Z

Opened an issue #16144

merwaaan added 2 commits March 4, 2019 20:09

Fix non power-of-2 textures being flipped when resized

b2e7559

Format code

3c5a5eb

Mugen87 approved these changes Mar 5, 2019

View reviewed changes

mrdoob reviewed Mar 5, 2019

View reviewed changes

merwaaan added 3 commits March 5, 2019 21:54

Texture resizing: rename useOffscreen to isWorker for clarity

1c0eefa

Texture resizing: back to OffscreenCanvas by default

2397c7b

Texture resizing: fix typo

0b40596

mrdoob reviewed Mar 6, 2019

View reviewed changes

Texture resizing: format code

80d2073

Mugen87 approved these changes Mar 6, 2019

View reviewed changes

mrdoob added this to the r103 milestone Mar 11, 2019

Texture resizing: just return the canvas object

b6d472b

mrdoob reviewed Mar 11, 2019

View reviewed changes

Texture resizing: remove image flipping

508085d

mrdoob merged commit 7d387c3 into mrdoob:dev Mar 11, 2019


		// ImageBitmap is flipped vertically

		if ( useOffscreenCanvas ) {

Uh oh!

Fix non power-of-2 textures being flipped when resized #15904

Fix non power-of-2 textures being flipped when resized #15904

Uh oh!

Conversation

merwaaan commented Mar 4, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mrdoob commented Mar 5, 2019

Uh oh!

Mugen87 commented Mar 5, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mrdoob Mar 5, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Mugen87 Mar 5, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mrdoob commented Mar 11, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

merwaaan commented Mar 11, 2019

Uh oh!

merwaaan commented Mar 11, 2019

Uh oh!

mrdoob commented Mar 11, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mrdoob commented Mar 11, 2019

Uh oh!

takahirox commented Mar 11, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mrdoob commented Mar 11, 2019

Uh oh!

takahirox commented Mar 11, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mrdoob commented Mar 11, 2019

Uh oh!

takahirox commented Mar 12, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

takahirox commented Apr 3, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

merwaaan commented Mar 4, 2019 •

edited

Loading

mrdoob Mar 5, 2019 •

edited

Loading

Mugen87 Mar 5, 2019 •

edited

Loading

mrdoob commented Mar 11, 2019 •

edited

Loading

takahirox commented Mar 11, 2019 •

edited

Loading

takahirox commented Mar 11, 2019 •

edited

Loading

takahirox commented Mar 12, 2019 •

edited

Loading