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] Regression: loadImage is failing for a particular slice #908

Closed
sgielen opened this issue Nov 22, 2023 · 12 comments
Closed

[Bug] Regression: loadImage is failing for a particular slice #908

sgielen opened this issue Nov 22, 2023 · 12 comments

Comments

@sgielen
Copy link
Contributor

sgielen commented Nov 22, 2023

Describe the Bug

This slice loaded normally in cornerstone legacy, but no longer in cornerstone 3d. Another slice of the same volume works normally.

Steps to Reproduce

  1. Download this .zip file and unzip it:
    broken-slice.zip
  2. yarn install
  3. yarn dev
  4. browse to the indicated address
  5. observe the console

The current behavior

The following messages & error is displayed:
Scherm­afbeelding 2023-11-22 om 13 53 53

Uncaught (in promise) TypeError: B is not a constructor
    at q (f5e43be9-e9a4-4f29-b5e8-5fab295d2cee:1:1209505)
    at f5e43be9-e9a4-4f29-b5e8-5fab295d2cee:1:1211882
    at f5e43be9-e9a4-4f29-b5e8-5fab295d2cee:1:1211891
    at b (f5e43be9-e9a4-4f29-b5e8-5fab295d2cee:1:1212572)
    at async Object.handler (f5e43be9-e9a4-4f29-b5e8-5fab295d2cee:1:1212845)

The expected behavior

Both images load normally, and loadImage does not throw.

OS

macOS 14.0

Node version

20.8.1

Browser

Chrome 119.0.6045.159

@sgielen
Copy link
Contributor Author

sgielen commented Nov 22, 2023

The issue seems to be gone when I set preScale.enabled to false, but this then conflicts with #887

@sgielen
Copy link
Contributor Author

sgielen commented Nov 22, 2023

The B is not a constructor error seems to come from this call to new B(g.pixelData.length):

Scherm­afbeelding 2023-11-22 om 14 04 15

Which, based on proximity, seems to be a minimized version of this call to new TypedArrayConstructor here:

function _getDefaultPixelDataArray(min, max, imageFrame) {
const TypedArrayConstructor = getPixelDataTypeFromMinMax(min, max);
const typedArray = new TypedArrayConstructor(imageFrame.pixelData.length);
typedArray.set(imageFrame.pixelData, 0);
return typedArray;
}
function _validateScalingParameters(scalingParameters) {
if (!scalingParameters) {
throw new Error(
'options.preScale.scalingParameters must be defined if preScale.enabled is true, and scalingParameters cannot be derived from the metadata providers.'
);
}
}

@sgielen
Copy link
Contributor Author

sgielen commented Nov 22, 2023

Indeed, stopping in the debugger, B is undefined at this point:
Scherm­afbeelding 2023-11-22 om 14 08 27

@sgielen
Copy link
Contributor Author

sgielen commented Nov 22, 2023

So min being -8192 and max being 43746, getPixelDataTypeFromMinMax has pixelDataType being undefined (since max is larger than 32767), but then it should return Float32Array in this case, which it doesn't seem to do

@sgielen
Copy link
Contributor Author

sgielen commented Nov 22, 2023

So it looks like an issue with minification. If I run getPixelDataTypeFromMinMax exactly as copied from the source, it returns the Float32Array constructor as expected. But if I run the minified version as copied from the browser devtools, it returns undefined.

// f Float32Array() { [native code] }
console.log(getPixelDataTypeFromMinMax(-8192, 43746));
const B = function(A, I) {
                let g;
                return Number.isInteger(A) && Number.isInteger(I) ? A >= 0 ? I <= 255 ? g = Uint8Array : I <= 65535 && (g = Uint16Array) : A >= -128 && I <= 127 ? g = Int8Array : A >= -32768 && I <= 32767 && (g = Int16Array) : g = Float32Array,
                g
            }(-8192, 43746)
// undefined
console.log(B);

@sgielen
Copy link
Contributor Author

sgielen commented Nov 22, 2023

It looks like if I minify that getPixelDataTypeFromMinMax now, the result is slightly different, namely:

function t(e,n){let r;return Number.isInteger(e)&&Number.isInteger(n)&&(e>=0?n<=255?r=Uint8Array:n<=65535&&(r=Uint16Array):e>=-128&&n<=127?r=Int8Array:e>=-32768&&n<=32767&&(r=Int16Array)),r||Float32Array}

And this properly returns Float32Array when calling it with these min/max. So I think one of the minified pre-builds of cornerstone might be at fault. If I search for the original minified function which ends with :g=Float32Array,g} I can find it in these places:

node_modules/@cornerstonejs/dicom-image-loader/dist/cornerstoneDICOMImageLoaderNoWebWorkers.bundle.min.js
node_modules/@cornerstonejs/dicom-image-loader/dist/cornerstoneDICOMImageLoader.bundle.min.js
node_modules/@cornerstonejs/dicom-image-loader/dist/cornerstoneDICOMImageLoader.bundle.min.js.map
node_modules/@cornerstonejs/dicom-image-loader/dist/index.worker.bundle.min.worker.js
node_modules/@cornerstonejs/dicom-image-loader/dist/cornerstoneDICOMImageLoaderNoWebWorkers.bundle.min.js.map
node_modules/.vite/deps/@cornerstonejs_dicom-image-loader.js
node_modules/.vite/deps/@cornerstonejs_dicom-image-loader.js.map

@sgielen
Copy link
Contributor Author

sgielen commented Nov 23, 2023

This still seems to occur in the newest main: Apologies - scratch that - my main was unexpectedly outdated, see later

packages/dicomImageLoader$ git checkout main
packages/dicomImageLoader$ rm -rf dist
packages/dicomImageLoader$ yarn build
packages/dicomImageLoader$ js-beautify dist/cornerstoneDICOMImageLoader.bundle.min.js | grep -C3 Float32Array
[...]
        function aA(A) {
            const I = function(A, I) {
                let g;
                return Number.isInteger(A) && Number.isInteger(I) ? A >= 0 ? I <= 255 ? g = Uint8Array : I <= 65535 && (g = Uint16Array) : A >= -128 && I <= 127 ? g = Int8Array : A >= -32768 && I <= 32767 && (g = Int16Array) : g = Float32Array, g
            }(A.smallestPixelValue, A.largestPixelValue);
            if (!I) throw new Error("Could not apply a typed array to the pixel data");
            {
[...]

Copy-pasting that const I into a console and running it on my min/max values:

const I = function(A, I) {
                let g;
                return Number.isInteger(A) && Number.isInteger(I) ? A >= 0 ? I <= 255 ? g = Uint8Array : I <= 65535 && (g = Uint16Array) : A >= -128 && I <= 127 ? g = Int8Array : A >= -32768 && I <= 32767 && (g = Int16Array) : g = Float32Array, g
            }
I(-8192, 43746)
// undefined

@sgielen
Copy link
Contributor Author

sgielen commented Nov 23, 2023

Actually, if I disable minification I still get a function that doesn't output properly, so it might not be the whole story --

--- a/packages/dicomImageLoader/.webpack/webpack-bundle.js
+++ b/packages/dicomImageLoader/.webpack/webpack-bundle.js
@@ -100,12 +100,7 @@ module.exports = {
     asyncWebAssembly: true,
   },
   optimization: {
-    // minimize: false,
-    minimizer: [
-      new TerserPlugin({
-        parallel: true,
-      }),
-    ],
+    minimize: false,
   },
   // plugins: [new webpack.ProgressPlugin(), new BundleAnalyzerPlugin()],
 };
packages/dicomImageLoader$ rm -rf dist
packages/dicomImageLoader$ yarn build
packages/dicomImageLoader$ js-beautify dist/cornerstoneDICOMImageLoader.bundle.min.js | grep -C32 Float32Array
[...]
// CONCATENATED MODULE: ./shared/getPixelDataTypeFromMinMax.ts
            function getPixelDataTypeFromMinMax(min, max) {
                let pixelDataType;
                if (Number.isInteger(min) && Number.isInteger(max)) {
                    if (min >= 0) {
                        if (max <= 255) {
                            pixelDataType = Uint8Array;
                        } else if (max <= 65535) {
                            pixelDataType = Uint16Array;
                        }
                    } else {
                        if (min >= -128 && max <= 127) {
                            pixelDataType = Int8Array;
                        } else if (min >= -32768 && max <= 32767) {
                            pixelDataType = Int16Array;
                        }
                    }
                } else {
                    pixelDataType = Float32Array;
                }
                return pixelDataType;
            }

This implementation also returns undefined on my values of (-8192, 43746) and differs from the implementation at https://github.com/cornerstonejs/cornerstone3D/blob/main/packages/dicomImageLoader/src/shared/getPixelDataTypeFromMinMax.ts#L3.

@sgielen
Copy link
Contributor Author

sgielen commented Nov 23, 2023

Oh I finally see now that my main was a bit behind and this was actually fixed very recently, Nov 14 85fd193. I thought I had pulled, but it must have been on a branch

@sgielen
Copy link
Contributor Author

sgielen commented Nov 23, 2023

I see 85fd193 was released in v1.29.0+, but unfortunately when I try to use that I get other errors which might boil down to the known circular dependency issue in Vite:

createImage.ts:155 Uncaught (in promise) TypeError: Cannot destructure property 'use16BitDataType' of 'o2' as it is undefined.
    at kA (createImage.ts:155:11)
    at A3.then.D2.error (loadImage.ts:63:30)

options is undefined here, but if I click through to the getOptions call then that returned options is set properly. So this is blocking me from upgrading to v1.29.0 currently.

@sgielen
Copy link
Contributor Author

sgielen commented Nov 23, 2023

Indeed, if I apply this patch to the v1.21.2 tag:

--- a/packages/dicomImageLoader/src/shared/getPixelDataTypeFromMinMax.ts
+++ b/packages/dicomImageLoader/src/shared/getPixelDataTypeFromMinMax.ts
@@ -24,5 +24,5 @@ export default function getPixelDataTypeFromMinMax(
     pixelDataType = Float32Array;
   }
 
-  return pixelDataType;
+  return pixelDataType || Float32Array;
 }

Then do a rebuild

packages/dicomImageLoader$ rm -rf dist
packages/dicomImageLoader$ yarn build

And then replace the dist files in my node_modules , taking care to remove any caches:

$ cd node_modules/@cornerstonejs/dicom-image-loader
$ rm -rf dist
$ cp -R /Users/sjors/..../cornerstone3D/packages/dicomImageLoader/dist .
$ cd ../../../
$ rm -rf .vite node_modules/.vite
$ yarn dev

Now, both the working.dcm and broken.dcm load properly.

@sgielen
Copy link
Contributor Author

sgielen commented Nov 23, 2023

Closing this, since the actual issue is already resolved in v1.29.0, and the most likely reason that version doesn't work in Vite is known and being addressed for v2.

@sgielen sgielen closed this as completed Nov 23, 2023
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

No branches or pull requests

1 participant