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

wip Feat/norm16 textures #336

Closed
wants to merge 10 commits into from
Closed

wip Feat/norm16 textures #336

wants to merge 10 commits into from

Conversation

sedghi
Copy link
Member

@sedghi sedghi commented Dec 16, 2022

This relies on this cornerstonejs/cornerstoneWADOImageLoader#500

Volume Viewport

  • I made the necessary changes, but seems like the part where we update texture in fillImage3D has some problem, when I run volumeAPI example, the webGL context get lost, and I get a flash of empty screen

https://github.com/cornerstonejs/cornerstone3D-beta/blob/feat/norm16-textures/packages/core/src/RenderingEngine/vtkClasses/vtkStreamingOpenGLTexture.js#L103

Stack Viewport

  • stack viewport not working need to be fixed. I guess we can just use Float32 for now? as before. Later we optimize this? Oh yes, I guess I didn't add vtk.js support for imageMapper for norm16, so we don't have option, we should just use it as before

CPU

  • Needs fixing I guess since I commented out bunch of things. I don't think we need to do anything here, just uncomment what I commented out

CC @swederik @Ouwen It is very interesting, sometimes when I loose the GL context, the extension becomes unavailable (webgl report v2) I need to restart chrome for it to appear again!)

@netlify
Copy link

netlify bot commented Dec 16, 2022

Deploy Preview for cornerstone-3d-docs ready!

Name Link
🔨 Latest commit dc2366e
🔍 Latest deploy log https://app.netlify.com/sites/cornerstone-3d-docs/deploys/63a376a92ac9280009635559
😎 Deploy Preview https://deploy-preview-336--cornerstone-3d-docs.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

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

* @param {any} [scalarData] - The data to be converted.
* @returns The data type of the scalar data.
*/
export default function getScalarDataType(
Copy link
Member Author

Choose a reason for hiding this comment

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

This needs to get expanded to consider scaling parameters to decide on the final data type (if scalings are float, then the type passed to wado as option should be float)

Copy link
Contributor

Choose a reason for hiding this comment

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

I am assuming this is usually used for PET overlays. Would it be worth it to account for preferSizeOverAccuracy to perform a scale + cast

@Ouwen
Copy link
Contributor

Ouwen commented Dec 20, 2022

CC @swederik @Ouwen It is very interesting, sometimes when I loose the GL context, the extension becomes unavailable > (webgl report v2) I need to restart chrome for it to appear again!)

That is weird behavior... is the GL context lost just during debugging or will it randomly occur. I can try it on my local too

@Ouwen
Copy link
Contributor

Ouwen commented Dec 20, 2022

@sedghi the divergence of stack and volume might have some consequence for the cache since the stored values are going to be uint16 / int16 in the cache.

Is the plan just to cast them to float32 inside the stackViewport on vtkImageData creation? Then the CPU cache can be small.

StackViewport doesn't seem to use too much GPU memory anyway so I don't think it is a big deal

@sedghi
Copy link
Member Author

sedghi commented Dec 20, 2022

@Ouwen That is an interesting point, maybe then I just add the norm 16 to image mapper as well.

I'm having some trouble with the rendering volume right now on Mac and Chrome. If you have time and look for some headache, can you check this branch (with the changes in wado, PR as well there), and see if volumeAPI example work for you?

I'm looking inside Chromium forums to see if it has been reported there too.

@Ouwen
Copy link
Contributor

Ouwen commented Dec 20, 2022

image

ha nice abstract art feature, is this also what you are seeing so far?

@Ouwen
Copy link
Contributor

Ouwen commented Jan 15, 2023

@sedghi did a rebase of this on top of
https://github.com/gradienthealth/cornerstone3D-beta/tree/gradienthealth/no_shared_array_buffer

I made some changes to the stack viewport so it should also properly render int16 and uint16 data types. Apologies on the premature commit squashing for now...
https://github.com/gradienthealth/cornerstone3D-beta/tree/gradienthealth/norm16-textures

One of the issues I found was that uint16 support requires some more logic for prescaling.
I think that we should perform some casting logic here
https://github.com/cornerstonejs/cornerstoneWADOImageLoader/blob/9c14da52e0ddd8224d2e0973105895d0bc6b6b19/src/shared/scaling/scaleArray.js#L1-L21

Thoughts on the following:
if a targetbuffer type is present it means we should always return that targetbuffer type. Thus, if the target is uint16 and scaling values are an int we may need to cast the pixeldata to int16, scale, then recast to uint16.

if a targetbuffer is not present we are using the native decoded pixelData (float32, uint8, uint16, int16, int8 not supported yet but maybe later). We try to preserve dynamic range. So if rescale slope and rescaleIntercept are both positive and native type is uint16, we can keep uint16 type. However, if rescale slop and rescaleIntercept are possibly negative we can cast to int16. Alternatively, we can always cast to float32 and recast to the min pixel type to preserve dynamic range (keep track of a min/max)

@sedghi
Copy link
Member Author

sedghi commented Jan 16, 2023

@Ouwen great observations.
Yes, I was thinking about the same thing couple of days ago
IMO, we shouldn't force the targetBuffer to be float32, we should just let the wado detect and choose the correct type. Why do you think there is a use case for specifying the targetBuffer type? it should just be the array buffer. no?

@Ouwen
Copy link
Contributor

Ouwen commented Jan 16, 2023

I think the only reason target buffer is needed is because for shared array buffers we need to allocate prior to decoding for progressive loading.

So to allocate the best type, we'd need to look at scaling, minvalue, maxvalue, and some other dicom metadata tags (signed, bitsAllocated, photometric interpertation)

@Ouwen
Copy link
Contributor

Ouwen commented Mar 16, 2023

this can be closed with #420

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.

2 participants