-
Notifications
You must be signed in to change notification settings - Fork 36
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
Fix selection of VOLUME images #83
Conversation
Visit the preview URL for this PR (updated for commit 9b43a2c): https://idc-external-006--pr83-bugfix-volume-image-s89eob8w.web.app (expires Sat, 05 Feb 2022 17:58:16 GMT) 🔥 via Firebase Hosting GitHub Action 🌎 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Comments from @hackermd made on slack, sharing here for the sake of facilitating conversation with @dclunie In the current dev version of the dicom-microscopy-viewer (not yet publicly released), we specify many more acceptable media types for retrieval of frames via the Accept header field: generic::INVALID_ARGUMENT: 1:09 |
All samples I tried to open in CPTAC-COAD failed to zoom. A related bug in IDC portal is that OHIF Viewer, not Slim, is opened when attempting to open individual SM series as opposed to the study opening - see ImagingDataCommons/IDC-WebApp#872. |
@dclunie has anything changed in the JPEG 2000 encoding of frames when compared to the previous conversion? Since Healthcare API and client-side decoder both fail to decode the images, I suspect that it's due to the JPEG 2000 bitstreams. |
A couple of observations related to the "Slim won't zoom this series" problem, which may or may not be actually related to this pull request #83, as opposed to some other tracker or PR. Re. PhotometricInterpretation Since the beginning, I have been converting SVS images such that the first IFD, which is the highest resolution layer, is left in whatever tiled compressed encoding it was (JPEG or J2K), and the second IFD, which is stripped rather than tiled in the SVS, has always been decompressed, so there has always been (and is for all SVS converted CPTAC and TCGA images whether Slim zooms them or not) a difference in PhotometricInterpretation for at least these two (i.e., YBR_FULL_422 or RGB for JPEG (since Aperio sometimes color transforms for JPEG, and sometimes doesn't), YBR_ICT for J2K and RGB for uncompressed). This is the case for one example we saw on today's call that fails to zoom: CPTAC/01OV019-2f901c90-617e-46da-9fac-d68369] % dctable -describe -k PhotometricInterpretation -k TransferSyntaxUID -k ImageType DCM_* The original SVS for this one was "CPTAC-OV/01OV019-2f901c90-617e-46da-9fac-d68369.svs". Re. JPEG 2000 I can confirm that, for this series at least, the encapsulated JPEG 2000 bitstream appears to be problematic, in that some codecs cannot decode it. The Java JJ2000 codec can't, for sure. Kakadu (kdu_expand) seems to be able to though (at least for the first frame that I extracted from DCM_3 and tested) Is this perhaps related to: https://docs.google.com/document/d/1hDj62kAZLYIzF_4-jnMd5RkpOFZaR684HPGw-S1Sjhg/edit Is this just a J2K bitstream that Slim's codec cannot handle? Interestingly, QuPath 0.3.1, which has the DICOM WSI BioFormats jar file, also fails to open the DICOM converted files (throws a Java codec-related exception), but it will load the SVS file, which should contain exactly the same J2K bitstreams. I mention this, because it might be worth asking Melissa why one loads with her code and not the other. |
Interesting! Thanks for the insight @dclunie. I have included a check in the decode function that asserts that the decoded array buffer has the expected length. In this example, the check fails. My guess therefore is that the bitstream is truncated and some decoders may be able to handle it while others don't. Not sure whether this is compliant with the JPEG 2000 standard and just an edge feature that few bothered implementing or rather a compliance issue. |
If it turns out that the data is encoded incorrectly, it raises the questions what we do with such images. Shall we exclude them or attempt to fix them? The latter could result in lossy decompression. |
If Kakadu decodes them, then likely they are not "encoded incorrectly". Have you evidence of streams that do not begin, contain or end with the correct marker segments? It is not inconceivable that I may be encapsulating some frames incorrectly in my conversion code, but I haven't seem evidence of that so far. |
Good idea! I do check for the start of file marker (also to determine which decoder to use), but not for the end of file marker. I will take a look at the end of the frames and report back to you. |
I did some exploration using this dashboard I shared with you a bit earlier. If I look at the Those samples that I tried from This is the error in the console: |
As aside, it is unfortunate that it is not possible to view the DICOM header in Slim - in this case it would be handy to confirm that the metadata shown in the dashboard matches what is in those specific series, as a way to double-check. |
Agreed, that would be a very useful feature. |
Any series that contains images with different values for the Photometric Interpretation attribute is currently expected to fail. This behaviour should be fixed with this PR. The other issue I reported via Slack regarding the decoding the JPEG 2000 bit streams is probably different. Let me draft a new release of Slim so that we can start untangling the two issues. |
We have been assuming that VOLUME images of a digital slides (images with the same Frame of Reference UID and Container Identifier) have the same Photometric Interpretation. The motivation for this has been to separate color and monochrome images. However, the value of Photometric Intpretation is dependent on the transfer syntax and color or monochrome images may have a different Photometric Interpretation when they have been compressed with a different codec (e.g., JPEG 2000 and JPEG).