GNOME Bugzilla – Bug 163256
DICOM plugin crash
Last modified: 2008-01-15 12:44:28 UTC
I'm under orders not to post the DICOM file to a public location, email me if you want to try it, i'll send it to you. Take said file, and try to open it. The DICOM plugin crashes when it attempts to open the file. I have a whole CD full of these files from the output of an MRI scan of a family member.
Created attachment 35627 [details] [review] patch to make the bug go away This patch fixes the problem. I'm not a DICOM expert, so I have no idea if this is breaking a specification or not.. any expert want to check this? :) dave.
Can you explain why the patch is necessary? Are the files corrupt, or is the dicom plug-in just not handling them properly? Do other applications succeed in opening them?
I assume the image files are not corrupt, given I have a CD full of them. The windows app on the CD opens them fine, as does ImageMagick and xmedcon. It appears as though the dicom plugin is just not handling it. (fun and exciting new sub-format?) Specifically, the element_length is being set to -1, fread() that way directly from the file (in the "Implicit encoding" case), not because a function call somewhere failed The failing iteration of the load_image loop begins at byte 350 of 135988: group_word = 64 element_word = 629 value_rep = {-1,-1} .... Enters the Implicit Encoding Case ... element_length_chars = {-1,-1} Thus, element_length = -1, and the next fread doesn't like that particular length value. :( I'm not an image file expert, so this is probably not the "correct" solution. But might be preferrable to a crash until the image can be handled properly. Simply discarding the iteration has no visible side effects in the image, but I haven't run a pixel for pixel diff against known good output.
This bug report is void unless you can add an example image. You should be able to anonymize an image so that you can attach it here w/o compromising any personal data.
Then it is void. I do not have sufficient free time right now to learn what all the bits of the header do, to strip out _all_ the personal stuff and leave parts related to the image unmodified. (maybe someone can suggest a tool to do so?). If I ever come across an MRI scan on the net that exhibits this same problem I'll refile and attach a reference. I'm the meanwhile I'll just use ImageMagick.
I don't see why this necessarily needs to be void without a sample image, and if the format were almost anything other than dicom, it could probably be handled by looking at the file spec and deciding what is the Right Thing To Do. The problem is that the dicom file spec is hideously unreadable. However, at the site http://www.leadtools.com/SDK/Medical/DICOM/ltdc11.htm, I found the following information: BEGIN QUOTE Explicit Length: The number of bytes (even) contained in the Data Element Value (following but not including the Data Element Length Field) is encoded as a 32-bit unsigned integer value (see Section 7.1 of the DICOM Standard). This length shall include the total length resulting from the sequence of zero or more items conveyed by this Data Element. This Data Element Length shall be equal to 00000000H if the sequence of Items contains zero Items. Undefined Length: The Data Element Length Field shall contain a Value FFFFFFFFH to indicate an Undefined Sequence length. It shall be used in conjunction with a Sequence Delimitation Item. A Sequence Delimitation Item shall be included after the last Item in the sequence. Its Item Tag shall be (FFFE,E0DD) with an Item Length of 00000000H. No Value shall be present. END QUOTE So it looks to me like you're in the "Undefined Length" situation, and it doesn't look like you're handling it correctly. If you can come up with a solution that does something reasonable, such as skipping over everything up to the Sequence Delineation item, and attach a patch here that does it, that would certainly be helpful, although we probably would not commit the patch without getting some expert assurance that it won't break the plugin in other situations. GIMP's handling of dicom seems to be generally pretty weak, but the format is so difficult that it would probably require somebody who has worked with these files a great deal to get involved for it to improve very much.
Well, I've added Dov to the Cc so that he can have a look. I don't think it makes sense if someone else wastes time on this, especially since we are lacking a test case.
In any case this shouldn't be NEEDINFO any more, although it would definitely be helpful to have an example file. Might as well confirm since there is good reason to think this is something the DICOM plug-in ought to handle and doesn't.
Bug #166538 also reports a crash and is most likely a duplicate. Fortunately it links to some publically available images that trigger the problem.
*** Bug 166538 has been marked as a duplicate of this bug. ***
The plugin cannot cope with image files that contain data elements whose value length is undefined/implicit (0xFFFFFFFF or -1 represented as a two's complement integer). This pretty much only happens for sequences (i.e. value representation of the data element is "SQ"). It also happens for encapsulated pixel data (e.g. JPEG or RLE compressed). I have also found the plugin to not handle the following color models: - RGB (color-by-plane). Note: RGB (color-by-pixel) works. - MONOCHROME1. Note: MONOCHROME2 works. - PALETTE COLOR (i.e. indexed RGB) - YBR_FULL, YBR_FULL_422, ... Compressed data is also not handled but of course part of the problem is not being able to decode undefined sequences at indicated above.
I think there are definitely problems in the dicom plug-in that should be fixed, but it seems impossible to fix them without example images that show the problems. All of the dicom images I have tried have opened correctly in gimp. Accordingly, I am going to set this bug report to NEEDINFO until examples of problems can be shown.
First accept my apologies, that only now was I made aware of this bug report. I had an email problem a year ago, and perhaps the email got lost then. I have learned a lot more about Dicom since I wrote the plug-in and I aware of the fact that there are various bugs in it that i have fixed in my non-public Dicom reading library, that I never entered in the gimp plug-in. I have quite a lot of images myself, and I will go through them and make sure that they load. In case I don't support them I will make sure that there is an error message issued. There is actually a general problem here and that is the fact that Dicom is a big and complex format and in order to have full support for it, it is necessary to be dependant on an external library. In the case of Dicom this would probably be the bsd-like licensed dcmtk (www.dcmtk.org). But introducing this dependancy would certainly remove the dicom plug-in from the default plugins. Thus I will keep the current code, but will try to improve it a bit. There is a similar problem, btw, with the creation of a plug-in for the HDF5 format.
Hmm. Given that DICOM is a format used mainly by people involved in medical imaging, I'm not sure it is so bad to make it depend on an external library. It may well be worth the extra dependency if it significantly improves the level of support, and particularly if it eases access to the metadata in dicom files. In any case I will now remove NEEDINFO from this bug report, since apparently you don't.
Created attachment 65951 [details] [review] Better handling of different dicom encodings I have now patched the plug-in so that it can read all Dicom images that I have in my collection. That includes several ones that the old plug-in crashed on. Please test the included patch with any private data files. If there are still problems, please send images to me privately.
Since my last comment I have verified that the new code can open at least some of the images mentioned in the bug report #166538, that was marked as a dup. That together with the fact that no negative feedback has been reported made me come to the conclusion that we should apply the patch in the cvs. Since I am not sure that I have write permission to the cvs anymore, I suggest that someone who does, will apply the patch and close the bug.
Thank you. Patch applied to both branches. 2006-05-26 Bill Skaggs <weskaggs@primate.ucdavis.edu> * plug-ins/common/dicom.c: applied patch from Dov Grobgeld with several fixes for dicom loading, fixes bug #163256.