Open fedorov opened 2 years ago
Related links found during the call today:
As pointed out today, it may be better and more feasible to catch the error in buffer allocation that is caused by the fact that the browser is 32-bit.
Whatever we decide to do for Slim we should also do for OHIF, if there is an issue on 32-bit.
ok working on it
@fedorov the issue is at the WASM level (the codecs)
the viewer will use libjpegturbowasm
which fails for memory issues.
the wasm requires 1 Gb https://github.com/cornerstonejs/codecs/blob/main/packages/libjpeg-turbo-8bit/src/CMakeLists.txt#L31
also it needs to be one contiguous chunk of memory, which can be hard on a 32 bit browser as memory tends to fragment over time. On 64-bit this is not an issue (due to the huge virtual address space possible with 64-bit pointers).
I have to say it is the occurrence to have a user on a 32 bit browser nowadays should be very low. For example on latest stable release of ubuntu (22.04) I could not install and making it working either google-chorme or firefox 32 bit. For windows, in the google-chrome download page will always pop up the 64 bit version, and you need to explicit navigate the 32 bit version download page (although you can install it in Windows and it works).
I double-checked there is no way to uniquely identify if google-chorme (or any other browser) is 32 bit or 64 bit (e.g., https://github.com/arouel/uadetector/issues/60#issuecomment-31763975).
This issue is related to the wasm modules used in the codecs, so also OHIF (i.e. cornerstoneWADOLoader) will be affected (however to reproduce you need large images and it really depends on the memory fragmentation).
In conclusion, the only possible action is to state in the docs to use a 64 bit web browser (which I would say in 2022 it is fair to assume almost all of the desktops and laptops are running on 64, while some phones are still at 32 bits, niether OHIF and SLIM are optmized for small devices). See https://github.com/ImagingDataCommons/IDC-Docs/pull/40
For reference: https://github.com/WebAssembly/design/issues/1397
user on a 32 bit browser nowadays should be very low
Unfortunately our customer (NIH users) do run into this issue so it's a priority to handle it gracefully 😄
We can't rely on people looking for documentation. We need to catch the memory allocation error and provide a meaningful message.
I agree that it would be nice to handle such errors gracefully. However, that will not always be straight forward. I wonder whether we could instead check for resource requirements (available memory, browser version, operating system version, etc.) upon instantiation of the app and route to an error page if a requirement is not met.
From what @Punzo was able to find out it's not easy to detect these issues in advance. I know we cannot assure flawless behavior in all circumstances, but gracefully handling the special case of NIH default computer configuration is something we should be able to do through conventional error catching.
If our primary goal is to work around issues with NIH computer configurations in the context of the IDC, we could add a specific check into the source code of the IDC fork.
However, I think there would be value in trying to reduce the memory footprint, since this may become an issue in other environments (e.g., mobile).
We can discuss with Andrey, perhaps during tomorrow's IDC meeting. Yes, doing something specific to patch the NIH user use-case may be the best tradeoff of time and effort. If IDC can't fund a more general solution then this specific use case may be easier to fix instead (if we can find a simple solution).
BTW, @hackermd I am in complete agreement with keeping the viewer clean and free of special case code. At the same time putting too much logic in a fork is not good either (chance for divergence, merge conflicts, etc). So you might consider adding a hook for extensions so that IDC wouldn't keep it's logic in a fork but rather in a self-contained library that it adds in at build time or even run time.
@pieper I don't want to commit to an extension framework or provide backwards compatibility guarantees for libraries (yet). Since we already maintain a fork with IDC-specific changes, I would prefer adding this very IDC-specific check or error handling logic to the fork.
If our primary goal is to work around issues with NIH computer configurations in the context of the IDC
I don't think we need this to work around issues with NIH computers per se. What we identified is that a large organization with important users is still using legacy browser. If this is happening at NCI, it can be the case at NHLBI, NIBIB, FDA, large hospitals, and other organizations with complex corporate IT infrastructure. This actually brought memories when at one point I was involved in a project with a not to be named pharma company, which used a file transfer service that required a version of IE which at that point was completely deprecated and labeled as a known security risk...
I would argue it is actually less important for IDC than for other situations. With IDC, the chain of command within NCI is pretty clear, and if something doesn't work as identified by someone at NCI, our IDC federal lead will get contacted. But in other situations, where Slim or OHIF is used for a demo, or in a grant submission, or in a paper submission by the reviewers, most likely we will never know about those failures.
If the only way to address this is by adding this check to a fork, then let's keep it out of the fork, and close this issue. I personally do not think this belongs to a fork.
I agree that we should aim to support as many browsers as possible. However, we rely on state-of-the-art web technologies (WebGL, WebAssembly, WebWorkers, etc.) to achieve high image decoding and rendering performance and provide a smooth viewing experience. Therefore, there will be limitations when it comes to support of very old browser versions.
The app should be more robust in identifying situations where user browser is not compatible. As a specific example, it turns out there still users that have 32-bit Chrome, which does not work with IDC pathology data (I do not know if it doesn't work at all, or doesn't work with some images) - see details in https://discourse.canceridc.dev/t/cant-visualize-cptac-pathology-image/281. Is this something that is possible to address?
I do not know if this is a "bug" or "enhancement". It definitely comes across like a bug for an IDC user trying to view pathology images on a 32-bit Chrome browser.