Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Resize textures for iOS in low material quality mode. #5437

Merged
merged 1 commit into from
Jul 19, 2022

Conversation

takahirox
Copy link
Contributor

@takahirox takahirox commented May 16, 2022

From: #5295

Note: This change description is outdated. See #5437 (comment) and the following comments for update.

Resize huge textures up to 512x512 (Update: I changed to 1024x1024) for low material quality mode on HubsMeshBasicMaterial creation, for

  • Better reliability on iOS devices. Currently browsers on iOS can crash with huge textures [iOS] The page is refreshed when uploading certain objects from Sketchfab #4669
  • Better performance. Users choose the low material quality mode for better performance. But fetching huge textures may be costly.
  • Shorter main thread blocking time. It seems the main thread blocking time caused by texture image resize + smaller texture upload than the one caused by huge texture upload.

As written in #5295 (comment) I want to start with resizeing textures only for low material quality mode on HubsMeshBasicMaterial creation for simplicity and quickly fixing #4669. I hope we can think of further texture resizing optimization later if needed.

@takahirox
Copy link
Contributor Author

takahirox commented May 18, 2022

Visual quality with resized 512x512 textures looks worse than I expected. Perhaps it isn't acceptable even for low material quality mode.

image

image

Ideally it's the best if we come up with a technique that can resize the textures while keeping the quality. (We need smoothing and/or uv adjustment?)

But what I really want to quickly fix is #4669 , I want to think of better texture resizing later if needed.

Instead I'm thinking of relaxing the maximum texture size for the low quality mode, like from 512x512 (all textures must be small enough) to 1024x1024 or 2048x2048 (too huge textures must be avoided).

@takahirox
Copy link
Contributor Author

Changed from 512x512 to 1024x1024

image

Looks ok so far in this scene (tho I'm not really sure what textures in the scene are resized).

@takahirox
Copy link
Contributor Author

We internally discussed in the team. Resized textures visual quality may not be acceptable especially if scene authors tune well and it's much worse than author's expectation.

But we also agree that assets from Sketchfab may not be optimized.

So we are thinking of resizing textures only the assets imported from Sketchfab so far.

@takahirox
Copy link
Contributor Author

In today's team meeting in Hubs, I encountered the unexpected page refresh problem on my iPhoneX.

Probably no assets is imported from Sketchfab. And I can enter the room if no one there. I speculate some avatars were not optimized well.

So... I'm thinking what we should do for the problem. Ideally we create a simple test reproducing the problem and push the iOS or Safari team to fix the problem. Until the problem will be fixed on iOS or Safari end, what should the workaround be?

@takahirox takahirox marked this pull request as draft June 9, 2022 18:39
@takahirox
Copy link
Contributor Author

Let me switch this PR to draft for now because I started to worry if this feature or configuration is good because

  • The resized texture visual quality is worse than my expectation
  • Even if textures are resized only for assets from Sketchfab the problem can still happen
  • This PR forces the texture resize in low material quality mode. But we might want to think of adding a new preference max texture size.

@takahirox
Copy link
Contributor Author

Rethinking the issue because I encountered the problem on an iOS device again...

I'm thinking if we can resize huge textures to 2K textures in the low material quality mode only on iOS devices (regardless of whether assets from Sketchfab or not).

Some thoughts.

  • Crashing problem on iOS is a serious problem. For example if someone places an asset with huge textures all the iOS devices in the room can crash. We may need a workaround.
  • We already reported the root issue to webkit dev. We may be able to expect that the problem can be fixed on webkit end. The workaround will be removed sooner or later. So I don't want to add a complex workaround.
  • Not distinguishing assets whether from Sketchfab or not can make the workaround simpler.
  • Resizing to 2K textures may not be a perfect solution. But it should metigate the problem. It would be good as workaround.
  • Resizing textures can affect visual quality. But (I hope) 2K may be big enough. And doing it only on iOS devices can limit the devices affected by the workaround.
  • At least, a bit worse visual quality should be better than broken app.

@wsxiaoys
Copy link
Contributor

Adding a few cases this PR won't solve (but still causing problem on mobile iOS) for consideration:

  1. Large resolution image implemented by a-image still causes crash.
  2. Large resolution pdf will not render (black plane)

@takahirox
Copy link
Contributor Author

Then resizing textures in HubsTextureLoader may be better? I assume all textures are loaded via it, but not sure if it's true.

@takahirox takahirox marked this pull request as ready for review June 29, 2022 01:05
@takahirox
Copy link
Contributor Author

Updated the PR to force to resize huge textures to 2K textures in HubsTextureLoader in the low material quality mode on iOS devices.

Browsers on iOS devices can crash if huge or many textures are
used in a room. Perhaps huge VRAM usage may cause it. We already
reported it to the webkit devs.

Until the root issue is fixed on iOS end, we resize huge texture
images in the low material quality mode as workaround. It isn't
a perfect solution but it should mitigate the problem.

Also disable image bitmap for iOS because texture image resize +
image bitmap can still even cause the problem. Disabling it
seemt to further mitigate the problem.
@takahirox takahirox changed the title Resize textures for low material quality mode. Resize textures for iOS in low material quality mode. Jun 29, 2022
Copy link
Contributor

@netpro2k netpro2k left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still not in love with doing this but like you said I guess its better than the app crashing... I think the issue mentioned in the thread about a-image no longer applies since that has since been removed from our aframe, so everything should be going through our texture loader.

While reviewing I dug into ThreeJS since I remember that it had texture resizing code. It does resize if textures are > gl.MAX_TEXTURE_SIZE. Another options for doing this patch would be to override WebGLCapabilities in our fork to report a lower MAX_TEXTURE_SIZE on iOS. Not sure if I like that solution better but figured i would mention it as an option.

I also wonder what the typical MAX_TEXTURE_SIZE is on an iOS device, since we would have already been resizing anything larger than that anyway.

const newWidth = Math.round(image.width * conversionRatio);
const newHeight = Math.round(image.height * conversionRatio);

const canvas = document.createElement("canvas");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We may want to reuse this cavnas/context and just resize it instead of making anew one for each texture. (I noticed the code in ThreeJS for resizing textures does this).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

canvas is set to texture.image. If we reuse canvas, doesn't the update affect all the textures having resized images?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah yeah, since we aren't directly uploading it to the GPU we can't reuse it.

@takahirox
Copy link
Contributor Author

I also wonder what the typical MAX_TEXTURE_SIZE is on an iOS device,

16K as far as I know

@takahirox
Copy link
Contributor Author

I guess its better than the app crashing...

Yes, it should be much better. Actually I rarely be able to join our meeting and meetup rooms on my iOS devices due to this problem. And 2K may be big enouch. It may not affect many assets.

While reviewing I dug into ThreeJS since I remember that it had texture resizing code. It does resize if textures are > gl.MAX_TEXTURE_SIZE. Another options for doing this patch would be to override WebGLCapabilities in our fork to report a lower MAX_TEXTURE_SIZE on iOS.

Hm, honestly I don't really agree with that option. (But thanks for mentioning the option anyways.) I don't think we should really rely on Three.js internal implementation. It's an internal design. The behavior might be able to change in future revisions without any notice (internal change may not appear in the migration guide.) Overriding capabilities might have similar risk. In the future revision it might cause unexpected side effects.

Resizing image code is just about 50 lines. It may not be a big deal. I think having the 50 lines in our code is less problem and better maintainability.

If we want to avoid duplicated code with Three.js core, the best option may suggest to Three.js upstream for exposing the resize dunction.

@wsxiaoys
Copy link
Contributor

wsxiaoys commented Jul 1, 2022

Still not in love with doing this but like you said I guess its better than the app crashing... I think the issue mentioned in the thread about a-image no longer applies since that has since been removed from our aframe, so everything should be going through our texture loader.

Great to know a-image is removed!

while it seems media-pdf is still loading texture by its own. It won't cause crash on iOS device, but the content become black due to large dimension

@takahirox
Copy link
Contributor Author

takahirox commented Jul 1, 2022

If I remember correctly, the black texture problem was caused on iOS with image bitmap. But I heard it has been fixed in the latest iOS. (I have bad memory. Sorry if I'm wrong.) Do you use the latest iOS? Even if it can happen in the latest iOS, disabling image bitmap may avoid or mitigate the problem.

@wsxiaoys
Copy link
Contributor

wsxiaoys commented Jul 1, 2022

I'm on iPhone 12 Pro Max 15.5.

I haven't tried disabling image bitmap, but this MR fixed the pdf rendering issue for me on an 1920x1080 resolution pdf: #5554

@takahirox
Copy link
Contributor Author

Sounds like the same issue as

But the root issue seems to have been fixed on iOS end. If you can still reproduce the problem, first we should report to the iOS team.

Can you make and share a room that you can reproduce the problem?

@takahirox takahirox merged commit 778e02c into master Jul 19, 2022
@takahirox takahirox deleted the ResizeTextures branch July 19, 2022 21:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants