Bug : Empty dicom file writen by 0.45

[salimkanoun]

Hi there,

Just wanted to warn that there is a regression in 0.45 but not yet fully indentified,

I have some dicoms (pontentially depends on TS) that loose pixel data after doing

originalDicomDict.write({ allowInvalidVRLength: true });

Suspected changes : https://github.com/dcmjs-org/dcmjs/compare/v0.44.1...v0.45.0

I'm trying to recover the original image that can show the problem, will update if i can get them.

Wanted to warn meanwhile as dicom generation with DCMJS is prone to corruption with 0.45

[salimkanoun]

Hi there, sadly i have now way to recover files showing the issue, my guess is that it is depending on transfer syntax, is there a dataset representing all transfer syntax to tests somewhere ?

[pieper]

@wayfarer3130 can you think of any reason this is happening?

I believe soon we will have resources to really ramp up testing infrastructure for dcmjs and I hope that streaming an transfer syntax testing can be part of that.

[wayfarer3130]

@wayfarer3130 can you think of any reason this is happening?

I believe soon we will have resources to really ramp up testing infrastructure for dcmjs and I hope that streaming an transfer syntax testing can be part of that.

There were a number of changes to how things got created/generated that could cause problems, but I can't think of anything specific enough to cause that problem. I haven't had much of a chance to get back to that work for a while now. I'd really like to have some better testing infrastucture for dcmjs and add testing for streaming properly. Really like to have metadata and naturalized streaming conversions too, with the writer changed to just use streaming to write the output so that we don't actually need to convert formats in order to stream to binary DICOM.

[wayfarer3130]

It might be fixed by https://github.com/dcmjs-org/dcmjs/pull/461 but I'm not sure. I think that one mostly resolves to a null pointer, but maybe it is related.

[JamesAPetts]

Any movement on this? We want to upgrade but this is a bit of scary one.

[wayfarer3130]

I don't think it was caused by the NPE when I looked at it a bit more, but without test data/cases it is really hard to know what is going on.

[pieper]

If anyone finds a way to replicate this please contribute data and/or scenario.

[JamesAPetts]

@salimkanoun do you have any data that reproduces this issue?

[salimkanoun]

Unfortunately despite my efforts didn't get the DICOM to reproduce ... I downgraded and i'm also afraid of upgrading ....

[JamesAPetts]

Are you sure it was caused by 0.45? Like did you have a certain type of file which then had issues on upgrade? Or did you just happen to spot this issue when using 0.45?

I have similar worries about upgrading. If we are sure its with 0.45 maybe we should roll it back, and have the 0.45 code on a branch until its deemed safe?

It seems like a pretty dangerous branch to have in the latest released version someone will get automatically when pulling from npm.

[salimkanoun]

Yes I'm sure about 0.45. I didn't tried other version over 0.45, I get back to 0.44.1, this version is safe.

When I upgraded to 0.45 I had several upload in GaelO corrupted, it didn't throw any error but it was resulting in DICOM without pixels in it. It didn't happens for all dicoms, making it hard to depict, this is why i suspect TransferSyntax related issue (to explain why it is not every dicom that had the issue)

[pieper]

@salimkanoun can you provide any more details about what data failed and what didn't? Can you explain what you mean by upload in GaelO corrupted? I'm not familiar with that term GaelO. Can it be enhanced to check for missing pixel data to help debug? If only the PixelData was missing can you tell anything from the rest of the headers?

Regarding testing, we are using dcmjs as part of this browser-based data curation project, and definitely want to capture any real-world scenarios in our testing. See this issue and chime in if you have ideas to make things more robust: https://github.com/bebbi/dicom-curate/issues/128.

Also for that project we'll want streaming support. I'd been thinking to implement it using an incremental reading approach as described here instead of doing it natively in dcmjs. This could be a fallback if we revert the native streaming, but native streaming inside dcmjs would be preferable if we can be confident it doesn't introduce regressions.

[salimkanoun]

Dear @pieper ,

GaelO is our product (https://www.gaelo.fr/), a part of the platform is using DcmJS. We use DCMJS to read DICOM, do some dicom tags changes and rewrite DICOMs .

What we do with DCMJS : data.DicomMessage.readFile and store dicom header as js object update the read dicom tags in memory (just update some tags, not changing pixel data) Instanciate let dicomDict = new DicomDict(), inject the updated dicom tags in .dict property and write it with dicomDict.write()

You will find a corrupted sample : https://e.pcloud.link/publink/show?code=XZww6AZ1GJ7QY4bb6FTmsKgYy8ziLXsQYT7 You will see for this series we have 914 instances, 2.3kb files while it is CT data, expected size would be around 500kb. By reading with http://dcmjs.org/dump/index.html I don't see pixelData, but others dicom tags seems OK.

Hope it can help,

Best regards,

SAlim

[pieper]

Hi @salimkanoun - thank you for the context and example data. I agree there doesn't appear to be anything unusual about the header, other than the missing pixel data. If I understand your setup correctly you should have the option of adding some sanity checks to your GaelO code to detect if this happens again; that seems like a good idea regardless of how we resolve this issue.

Getting back to debugging, does your relationship with the original provider include the option of having them find the original data and try it again? Even if we revert the changes to dcmjs we really do want to understand the source of the issue so we can have tests that prevent it happening as a result of future development.

Also, in the details, the transfer syntax in the metaheader says LittleEndianImplicit. Is that one of the tags you changed or was that the original value?

[wayfarer3130]

Hi @salimkanoun - I'm gong to try to look at the issue tomorrow to see if I can reproduce it.
What exactly are your steps?

1. Read Part 10 file into memory
2. Read DICOM metadata format from DICOM binary
3. Read Convert to Naturalized
4. Add tags - any tags being deleted? Any new sequences?
5. Convert to DICOM metadata format
6. Write to in-memory data
7. Write buffer to file I'm not saying this is what you are doing, just saying it might be so that I can guess what is going on? Is the source data LEI and the destination as well? Is it multiframe data? I'm guessing that the original data was LEI and that you are writing as LEI given that you talk about 500 kb files. Any chance I could get a dcmdump of one of them minus the PHI containing fields? Also maybe dciodvfy output from one of the input files - it would be nice to know it was valid before being updated.

[wayfarer3130]

@salimkanoun - I have a unit test now that reproduces your issue - that is, I'm assuming it is your issue as would cause exactly what you are seeing on the files that must have been the inputs you were using. Will let you know when I have a fix.

[salimkanoun]

@pieper for the transferSyntax, Yes we changed it, after DCMJS the generated file by DcmJS goes through a transcoding pipeline with Orthanc. Unfortunately we don't have access to original sample, sometimes when we ask for sample for debugging we got some sample but sometime the user who generated the error is not available anymore and we fail to get the original data.

@wayfarer3130 yes the process you mentioned is what we are doing, and yes data could be multiframe (even if in this case not multiframe) We don't change the transcoding in dcmjs it is done afterwards with Orthanc that is why you wave the LEI, but Orthanc is not causing the problem, I didn't update Orthanc for a while, it is dcmjs that was making the pixel data missing.

Wonderfull that you get something to fix it.

I think the best we can do is trying to make a repository for testing dicoms, these aspects are hard to debug.

[wayfarer3130]

Being fixed in PR https://github.com/dcmjs-org/dcmjs/pull/462

[salimkanoun]

Yes ! I also think this is the bug, as the bug was not occurring from center to another I suspected a transfer syntax issue that could be different across sending centers

[wayfarer3130]

@salimkanoun - I'm not sure what else to look at right now without example input files or reproduction steps. I suspect it is an issue on the read side but I don't immediately see anything that could cause it. Do you have a chance to try the PR and/or capture at least one input file that reliably reproduces the issue?

[salimkanoun]

Unfortunately I tried but failed to get the original file. Our platform is meant to anonymize the dicom on the file so I can't capture the original data without a willing help for sending centers (and unfortunately failed to get someone able to understand what's happening and able to help us)

[pieper]

@wayfarer3130 are the changes in #462 likely to fix the issue?

@salimkanoun are you able to add more sanity checks to your code to detect any regressions in the future?

[wayfarer3130]

@wayfarer3130 are the changes in #462 likely to fix the issue?

@salimkanoun are you able to add more sanity checks to your code to detect any regressions in the future?

@pieper - I don't immediately see how any of the fixes I have could resolve the issue - I'm trying to see if I can figure out an integration test in this version that fixes those, but I don't see how to reproduce the problems they are fixing without doing incremental/streaming addition to the buffer objects.

[pieper]

@salimkanoun can you have a look at #463 and let us know if it addresses your concerns? It a lot of code to look at but do you want to test it before we merge? It reverts the streaming changes with the idea that we'll design a new API for that and leave the other code in its prior state.

[salimkanoun]

I there, I can confirm @wayfarer3130 scenario that i'm able to reproduce, compressed TS loose their pixel data in 0.45 but not 0.44

@wayfarer3130 there a think a bit strange, although I see pixel data is missing after read and write operation with dcmjs, the generated filesize is stable before and after write. Is it because the pixel data are inside the file but malformed and thus unable to read it ?

[salimkanoun]

I put a sanity check on my app but it has performance impact as it add a re-read /parsing process of each instance. Will use it temporarly to validate the new dcmjs version.

I didn't understand which is the relation between #462 and #463 ? which one is the fix ?

[wayfarer3130]

#462 was an attempt at a fix, but it turned out not to resolve what I thought it was due to a bug in the unit test #463 is a revert of the issues that caused the problem, but also got rid of any streaming parsing #464 is a new streaming parsing implementation that touches very little of the existing DicomMessage class and a bit of hte buffer stream class, but with a decent unit test to make sure that it is working.

I would suggest trying #463, and avoid doing a reparse, BUT, check the output file size - if the size before is bigger than 10k say, and the size after is less than 10k, then add a warning/error message. That is the easiest way to make sure the pixel data is present. You could even do it during the input phase - if a file size is > 40k or so and doesn't contain either a large bulkdata item OR pixel data, then abort the transform.

Longer term, for improved performance, the async version of the class should be used with direct streaming of updates. Basically that will avoid needing to import the data all at once into memory. The process is:

async reader => update listener => async writer

Each => phase is a listener interface, not a DICOM metadata interface or naturalized representation interface so it just writes to the output as it reads from the input.

[salimkanoun]

Dear @wayfarer3130, the problem is that with 0.45 the file size is correct even if the pixel data is missing so i won't detect the bug. I'm supprised the size being still correct, it sounds that the written dicom contains the pixel data but in a non readable way making the read not founding the pixel (that why i have to re-read the gerated dicom to see it does not contain the pixel values)

Is it consistent with the bug you found ? size is not changed because there is pixel data somewhere but not formatted accordingly ?

[wayfarer3130]

Dear @wayfarer3130, the problem is that with 0.45 the file size is correct even if the pixel data is missing so i won't detect the bug. I'm supprised the size being still correct, it sounds that the written dicom contains the pixel data but in a non readable way making the read not founding the pixel (that why i have to re-read the gerated dicom to see it does not contain the pixel values)

Is it consistent with the bug you found ? size is not changed because there is pixel data somewhere but not formatted accordingly ?

I never actually reproduced the problem. The sample you uploaded did NOT show the original size, it was missing the pixel data entirely. Regardless, the rollback should resolve the issue, and then at some point in the future you may want to try the streaming parser once it has enough miles on it.

[salimkanoun]

OK great, I did reproduce the error with your testing file test/sample-op.dcm Will recheck once dcmjs will be released and will test it for a while on our demo server