search

dcmjs-org / dcmjs

Javascript implementation of DICOM manipulation
https://dcmjs.netlify.com/
MIT License
299 stars 112 forks source link

writeBytes is crashing the browser when using encapsulated files with lots of frames #159

Closed dpedranti closed 3 years ago

dpedranti commented 3 years ago

Hi,

I'm doing a write() after a data.DicomMessage.readFile and Chrome will freeze up and crash if I use it on larger Dicom files with encapsulated pixel data and lots of frames. Works great with other images that are big in size.

The file that failed is 117.3 MB with 120 frames.

The issue seems to happen in the for loop on line 258 here: https://github.com/dcmjs-org/dcmjs/blob/master/src/ValueRepresentation.js

Each iteration of that for loop becomes progressively slower until it stops somewhere in the middle of the 120 frames.

Can you help fix?

Thanks!

pieper commented 3 years ago

Thanks for the report. It would really help if you could provide a way to replicate this issue (i.e. sample data and test code to reproduce). That way it can be profiled and debugged.

dpedranti commented 3 years ago

Sure! And thank you. Here's a sample image that has encapsulated pixel data and 120 frames: https://oculusstoragestaging.blob.core.windows.net/error-testing/IM-0001-0010.dcm?sv=2019-12-12&st=2021-01-11T14%3A29%3A14Z&se=2021-02-11T14%3A29%3A00Z&sr=b&sp=r&sig=94kfZANKkjjd1q4GsgMaDkPG8vSEoHKTwQk6vG3A6s0%3D

Here's the code snippet where the problem occurs. Passing in the arrayBuffer of the aforementioned dicom file.

let dicomData = data.DicomMessage.readFile(arrayBuffer) // I have other logic here but not needed for this test as it fails with the same issue regardless let newFileWriterBuffer = dicomData.write()

So it's pretty much just doing a write() on dicomData. It very quickly freezes up and eventually crashes in my Chrome browser.

WoonchanCho commented 3 years ago

This is caused by too many BufferStream's "concat" method call to concatenate fragmented binary stream when dealing with a multiframe image, resulting in too many times of ArrayBuffer creation. This leads to O(n^2) on the memory (& time) complexity of writing DICOM data.

Some kind of optimization is needed for this part. I just quickly changed the part of source and am seeing an improvement of the performance. On my laptop with the same DICOM image, the same code ends within a second. I will raise a PR soon after performing few more tests.

Thank you.

dpedranti commented 3 years ago

Thank you very much! dcmjs is a great library and this fix will really help when working with multiframe images.

WoonchanCho commented 3 years ago

I've just raised a PR : #160. Hopefully, pieper or anohter maintainer will review and review or reject with some comments soon. If you just want to do an immediate test, you can clone my forked repo at https://github.com/WoonchanCho/dcmjs.

Thank you.

dpedranti commented 3 years ago

@pieper When do you anticipate this commit/fix will be released?

pieper commented 3 years ago

@pieper When do you anticipate this commit/fix will be released?

Good question - I forgot that we should have started the commit message with something like: "fix(writeBytes)" so that it would trigger the automatic release.

https://github.com/dcmjs-org/dcmjs#for-maintainers

I have to admit I haven't used this auto-release mechanism much, but I believe I can just put in a new commit with the correct format to trigger the release process. Let's see if that works.

pieper commented 3 years ago

Let's see if that works.

Yes, it did.

https://github.com/dcmjs-org/dcmjs/releases/tag/v0.17.1