feat: option to use Arraybuffer for volume loader instead of sharedArrayBuffer #358

Ouwen · 2023-01-11T10:01:37Z

This feature allows for volumes to be loaded even if the page is not cross origin isolated.
For some use cases such as running cornerstone3D in cordova or mobile webview this is required.

Ouwen · 2023-01-11T10:02:45Z

@sedghi what are your thoughts on this?

netlify · 2023-01-11T10:26:26Z

✅ Deploy Preview for cornerstone-3d-docs ready!

Name	Link
🔨 Latest commit	`a78c285`
🔍 Latest deploy log	https://app.netlify.com/sites/cornerstone-3d-docs/deploys/63ca06cd5e763d00082ac7e1
😎 Deploy Preview	https://deploy-preview-358--cornerstone-3d-docs.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

sedghi · 2023-01-11T12:48:27Z

packages/core/src/utilities/createFloat32SharedArray.ts

+  const buffer = useSharedArrayBuffer
+    ? new SharedArrayBuffer(length * 4)
+    : new ArrayBuffer(length * 4);

-  return new Float32Array(sharedArrayBuffer);
+  return new Float32Array(buffer);


I don't think you should do it here. This file should do what it says (creating sharedArray). Maybe in the cornerstoneStreamingImageVolume you branch to use non-shared

Sounds good I can make that change

sedghi · 2023-01-11T13:00:06Z

packages/streaming-image-volume-loader/src/StreamingImageVolume.ts

          offset: imageIdIndex * lengthInBytes,
          length,
          type,
        },
        skipCreateImage: true,
+        transferPixelData: arrayBuffer instanceof ArrayBuffer,


I was about to fix this in another PR. Do you need it here? since I prefer it to be a separate fix: broken transferPixelData for both volume and stack viewport.

Does it hurt to set transferPixelData to true for SharedArrayBuffer too? although I guess it is shared by construct ..?

I can remove this for your fix. It shouldn’t cause any issue to copy rather than transfer the encoded pixel array data

sedghi · 2023-01-11T13:00:46Z

packages/streaming-image-volume-loader/src/StreamingImageVolume.ts

@@ -415,12 +414,14 @@ export default class StreamingImageVolume extends ImageVolume {
      const options = {
        // WADO Image Loader
        targetBuffer: {
-          arrayBuffer,
+          arrayBuffer:
+            arrayBuffer instanceof ArrayBuffer ? undefined : arrayBuffer,


Can you please add a comment to explain why we need it for sharedArrayBuffer (since the .set is happening inside the worker in cswil), but not for the arrayBuffer since it happens down below?

Sure, I’ll add something like: keeping this in the options means a large empty volume array buffer will be transferred to the worker.

This is undesirable for streaming volume without shared array buffer because the target is now an empty 300-500MB volume array buffer. Instead the volume should be progressively set in the main thread.

sedghi · 2023-01-11T13:01:27Z

packages/streaming-image-volume-loader/src/StreamingImageVolume.ts

+            const scalarData = this.scalarData;
+            if (scalarData.buffer instanceof ArrayBuffer) {
+              const offset = options.targetBuffer.offset; // in bytes
+              const length = options.targetBuffer.length; // in frames
+              try {
+                if (scalarData instanceof Float32Array) {
+                  const floatView = new Float32Array(image.pixelData);
+                  if (floatView.length !== length) {
+                    throw 'Error pixelData length does not match frame length';
+                  }
+                  scalarData.set(floatView, offset / 4);
+                }
+                if (scalarData instanceof Uint8Array) {
+                  const intView = new Uint8Array(image.pixelData);
+                  if (intView.length !== length) {
+                    throw 'Error pixelData length does not match frame length';
+                  }
+                  scalarData.set(intView, offset);
+                }
+              } catch (e) {
+                console.error(e);
+              }
+            }


These all better be refactor into its own function e.g., handleArrayBufferLoad or somthing similar

if (scalarData.buffer instanceof ArrayBuffer) { handleArrayBufferLoad() } successCallback...

sedghi · 2023-01-11T13:02:53Z

utils/ExampleRunner/example-runner-cli.js

@@ -179,7 +179,7 @@ if (configuration.examples) {
    // console.log('conf', conf);
    shell.ShellString(conf).to(webpackConfigPath);
    shell.cd(exBasePath);
-    shell.exec(`webpack serve --progress --config ${webpackConfigPath}`);
+    shell.exec(`webpack serve --host 0.0.0.0 --progress --config ${webpackConfigPath}`);


Does this make it run on 0.0.0.0 instead of localhost:3000?

It will run on localhost and bind ip address so mobile devices on the same network can access.

sedghi

This looks great as first iteration.
The only part I'm not certain right now is the global setting for sharedArrayBuffer usage in the init. Hmmmm, Do we want it there? I'm not really sure I like the idea of people fallbacking to ArrayBuffer (instead of more performant sharedArrayBuffer) if they have not set the headers correctly. They can just debug the error for cross origin and fix their headers and enjoy the performant path.

Do you think we put it at the volume loader options? or maybe just a boolean at the init instead of checking new SharedArrayBuffer(0) for those who know what they are doing

CC @wayfarer3130

Ouwen · 2023-01-11T15:40:29Z

@sedghi one interesting find. I've been testing this across different devices desktop chrome, safari, android chrome, and iOS safari. They all work well except iOS safari which occasionally crashes the worker. On a single worker, either all slices will load properly without a crash. Or, after successful initialization, the worker throws a "Range Error: Out of Memory".

Refreshing the page after waiting about 10 seconds usually fixes the problem and all slices load in. Have you seen this behavior before?

Ouwen · 2023-01-11T23:17:23Z

After some more investigation. It seems like the iOS safari behavior is due to something with web worker garbage collection. Hard refreshing iOS will have 1 web worker properly spawn; however, follow on web workers will fail with a generic "Range Error: Out of Memory".

With just one web worker, we can refresh the page within safari and have the volume properly load. However, the web worker will occasionally run into some error and crash (not loading any slices). A hard refresh (hard close safari and refresh) causes the page to function again.

The consistent finding is that the first load after a hard refresh will always work.

abluobo · 2023-01-12T02:40:04Z

@Ouwen Will the memory occupied by the browser increase after the Arraybuffer replaces the sharedArrayBuffer?

Ouwen · 2023-01-12T16:10:54Z

The browser memory creates an empty volume array. Let's say it is 100MB or so.
The workers will decode image pixel data and set a 1MB slice inside the 100MB volume.

The main difference is that without sharedArrayBuffer the workers have to transfer the 1MB decoded slice back into the main thread. The main thread then performs the set.

With sharedArrayBuffer, the workers can set directly into the 100MB sharedArrayBuffer

sedghi · 2023-01-13T19:36:41Z

@Ouwen I talked to Bill, and we think useSharedArrayBuffer should have three options true, false, auto with default of true. What do you think?

Ouwen · 2023-01-14T15:59:04Z

Yeah I think that makes sense.

Ouwen · 2023-01-14T16:01:06Z

After some more investigation. It seems like the iOS safari behavior is due to something with web worker garbage collection. Hard refreshing iOS will have 1 web worker properly spawn; however, follow on web workers will fail with a generic "Range Error: Out of Memory".

With just one web worker, we can refresh the page within safari and have the volume properly load. However, the web worker will occasionally run into some error and crash (not loading any slices). A hard refresh (hard close safari and refresh) causes the page to function again.

The consistent finding is that the first load after a hard refresh will always work.

This is unrelated to the non shared array buffer implementation and is an issue with loading codecs on iOS safari.

cornerstonejs/cornerstoneWADOImageLoader#506

…rayBuffer

Ouwen · 2023-01-15T04:32:57Z

@sedghi, ready for review.

sedghi · 2023-01-16T18:07:51Z

@Ouwen One of the tests are failing. You can force run it via changing it to fit and describe to fdescribe just FYI. Hopefully it will fail once you run it in isolation, if not there is some leaking happening between tests (or in source)

sedghi

Looks great !

Ouwen · 2023-01-20T05:48:10Z

Thanks @sedghi

Ouwen force-pushed the gradienthealth/no_shared_array_buffer branch from c527ec8 to 90471bf Compare January 11, 2023 10:26

sedghi reviewed Jan 11, 2023

View reviewed changes

sedghi requested changes Jan 11, 2023

View reviewed changes

sedghi changed the title ~~feat: no use shared array buffer~~ feat: option to use Arraybuffer for volume loader instead of sharedArrayBuffer Jan 11, 2023

sedghi mentioned this pull request Jan 11, 2023

is there any solution can use volume without cross-origin isolated #295

Open

Ouwen force-pushed the gradienthealth/no_shared_array_buffer branch from 90471bf to e0e3e00 Compare January 11, 2023 20:08

Ouwen force-pushed the gradienthealth/no_shared_array_buffer branch 2 times, most recently from 0cbaefb to d58ab7c Compare January 15, 2023 01:58

feat: option to use Arraybuffer for volume loader instead of sharedAr…

85098d8

…rayBuffer

Ouwen force-pushed the gradienthealth/no_shared_array_buffer branch from d58ab7c to 85098d8 Compare January 15, 2023 04:31

sedghi added 2 commits January 19, 2023 22:09

fix build

527f7b0

use the init flag instead of window

a78c285

sedghi approved these changes Jan 20, 2023

View reviewed changes

sedghi merged commit ab8237c into cornerstonejs:main Jan 20, 2023

Ouwen deleted the gradienthealth/no_shared_array_buffer branch February 7, 2023 05:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: option to use Arraybuffer for volume loader instead of sharedArrayBuffer #358

feat: option to use Arraybuffer for volume loader instead of sharedArrayBuffer #358

Ouwen commented Jan 11, 2023

Ouwen commented Jan 11, 2023

netlify bot commented Jan 11, 2023 •

edited

Loading

sedghi Jan 11, 2023

Ouwen Jan 11, 2023

sedghi Jan 11, 2023

Ouwen Jan 11, 2023

sedghi Jan 11, 2023

Ouwen Jan 11, 2023

sedghi Jan 11, 2023

sedghi Jan 11, 2023

sedghi Jan 11, 2023

Ouwen Jan 11, 2023

sedghi left a comment •

edited

Loading

Ouwen commented Jan 11, 2023

Ouwen commented Jan 11, 2023

abluobo commented Jan 12, 2023

Ouwen commented Jan 12, 2023

sedghi commented Jan 13, 2023

Ouwen commented Jan 14, 2023

Ouwen commented Jan 14, 2023

Ouwen commented Jan 15, 2023 •

edited

Loading

sedghi commented Jan 16, 2023

sedghi left a comment

Ouwen commented Jan 20, 2023

feat: option to use Arraybuffer for volume loader instead of sharedArrayBuffer #358

feat: option to use Arraybuffer for volume loader instead of sharedArrayBuffer #358

Conversation

Ouwen commented Jan 11, 2023

Ouwen commented Jan 11, 2023

netlify bot commented Jan 11, 2023 • edited Loading

✅ Deploy Preview for cornerstone-3d-docs ready!

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sedghi left a comment • edited Loading

Choose a reason for hiding this comment

Ouwen commented Jan 11, 2023

Ouwen commented Jan 11, 2023

abluobo commented Jan 12, 2023

Ouwen commented Jan 12, 2023

sedghi commented Jan 13, 2023

Ouwen commented Jan 14, 2023

Ouwen commented Jan 14, 2023

Ouwen commented Jan 15, 2023 • edited Loading

sedghi commented Jan 16, 2023

sedghi left a comment

Choose a reason for hiding this comment

Ouwen commented Jan 20, 2023

netlify bot commented Jan 11, 2023 •

edited

Loading

sedghi left a comment •

edited

Loading

Ouwen commented Jan 15, 2023 •

edited

Loading