search

knopkem / dicomweb-proxy

A proxy to translate between dicomweb and traditional dicom dimse services (PACS communication)
Other
68 stars 19 forks source link

Wado rs #89

Closed vespasianvs closed 2 years ago

vespasianvs commented 2 years ago

This gets WADO-RS requests working via socket and directly.

vespasianvs commented 2 years ago

Trying to get a better version of socket.io-stream

vespasianvs commented 2 years ago

Can't see which security fix snyk wants...

knopkem commented 2 years ago

image

vespasianvs commented 2 years ago

Looks like simple-node-logger isn't really being maintained any more - there are a load of dependabot PRs waiting to be approved from up to 6 months ago and 23 issues outstanding.

Options as far as I see them:

knopkem commented 2 years ago

I would ignore the security issue for this PR and will take care of the logger issue (probably replacing it). Is this PR final now?

vespasianvs commented 2 years ago

I believe it's final now - it's been running on my server doing queries for a few days now and seems to be bug free (no guarantees of course!). You will probably get some jest files at some point...

knopkem commented 2 years ago

Hi, I had a look at the changes and I can see some improvements, e.g. providing more wado-rs routes (series, study level) which I omitted because I only focused on OHIF so far and there it's never used. Anyway, there are a couple of issues: In wado-rs: trying to perform c-finds and fetches on image level for every request is very very inefficient. There is just too much overhead coming with it. This is why I usually always do Series Level (waitAndFetchData) as this is way faster than fetching each image. Even trying this with local conquest is super slow (minutes vs seconds).

The other issue I faced was that the image was not rendered correctly. (From a quick look, I could not yet find out why). But here is how it should look: wadors_good And this is what I get: wadrs_bad

I did not yet test the socket changes.

vespasianvs commented 2 years ago

OK, I'll take a look. For the project I'm on we're more interested in Image level, so it's should be good with the two of us looking. I'll take a look at the speed issue as I've noticed it's a little slow, though most of the DICOM images I'm dealing with are single frame and fairly simple so ~40MB. I've not seen the rendering inconsistencies. Is that DICOM you're using a multiframe one or anything? Wondering if there is something going on with the frames.

knopkem commented 2 years ago

Hi, I think for image level you could skip the c-find lookup as the sopInstanceUID is already known, but requesting on image level will still be much slower than fetching the whole series (which you will need anyway). Regarding the image rendering issue: it's simple single frame images, I've used the samples provided by osirix (they were free once, now require registration): https://www.osirix-viewer.com/resources/dicom-image-library/ (macoessix)

knopkem commented 2 years ago

1.2.840.113704.1.111.4492.1160040150.25.dcm.zip Here is one image file of this series, should be enough

vespasianvs commented 2 years ago

Reproduced the weird display issue - not sure what on earth is going on there as the other viewers that I use all display correctly and dcmj2pnm decodes the DCM file into a correct JPEG. Will try to work out how it's getting messed up in OHIF.

As for speed, had a play with going back to series lookup and it seems about the same speed to me. Going to keep playing though - this is just an update for you!

vespasianvs commented 2 years ago

Made quite a lot of progress today - worked out that OHIF needs PixelData, NOT a DCM file returning, so fixed that. Also (for the purpose of my project) decided to implement the /rendered and /thumbnails endpoints. Almost there with it and will push changes probably tomorrow. It seems faster and things render properly now.

It does look as though OHIF doesn't support JPEG Baseline (lossy) - is this something that you've experienced?

vespasianvs commented 2 years ago

It is pretty much complete now, just want to test all of the endpoints properly. That's tomorrow's job though!

vespasianvs commented 2 years ago

OK, this is working the way I'm expecting it to work now and I believe meets the dicomweb and DIMSE standards - obviously I know you're going to have a check too, so let me know if you spot anything else I need to look at. Seems to work fairly fast and returns the expected data.

knopkem commented 2 years ago

Hi, thanks for the update will try to look at it asap. Regarding pixeldata vs dicomfile as buffer, this is actually not something special to OHIF: for wadouri the response contains the buffer of the whole file, while for wadors you have to only provide the pixeldata in the multipart responses. (Defined by the Content-Type), see https://dicom.nema.org/medical/dicom/current/output/chtml/part18/sect_8.6.html But maybe I misunderstood something here. Still surprised that image vs series level retrieve doesn't show much difference for you, what PACS are you testing again? DCM4CHEE? With CONQUESTSRV (but it's not the only one) this is much slower. Anyway, will test and let you know.

vespasianvs commented 2 years ago

I'm using Orthanc as my PACS server. I'll have a test against DCM4CHEE or CONQUESTSRV and see what it's like there.

Going to give the standards another read as I do want to get this matching the standards (That's kind of the point). I thought I'd read it as I could return the whole file, but I'm suspecting you might be right and I have to split out the bulkdata from the metadata and return them separately.

vespasianvs commented 2 years ago

Ok, done some playing with the return type and I'm happy with how it returns. Basically if you request an instance it does return with each multipart section having a content-type of application/dicom with a copy of the whole dicom file as a buffer. This matches what DICOMWeb seems to do for the servers that we're using so it's what we need. I guess for full compatibility it should look at the accepts header on the request and return appropriately (which is what the standards seem to suggest).

For what I'm doing, dcm4chee and conquest aren't on our list of servers so sadly not going to have time to test against those. I did change the code though to do series level searches on images anyway, so hopefully that will make things work better?

You may have noticed that I have tried to keep the bridge in sync with the changes to this as well.

vespasianvs commented 2 years ago

Just managed to find a few mins to play with CONQUESTSRV and didn't get any speed issues with this code setup.

knopkem commented 2 years ago

Hi, just had a first quick look at the latest changes and so far it's working well (no image corruption, speed is the same as before) and the code changes so far look good. Just see the one remark about the use exec, Will work on the update and hopefully merge this than soon. Thanks for the great work! (Will also test the socket changes soon)

vespasianvs commented 2 years ago

OK cool - as you probably worked out, I'm working in this for some paid work and I can't really give much more time to this now as I have it working in the way I need. Happy to chip in some things though if you need some help.

I do also have code for securing the sockets using certificates and also for STOW-RS if you're interested, but they're both on the same branch. I'm afraid it would literally be a case of me opening the PR and then you editing and merging it in the way you want (but the STOW-RS code does work well for me).

knopkem commented 2 years ago

Hi, ok no problem, will merge this now as it works well and modify the parts about the jpg generation later. I saw the stowrs branch already and would like to have this here as well, so just go ahead an open a PR, will pick what I need. Btw. If you happen to know any other good dicomweb viewer (e.g. that uses stowrs or the other parts that you added) let me know. Thanks