Support for overlapping SEG segments

fedorov commented 3 years ago

I didn't realize it until now that overlapping segments are not supported. Example below shows an example of such study: https://idc-sandbox-000.firebaseapp.com/projects/idc-tcia/locations/us-central1/datasets/tcia-idc-datareviewcoordination/dicomStores/issue11-lung1-20201015-review/study/1.3.6.1.4.1.32722.99.99.203715003805996641695765332389135385095

Specifically, look for the GTV-1 in RTSTRUCT and Neoplasm in SEG, which should match up.

Same region below is shown in Slicer, SEG is in faded green outline.

As a short term solution, until this is fixed, I think it would be good to give some kind of a warning alerting the user when overlapping segments are present.

pieper commented 3 years ago

I'm not sure I completely understand the issue. Is discussing this with a demo at the meeting next Thursday soon enough?

fedorov commented 3 years ago

Not to imply I want it to be fixed by Thursday, but I do want the issue to be understood, so let me try again.

For the study here: https://idc-sandbox-000.firebaseapp.com/projects/idc-tcia/locations/us-central1/datasets/tcia-idc-datareviewcoordination/dicomStores/issue11-lung1-20201015-review/study/1.3.6.1.4.1.32722.99.99.203715003805996641695765332389135385095

There is segmentation of tumor in RTSTRUCT and the same tumor segmentation in SEG.

OHIF Viewer

RTSTRUCT tumor contour

SEG tumor contour

3D Slicer

RTSTRUCT tumor contour

SEG tumor contour

I believe in OHIF Viewer SEG tumor contour is cropped in the area where it intersects the lung segmentation:

Does this make sense?

pieper commented 3 years ago

I believe you yes, there's something odd going on. So you are saying that the image of the SEG in OHIF is being somehow obscured by the RT even thought the RT is not visible? I'm not sure how that would happen but yes, it's wrong.

Just to confirm too, we are talking about the 2D (cornerstone-based) view and not the MPR (vtk) view since the RT is not displayed in MPR. Which raises a question, are the SEGs displayed correctly in MPR mode?

fedorov commented 3 years ago

So you are saying that the image of the SEG in OHIF is being somehow obscured by the RT even thought the RT is not visible? I'm not sure how that would happen but yes, it's wrong.

No, I do no think it is related to RTSTRUCT. I believe that the issue is that when there are segments in the SEG series that are overlapping, the overlapping portion is lost (perhaps the lower-numbered segment takes precedence - in this example, GTV is the last one on the list).

are the SEGs displayed correctly in MPR mode?

No, they appear the same.

@JamesAPetts is it a known issue that overlapping segments from a single SEG are not supported?

pieper commented 3 years ago

Thanks for the explanation. Yes it sounds to me like the segs are being collapsed into a single labelmap so the lung is overwriting the tumor. Same labelmap would then be used MPR mode.

In Slicer there's some logic for using multiple labelmaps (collapsing the ones that can be correctly represented but adding more labelmap layers as needed to handle overlaps). We might need the same idea in ohif.

JamesAPetts commented 3 years ago

@fedorov Yeah this a known issue which is flagged up every time a seg is loaded (so long as warnings aren't being stripped!) :)

I believe its just never been priority for anything, would love to see this updated!

Cornerstone already has support for this, just a good adapter is needed.

Segmentations in vtkjs might be a problem with the current setup, however. If there are many overlapping segments.

fedorov commented 3 years ago

which is flagged up every time a seg is loaded

I think it should be flagged more prominently than in the console! ;-) I do not expect the average IDC user to keep the console open when using IDC.

But if fixing the underlying issue is not too difficult, would be great to just fix it.

JamesAPetts commented 3 years ago

I love to see it fixed, just not been a priority for anyone... until now!

I think it should be flagged more prominently than in the console!

Difficult to commit much more than that to a processing lib people use ;)

Punzo commented 3 years ago

@pieper @fedorov I have started looking into https://github.com/OHIF/Viewers/issues/2164 and, after having feedback from James and Erik, I have few notes on which I would like your feedback too. The problem is indeed that the overlapping segments in a segmentation overwrite each other (see https://github.com/dcmjs-org/dcmjs/blob/master/src/adapters/Cornerstone/Segmentation_4X.js#L658). This because for performance/memory reasons all the segments of the segmentation are merged in one array buffer (of dimensions equal to the data volume).

We see two solutions for loading properly the overlapping cases:

having N cropped (to the segment extensions) volumes, where N = number of total segment in the segmentation;
Create M non-cropped volume (i.e. segmentation) in which we have only non overlapping segments.

The best solution would be (1), but it would require more resources since we would need to:

have basically need a new cornerstone-tools version, as cornerstone segmentations assume an entire volume as large as the source volume.
modify react-vtkjs-viewport to position these segments in space differently (they are cropped).
these would be non editable if drawing tools are required in the future, unless we also modify both cornerstone-tools and react-vtkjs-viewport to support these cases.

While solution (2) would be easier to implement at the cost of computation time to create the M “non-overlapping” segmentation volumes (plus it can be strange for the users to have in the UI multiple segmentations which are actually the same divided in more, but we may fix this at the UI level).

In addition, since it would be ideal also to not affect the performances of the non-overlapping cases, the non-overlapping cases will be still treated as 1 segmentation volume. This means also that for not affecting the loading time for non-overlapping ones (either we use solution (1) or (2) for the overlapping ones), it would be ideal to have the data itself telling us if they are overlapping with a flag (e.g. https://dicom.innolitics.com/ciods/segmentation/segmentation-image/00620013). Would that be possible?

pieper commented 3 years ago

@swederik and I discussed this a bit earlier today. My preference is to see a simple and correct implementation first, then worry about performance. For example the overlap calculation could and probably should ultimately be done in a shader if it's a huge bottleneck.

I agree the user interface should not expose any of the implementation details, although we may want to visually indicate overlaps because they can be confusing to visualize.

For IDC we have considered modifying the SEGs to express the overlap tag correctly, but that's not the best solution for OHIF in the long run.

fedorov commented 3 years ago

My preference is to see a simple and correct implementation first, then worry about performance.

I agree.

This means also that for not affecting the loading time for non-overlapping ones (either we use solution (1) or (2) for the overlapping ones), it would be ideal to have the data itself telling us if they are overlapping with a flag (e.g. https://dicom.innolitics.com/ciods/segmentation/segmentation-image/00620013). Would that be possible?

I don't think we will add this flag to the existing data any time soon. I added a ticket to implement support for that feature in dcmqi (https://github.com/QIICR/dcmqi/issues/410). Once that is done, at least for the future datasets created with dcmqi, we could potentially have a check in OHIF and trust that flag to optimize load.

Punzo commented 3 years ago

I don't think we will add this flag to the existing data any time soon. I added a ticket to implement support for that feature in dcmqi (QIICR/dcmqi#410). Once that is done, at least for the future datasets created with dcmqi, we could potentially have a check in OHIF and trust that flag to optimize load.

ok, then I'll implement the checking if there are overlapping segments in the segmentation volume in the client.

@swederik and I discussed this a bit earlier today. My preference is to see a simple and correct implementation first, then worry about performance. For example the overlap calculation could and probably should ultimately be done in a shader if it's a huge bottleneck.

ok then I'll go with solution (2), which is the easier. For the moment just on CPU.

For IDC we have considered modifying the SEGs to express the overlap tag correctly, but that's not the best solution for OHIF in the long run.

@pieper do you mean just to indentify the overlaps and create new segments for them in the same segmentation volume (by simply assinging different interger values to the overlapping regions)? so in this way we can keep only 1 segmentation volume approach?

pieper commented 3 years ago

For IDC we have considered modifying the SEGs to express the overlap tag correctly, but that's not the best solution for OHIF in the long run.

@pieper do you mean just to indentify the overlaps and create new segments for them in the same segmentation volume (by simply assinging different interger values to the overlapping regions)? so in this way we can keep only 1 segmentation volume approach?

I meant to express that it would be cleaner if OHIF handles the case of overlapping segments robustly without relying on the flag, which as Andrey says it probably won't be available anyway at least for now.

I don't think assigning new lablemap numbers for overlap areas would scale well. Bookkeeping could be complex if there are lots of overlaps.

I had been thinking of this basic algorithm:

start a list with one labelmap
for each segment add pixel by pixel
- if you discover that there's already a non-zero where you are trying to assign try other labelmaps
- if there are no labelmaps available that don't overlap, create a new labelmap in the list
- keep track of which segments are in which labelmaps

There is still the question of how best to display the overlapping segments but for now color blending is good enough.

Like what Slicer does:

Punzo commented 3 years ago

logic done in https://github.com/dcmjs-org/dcmjs/pull/155. Tested on the datasets https://idc-sandbox-000.firebaseapp.com/projects/idc-tcia/locations/us-central1/datasets/tcia-idc-datareviewcoordination/dicomStores/issue11-lung1-20201015-review/study/1.3.6.1.4.1.32722.99.99.203715003805996641695765332389135385095 .

512x512x111 x 6 segment. The algorithm splits the Labelmap in two:

loading time were not affected (workstation with i9 5 GHz). However the computation complexity is pretty high and for more complex cases, it may be worth using shaders.

I have still to implement at UI level how to load the splitted labelMaps

fedorov commented 3 years ago

The algorithm splits the Labelmap in two

I think the existence of separate labelmaps should be completely transparent to the user. There should be no change in how segmentations are presented in the current UI after the implemented changes are in place.

Is it possible to show overlapping segments?

Punzo commented 3 years ago

I think the existence of separate labelmaps should be completely transparent to the user. There should be no change in how segmentations are presented in the current UI after the implemented changes are in place.

in this example/test, my idea would be to load the two generated splitted labelMaps volumes into two segmentations but then having the UI showing only one segmentation item in the combobox (so with all the segments of the two labelmap volumes again in one "segmentation"). This has still to be implemented and I am working on it, but I wanted to show already the logic and its perfomances (which for this simple case, looks good).

Let me know if you agree or you would go for another approach regarding the UI.

Is it possible to show overlapping segments?

yes, should be no problem, but I have to implement the UI to load both generated labelMaps.

P.S.: For making the screenshots in https://github.com/OHIF/Viewers/issues/2164#issuecomment-733080114, the selection of the labelmap volume is hardcoded now. Now I can load only one labelMap volume for a loaded segmentation (and I have only one segmentation in the combox).

fedorov commented 3 years ago

Let me know if you agree or you would go for another approach regarding the UI.

Yes, that sounds good. I thought that's what your plan was, but wanted to confirm. Thanks!

JamesAPetts commented 3 years ago

Is it possible to show overlapping segments?

Just want to chime in that you will likely have memory issues if you want to display multiple labelmaps in the MPR views of larger CTs (e.g. 512 ^3), you'll be fine for the CT above but:

GPU Memory is Float32 in RAM (or the copying is insanely slow) and Float16 on the GPU's VRAM. Currently memory is duplicated per viewport in MPR.

For the CT like above with 2 full segmentations:

=> 512 512 512 * 4 bytes = 512 Mb per volume in RAM, so for 2 that's already 1 GB RAM for those volumes alone, chrome starts to choke past ~1.5-2gb total ram in a tab.

On the GPU that's 512 512 512 2 bytes is = 256 MB per volume => 9 (for 3 views with a CT + 2 segmentations) === 2.3GB of VRAM which is too expensive for a lot of laptop cards.

pieper commented 3 years ago

So James you are saying we'll be needing some off-screen magic? 🧙

For now it would be good to confirm that we can detect such memory issues and report them to the user (something like "The selected data is too large to be viewed in this mode on your machine").

Punzo commented 3 years ago

James, you are right, and in the case that more segments are overlapping, the memory usage could spike easly using solution (2) of https://github.com/OHIF/Viewers/issues/2164#issuecomment-732240664 as in PR https://github.com/dcmjs-org/dcmjs/pull/155.

So James you are saying we'll be needing some off-screen magic?

well, unless we do major infrastructure changes at the rendering pipelines, I don't see what magic we can do. The labelmap are interger, it is not that we could blend the values in an offscreen solution (also all the UI controls refer to the integer values for swtiching off the visibility, opacity etc...).

Even solution (1) in https://github.com/OHIF/Viewers/issues/2164#issuecomment-732240664 for more complex cases will have issues. In fact, even cropping the lablemap volumes, could not be enough for a case with already 2 overlapping segments which extend over all the volume (if the volume is of the order of 512x512x100). I guess we should just accept the memory limitation of web solutions. I mean even for desktop solution as 3DSlicer, it is advisable to have available memory x5 times of the volume.

I think the only low memory usage/safe solution is:

@pieper do you mean just to indentify the overlaps and create new segments for them in the same segmentation volume (by simply assinging different interger values to the overlapping regions)? so in this way we can keep only 1 segmentation volume approach?

but I agree that it would not be ideal to have many (probably too many) segments that just indicates the overlaps.

If we like the current implementation of solution (2) in https://github.com/OHIF/Viewers/issues/2164#issuecomment-732240664 of the logic as in https://github.com/dcmjs-org/dcmjs/pull/155.

For now it would be good to confirm that we can detect such memory issues and report them to the user (something like "The selected data is too large to be viewed in this mode on your machine").

when OIHF parse the segmentation (https://github.com/OHIF/Viewers/blob/master/extensions/dicom-segmentation/src/loadSegmentation.js#L41), if the memory usage is > than 2 Gb we could warn the user indeed . Should we also undo the loading in that cases or just a warning would be enough?

For the moment I'll continue to implement the UI for solution (2).

JamesAPetts commented 3 years ago

Oh one thing that can be done right now is you could use Uint8 for these seperated labelmaps, vtkjs supports Uint8 out of the gate.

You would just have to add them as a type in CST, and put a disclaimer that it can only support 255 segments. We originally used Float32 as freesurfer is a problem as you could have like 400 segments on a labelmap, but for this use case you are probably ok.

This trade off in segment count would reduce the segmentation storage in RAM by a factor of 4, and by a factor of 2 on the GPU's VRAM, allowing you to do a lot more.

For now it would be good to confirm that we can detect such memory issues and report them to the user (something like "The selected data is too large to be viewed in this mode on your machine").

This is a good idea anyways, but not sure how much video card information you can actually get from the browser?

Punzo commented 3 years ago

Oh one thing that can be done right now is you could use Uint8 for these seperated labelmaps, vtkjs supports Uint8 out of the gate.

You would just have to add them as a type in CST, and put a disclaimer that it can only support 255 segments. We originally used Float32 as freesurfer is a problem as you could have like 400 segments on a labelmap, but for this use case you are probably ok.

This trade off in segment count would reduce the segmentation storage in RAM by a factor of 4, and by a factor of 2 on the GPU's VRAM, allowing you to do a lot more.

good idea. But I think at the moment it is used Uint16 (see https://github.com/dcmjs-org/dcmjs/blob/master/src/adapters/Cornerstone/Segmentation_4X.js#L362 and https://github.com/dcmjs-org/dcmjs/blob/master/src/adapters/Cornerstone/Segmentation_4X.js#L645). Anyway even a factor 2 would be good.

Punzo commented 3 years ago

cornerstoneTools at the moment support only Uint16 and float32 https://github.com/cornerstonejs/cornerstoneTools/blob/master/src/store/modules/segmentationModule/getLabelmapBuffers.js#L41. @JamesAPetts do you have any idea how much modifications to cornerstone would be needed to add Uint8 support? i.e. would we need just to update the labelmap classes? or rendering etc.. would need to be updated as well?

Punzo commented 3 years ago

@fedorov please have a look at the WIP PR https://github.com/OHIF/Viewers/pull/2185 and let me know what do you think about it.

My initial plan was to have only one unified UI list controls for the segments, but it looks like that OIHF/cornerstone allows only 1 "active" labelmap at time for segmentation and the others are rendered as non "active" (@igoroctaviano please correct me if I am wrong).

So the idea would be to have a combox to pick the active labelMap, see mock at https://github.com/OHIF/Viewers/pull/2185#issuecomment-733782643.

fedorov commented 3 years ago

Looks done to me!

OHIF / Viewers

Support for overlapping SEG segments #2164

OHIF Viewer

3D Slicer