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

Handle large volumes #3235

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Ouwen
Copy link
Contributor

@Ouwen Ouwen commented Mar 13, 2023

Fixes #3235

Context

Some volumes can be 2500-3500 large and it is challenging to load these in memory. This PR helps by limiting the volume size and adds a callback function on the ImageSet class which can be used to filter out or change the metadata of an ImageSet / displaySet

Changes & Results

Large volumes are evenly sampled when they go above the maxFramesInVolume config. Changes have been made to the ImageSet class and config has been added to the CornerstoneCacheService.

Testing

  1. Open the viewer to http://localhost:3000/viewer?StudyInstanceUIDs=1.3.6.1.4.1.25403.345050719074.3824.20170125095722.1

  2. Open MPR mode to go into the volume mode (stack is unaffected by these changes)

  3. Run the following on the console/window
    services.CornerstoneCacheService.setMaxFramesInVolume(20)

  4. You should see the volume update due to the DisplaySet being invalidated. You can also use the DisplaySetService to set the ImageMapper and invalidate. This functionality is left to be placed in extensions as use cases can vary.

Checklist

PR

  • My Pull Request title is descriptive, accurate and follows the
    semantic-release format and guidelines.

Code

  • My code has been well-documented (function documentation, inline comments,
    etc.)

Public Documentation Updates

  • [] The documentation page has been updated as necessary for any public API
    additions or removals.

Tested Environment

  • [] "OS:
  • [] "Node version:
  • "Browser: Chrome Version 110.0.5481.177 (Official Build) (arm64)

@netlify
Copy link

netlify bot commented Mar 13, 2023

Deploy Preview for ohif-platform-docs canceled.

Name Link
🔨 Latest commit ff3e562
🔍 Latest deploy log https://app.netlify.com/sites/ohif-platform-docs/deploys/640e9ba625428a00084ba558

@netlify
Copy link

netlify bot commented Mar 13, 2023

Deploy Preview for ohif-platform-viewer ready!

Name Link
🔨 Latest commit ff3e562
🔍 Latest deploy log https://app.netlify.com/sites/ohif-platform-viewer/deploys/640e9ba65169410008a93826
😎 Deploy Preview https://deploy-preview-3235--ohif-platform-viewer.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.

@codecov
Copy link

codecov bot commented Mar 13, 2023

Codecov Report

Merging #3235 (ff3e562) into v3-stable (9f1e813) will increase coverage by 12.77%.
The diff coverage is n/a.

Impacted file tree graph

@@              Coverage Diff               @@
##           v3-stable    #3235       +/-   ##
==============================================
+ Coverage      25.15%   37.93%   +12.77%     
==============================================
  Files            119       84       -35     
  Lines           2862     1334     -1528     
  Branches         555      294      -261     
==============================================
- Hits             720      506      -214     
+ Misses          1856      664     -1192     
+ Partials         286      164      -122     

see 76 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b7fff77...ff3e562. Read the comment docs.

@@ -186,9 +201,22 @@ class CornerstoneCacheService {
displaySet,
dataSource
);
const distributedCopy = (items, n) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

This is going to cause rendering issues when items.length ~= n because it is just going to throw out a certain set of instances, which causes the interval between images to be off by a factor of 2. When n << items.length, it works ok,
because the interval becomes nearly consistent again, or if n = items.length / integer value, then one can sub-select evenly. It is possible to write a inconsistent sample viewer, but currently we assume in the configuration that the sample volume is consistent, and don't account for this type of differences.

Copy link
Contributor

Choose a reason for hiding this comment

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

You could write a decimator instead - that takes out every other image, or as a general implementation, leaves 1 image, takes k images out, then repeats. Decimators leave consistent spacing.

Copy link
Contributor

Choose a reason for hiding this comment

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

I added a decimator to CS3D in the HTJ2K progressive branch if you want one.

constructor(images) {
if (Array.isArray(images) !== true) {
throw new Error('ImageSet expects an array of images');
}

// @property "images"
Object.defineProperty(this, 'images', {
// @property "rawImages"
Copy link
Contributor

Choose a reason for hiding this comment

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

Just set instances to the complete list - instances is used elsewhere to mean all the instance values, whereas images is used for the images for display.

@@ -32,6 +34,24 @@ class ImageSet {
});
}

get images(): any {
Copy link
Contributor

Choose a reason for hiding this comment

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

The general design of the display sets is that they are fixed representations of specific groups of DICOM objects - this mapping allows redefining that, and can cause problems when rendering different image sets. I'd rather have a mapping function produce a new ImageSet entirely rather than having the images be variable.

Copy link
Contributor

Choose a reason for hiding this comment

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

It isn't used anywhere here, and I don't see any testing behaviour for it - I think this is a bad idea without careful consideration of what it means for other classes which expect to the representation of display sets to be consistent.

@JeffersonCurveBeam
Copy link

Wondering if there was any plan to merge this PR soon - we are facing an issue addressed by this PR #3229 (comment)

In essence using the 2D MPR feature on our image will consume the entire machine's RAM and crash the browser temporarily on a 3-month old machine.

@sedghi sedghi changed the base branch from v3-stable to master June 19, 2023 13:28
@sedghi
Copy link
Member

sedghi commented Jun 19, 2023

@Ouwen Is there someone at your team to handle the comments for this PR to get merged? Thanks!

@sedghi
Copy link
Member

sedghi commented Oct 4, 2023

this is useful we should add it

@Ouwen
Copy link
Contributor Author

Ouwen commented Oct 6, 2023

Hey @sedghi, sorry for the radio silence from my side. I've been under pressure on finishing my dissertation ha

@sedghi
Copy link
Member

sedghi commented Oct 10, 2023

@Ouwen Oh wow, Good luck, nothing is expected, just was trying to include you in the loop

@james-hanks
Copy link

@sedghi comment and close

@sedghi
Copy link
Member

sedghi commented Nov 8, 2023

So, after more thinking about this feature, I believe it will not adequately address the problem of large volumes as we desire. However, changing to a 16-bit texture might be the solution for most use cases, and in the future, we can consider the chunk and overlap solution.

https://docs.ohif.org/faq#how-do-i-handle-large-volumes-for-mpr-and-volume-rendering

Anyway, thanks @Ouwen for this

@sedghi sedghi closed this Nov 8, 2023
@sedghi
Copy link
Member

sedghi commented Nov 7, 2024

I will reopen for visiblity

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.

5 participants