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

[Bug] Cannot display Float32 images on iOS Safari #3027

Open
abustany opened this issue Mar 4, 2024 · 12 comments
Open

[Bug] Cannot display Float32 images on iOS Safari #3027

abustany opened this issue Mar 4, 2024 · 12 comments
Labels
type: bug 🐞 Errors in functionality

Comments

@abustany
Copy link

abustany commented Mar 4, 2024

Bug description

Note: this bug originally comes from cornerstonejs/cornerstone3D#1133

Cornerstone loads grayscale DICOM images into Float32Array pixel data. Putting this data into a dataArray/imageData/imageMapper/imageSlice fails to display on an iPhone XS running iOS17 (and couple more iPhone models I could check using Browserstack).

Steps to reproduce

https://stackblitz.com/edit/vitejs-vite-gjcs6u?file=src%2Fmain.ts

Detailed Behavior

Image renders correctly on:

  • desktop browsers (tested latest firefox+chrome on an intel haswell gpu)
  • firefox for android

Image fails to render on:

  • safari on an iPhone XS running iOS17

Expected Behavior

Image should render identically on all platforms

Environment

  • vtk.js version: 29.7.3
  • Browsers: Safari
  • OS: iOS 17
@abustany abustany added the type: bug 🐞 Errors in functionality label Mar 4, 2024
@abustany
Copy link
Author

abustany commented Mar 4, 2024

Safari on iOS doesn't report the OES_texture_float_linear extension, which Firefox on my desktop reports... What this extension enables seems pretty close to what's done in the OpenGL implementation of Texture's create2DFromRaw. That correlates pretty well with the fact that if I change Float32Array to Uint8Array in the stackblitz posted above, the code works on iOS. There's a fallback for create3DFilterableFromDataArray, but not in create2DFilterableFromDataArray, is there a good reason for this?

@abustany
Copy link
Author

abustany commented Mar 4, 2024

ha, actually the code produces a warning "failed to load OES_text_float_linear", but because I don't have access to the web inspector on the iPhone I couldn't see it 😅

@abustany
Copy link
Author

abustany commented Mar 4, 2024

A question I'm asking myself about the fallback mentioned above: is it ever desirable to truncate the items of a Float32Array to Uint8? If this happens "under the hood", magically, how can the client know and about it and adapt the window values accordingly?

@abustany
Copy link
Author

abustany commented Mar 4, 2024

I'm using the following patch for now, which seems to fix things for the images I care about. I'm not sure if it's good enough as a general solutions though... The patch is against the built version of vtk.js, so it's compatible with yarn patch or pnpm patch:

diff --git a/Rendering/OpenGL/Texture.js b/Rendering/OpenGL/Texture.js
index a48e383723bf3e32afaf03f5e7c6d866b3e4cbbd..b1e091d9f27d2b8a77de63a14160775702cb3c67 100644
--- a/Rendering/OpenGL/Texture.js
+++ b/Rendering/OpenGL/Texture.js
@@ -932,8 +932,8 @@ function vtkOpenGLTexture(publicAPI, model) {
   }
   function processDataArray(dataArray, preferSizeOverAccuracy) {
     const numComps = dataArray.getNumberOfComponents();
-    const dataType = dataArray.getDataType();
-    const data = dataArray.getData();
+    let dataType = dataArray.getDataType();
+    let data = dataArray.getData();
 
     // Compute min max from array
     // Using the vtkDataArray.getRange() enables caching
@@ -955,6 +955,12 @@ function vtkOpenGLTexture(publicAPI, model) {
     if (!model.useHalfFloat) {
       publicAPI.getOpenGLDataType(dataType, true);
     }
+
+    if (dataType === VtkDataTypes.FLOAT && !!model.oglNorm16Ext && !model.context.getExtension('OES_texture_float_linear') && !minArray.some(x => x < -32767) && !maxArray.some(x => x > 32767)) {
+      data = new Int16Array(data);
+      dataType = VtkDataTypes.SHORT;
+    }
+
     return {
       numComps,
       dataType,

The code should probably be moved to updateArrayDataType too.

@gosvig123
Copy link

gosvig123 commented Mar 5, 2024

@abustany, thanks for all the context it is super useful!

  • A few questions that i wanted to check on is if you have been having the same issues with android phones or not at all?

  • And then i am curious if you are using Vite for your actual project (i am just going off the Stackblitz)

  • Finally i just wanted to check if this is for working with volume viewports or stack, or both that you are experiencing these issues (I am working with volume.)

Thanks for looking into it and hope you might be able to share some additional info - either way have a great day!

@finetjul
Copy link
Member

finetjul commented Mar 5, 2024

@abustany , Thanks for reporting the issue and your investigation. You might want to create PR.

@sankhesh WDYT ?

@sankhesh
Copy link
Collaborator

sankhesh commented Mar 5, 2024

Safari on iOS doesn't report the OES_texture_float_linear extension

Floating point textures have limited support for Safari on iOS. See https://developer.mozilla.org/en-US/docs/Web/API/OES_texture_float_linear

I'm using the following patch for now,

In your patch you're choosing the type based on value range. Instead, I would cast the data to short if the extension is not available and then calculate appropriate shift and scale values for the cast from short back to float in the shaders.

@abustany
Copy link
Author

abustany commented Mar 5, 2024

@sankhesh thanks for the feedback! Yes, that would be better, but then I need to scale the window/levels based on those shift/scale values in the shader too right? The model you describe is superior, but sounded a bit more complex to implement → I didn't go down that route because I wanted a quick fix yesterday 😬 I might have more time after this month to try to implement a proper fix, but if the current fix is "good enough" for my usecase I can't guarantee I'll have time to implement that proper fix (1-person engineering team here 😅).

@abustany
Copy link
Author

abustany commented Mar 5, 2024

@abustany, thanks for all the context it is super useful!

  • A few questions that i wanted to check on is if you have been having the same issues with android phones or not at all?

The Pixel 3a I tested on supported float textures, but I suppose this will vary across devices.

  • And then i am curious if you are using Vite for your actual project (i am just going off the Stackblitz)

Yes, we are using Vite. It works just fine with VTK, but will give you headaches with Cornerstone for various reasons: circular dependencies that Rollup can't bundle correctly, webpack-specific web worker loading, and broken webpack automatic public path detection. There are workarounds for all of those, happy to help if needed.

  • Finally i just wanted to check if this is for working with volume viewports or stack, or both that you are experiencing these issues (I am working with volume.)

I only use stack images so far, so I can't speak for volumes sorry 😬

@sedghi
Copy link
Contributor

sedghi commented Apr 17, 2024

Based on my investigation the issue is Float Linear interpolation (OES_texture_float_linear) is not supported on iOS devices and was incorrectly reported as supported up until recently in iOS 17

https://bugs.webkit.org/show_bug.cgi?id=264404#c2

@abustany
Copy link
Author

hmm the original bug reports was done on iOS 17, which does not report this extension as far as I remember. As you can see in the hacky patch I sent above, there's a check for OES_texture_float_linear. If I'm not mistaken, the issue is rather that vtk.js does not do the proper checks in that specific part of the code, and sends a Float32 texture to the GPU - but it's been couple of weeks since I dealt with this issue so I might remember it wrong.

@sedghi
Copy link
Contributor

sedghi commented Apr 17, 2024

I created this PR which would be good enough for a lot of use cases (except the PT Scaled data)

cornerstonejs/cornerstone3D#1212

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: bug 🐞 Errors in functionality
Projects
None yet
Development

No branches or pull requests

5 participants