cornerstonejs / dicomParser

JavaScript parser for DICOM Part 10 data
MIT License
716 stars 230 forks source link

Set default transfer syntax in options #48

Open NicolasRannou opened 8 years ago

NicolasRannou commented 8 years ago

Would it make sense to add a "defaultTransferSyntax" as an option?

The use case it that some custom made DICOM files do not contain this required field so DicomParser just returns an error.

Having an option would let the user "help" dicomParser to do the work: https://github.com/chafey/dicomParser/blob/master/src/parseDicom.js#L26

if(metaHeaderDataSet.elements.x00020010 === undefined && !this.option && !this.options.defaultTransferSyntax)

Please let me know if that makes sense and I'll submit a pull request.

Does it have any other implications that I am missing?

chafey commented 8 years ago

Hi Nicolas, I am aware of these files and have been reluctant to add support for them as they are non standard and would result in an inconsistent interface (sometimes the P10 header is present, sometimes it is not). My general stance on non standard files is that the logic to deal with them should be handled at the application layer and not the library itself. The library can expose functions to make it easier for the application to deal with but should not contain any logic for non standard files itself. The dicomParser library does allow a higher level application to parse these byte streams today - it just takes a few lines of code to do so.

NicolasRannou commented 8 years ago

Thanks that makes sense.

Would exposing the readDataSet and mergeDataSets method would be fine with you? https://github.com/chafey/dicomParser/blob/master/src/parseDicom.js#L140-144

This way I could add the defaultTransferSyntax in the metaHeaderDataSet before reading/merging the data.

Or would you rather expose something else in order to deal with it?

chafey commented 8 years ago

No that doesn't feel right - both of those are internal functions and don't look like good public APIs. Here is the code needed to parse little endian byte streams:

            var dataSetByteStream = new dicomParser.ByteStream(dicomParser.littleEndianByteArrayParser, byteArray);
            var dataSet = new dicomParser.DataSet(dataSetByteStream.byteArrayParser, dataSetByteStream.byteArray, {});
            dicomParser.parseDicomDataSetImplicit(dataSet, dataSetByteStream, dataSetByteStream.byteArray.length);

I thought about putting this in a utility function but that doesn't feel right either. Perhaps I just need an example to show how to do this? Open to more ideas

NicolasRannou commented 8 years ago

YesI agree, it is probably best to have an example or even just an entry in the wiki to let people know how they can manually parse a dataset.

chafey commented 8 years ago

I haven't forgotten about this (well sort of, but github doesn't). If someone wants to submit a PR with an example that does this it would be great as people keep running into this issue. It isn't something I need so I haven't been motivated to do it myself yet