From 8b695d4a325b768e1bec82f41d74c49ebb906cc8 Mon Sep 17 00:00:00 2001 From: frog Date: Tue, 29 Jun 2004 23:43:19 +0000 Subject: [PATCH] * Test/TestReadWriteReadCompare.cxx was properly written (with a call to gdcmFile::SetImageData()) BUT since gdcmFile is brain damaged (see new comments in this file) we temporarily (sigh) move to a weaker form of test... * Test/CmakeList.txt: with the change to Test/TestReadWriteReadCompare.cxx we don't need to black list the following images anymore: - 8BitsUncompressedColor.dcm - OT-PAL-8-face.dcm - US-PAL-8-10x-echo.dcm * src/gdcmDocument.[h|cxx]: RE-Reverting to version 1.42 with the proper fixes and the beautified code ;-) This fixes the bug introduced in version 1.42 (when beautifying) that made the parsing of 8BitsRunLengthColor.dcm unproper. Note: ctest was blind to this bug (this means we need to still improve the test suite). The bug could be detected by using gdcmbin/bin/PrintDocument $GDCM_DATA/8BitsRunLengthColor.dcm or by using gdcmbin/bin/ReadWrite $GDCM_DATA/8BitsRunLengthColor.dcm and by displaying the (garbage) produced file temp.XDCM... --- ChangeLog | 21 ++ Testing/CMakeLists.txt | 4 - Testing/TestReadWriteReadCompare.cxx | 21 +- src/gdcmDocument.cxx | 304 ++++++++++----------------- src/gdcmDocument.h | 10 +- 5 files changed, 158 insertions(+), 202 deletions(-) diff --git a/ChangeLog b/ChangeLog index e1225a89..52fde1bb 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,3 +1,24 @@ +2004-06-30 Eric Boix + * Test/TestReadWriteReadCompare.cxx was properly written (with a call + to gdcmFile::SetImageData()) BUT since gdcmFile is brain damaged (see + new comments in this file) we temporarily (sigh) move to a weaker + form of test... + * Test/CmakeList.txt: with the change to Test/TestReadWriteReadCompare.cxx + we don't need to black list the following images anymore: + - 8BitsUncompressedColor.dcm + - OT-PAL-8-face.dcm + - US-PAL-8-10x-echo.dcm + * src/gdcmDocument.[h|cxx]: RE-Reverting to version 1.42 with the proper + fixes and the beautified code ;-) + This fixes the bug introduced in version 1.42 (when beautifying) + that made the parsing of 8BitsRunLengthColor.dcm unproper. + Note: ctest was blind to this bug (this means we need to still + improve the test suite). The bug could be detected by using + gdcmbin/bin/PrintDocument $GDCM_DATA/8BitsRunLengthColor.dcm + or by using + gdcmbin/bin/ReadWrite $GDCM_DATA/8BitsRunLengthColor.dcm + and by displaying the (garbage) produced file temp.XDCM... + 2004-06-29 Jean-Pierre Roux FIX : - remove Frog's beautified, but never checked 'Parse7FE0' code, - replace by uggly but working old code :-( diff --git a/Testing/CMakeLists.txt b/Testing/CMakeLists.txt index 42ffc05b..c20bc31e 100644 --- a/Testing/CMakeLists.txt +++ b/Testing/CMakeLists.txt @@ -113,10 +113,6 @@ SET(BLACK_LIST "irmPhlipsNew1.dcm" #png looks ugly "mriThruVPRO.dcm" #png looks ugly "US.3405.1.dcm" #looks exactly the same as US.1.2.dcm - "8BitsRunLengthColor.dcm" # Write dicom broken - "8BitsUncompressedColor.dcm" # Write dicom broken - "OT-PAL-8-face.dcm" # Write dicom broken - "US-PAL-8-10x-echo.dcm" # Write dicom broken ) # gdcm-ACR-LibIDO seems to be cut diff --git a/Testing/TestReadWriteReadCompare.cxx b/Testing/TestReadWriteReadCompare.cxx index 356ddbd9..939ab869 100644 --- a/Testing/TestReadWriteReadCompare.cxx +++ b/Testing/TestReadWriteReadCompare.cxx @@ -54,9 +54,22 @@ int TestReadWriteReadCompare(int argc, char* argv[]) int dataSize = file->GetImageDataSize(); void* imageData = file->GetImageData(); //EXTREMELY IMPORTANT - file->SetImageData(imageData, dataSize); + /// \todo Following line commented out because gdcmFile::SetImageData() is + /// brain dead: it sets ImageDataSize to its argument and PixelRead to a. + /// Later on, when writing gdcmFile::WriteBase() and because PixelRead == 1 + /// we call + /// PixelElement->SetLength( ImageDataSizeRaw ); + /// where we use ImageDataSizeRAW instead of ImageDataSize ! + /// But when the original image made the transformation LUT -> RGB, + /// ImageDataSizeRaw is the third of ImageDataSize, and there is no + /// reason (since we called gdcmFile::SetImageData) to use the Raw image + /// size... This "bug" in gdcmFile made that we had to black list + /// images 8BitsUncompressedColor.dcm, OT-PAL-8-face.dcm and + /// US-PAL-8-10x-echo.dcm... + /// In conclusion fix gdcmFile, and then uncomment the following line. + /// file->SetImageData(imageData, dataSize); file->WriteDcmExplVR( "TestReadWriteReadCompare.dcm" ); - std::cout << " 2..."; + std::cout << "2..."; //////////////// Step 3: @@ -71,7 +84,7 @@ int TestReadWriteReadCompare(int argc, char* argv[]) delete reread; return 1; } - std::cout << " 3..."; + std::cout << "3..."; // For the next step: int dataSizeWritten = reread->GetImageDataSize(); void* imageDataWritten = reread->GetImageData(); @@ -102,7 +115,7 @@ int TestReadWriteReadCompare(int argc, char* argv[]) delete reread; return 1; } - std::cout << " 4...OK." << std::endl ; + std::cout << "4...OK." << std::endl ; //////////////// Clean up: delete (char*)imageData; diff --git a/src/gdcmDocument.cxx b/src/gdcmDocument.cxx index 318fbce6..5811719d 100644 --- a/src/gdcmDocument.cxx +++ b/src/gdcmDocument.cxx @@ -3,8 +3,8 @@ Program: gdcm Module: $RCSfile: gdcmDocument.cxx,v $ Language: C++ - Date: $Date: 2004/06/29 14:38:29 $ - Version: $Revision: 1.43 $ + Date: $Date: 2004/06/29 23:43:19 $ + Version: $Revision: 1.44 $ Copyright (c) CREATIS (Centre de Recherche et d'Applications en Traitement de l'Image). All rights reserved. See Doc/License.txt or @@ -750,7 +750,7 @@ std::string gdcmDocument::GetEntryVRByName(TagName tagName) { */ std::string gdcmDocument::GetEntryByNumber(guint16 group, guint16 element){ TagKey key = gdcmDictEntry::TranslateToKey(group, element); -// TODO : use map methods, instead of multimap // JPR + /// \todo use map methods, instead of multimap JPR if ( ! tagHT.count(key)) return GDCM_UNFOUND; return ((gdcmValEntry *)tagHT.find(key)->second)->GetValue(); @@ -862,7 +862,7 @@ bool gdcmDocument::SetEntryByNumber(void *content, if ( ! tagHT.count(key)) return false; -/* Hope Binary field length is *never* wrong +/* Hope Binaray field length is *never* wrong if(lgth%2) // Non even length are padded with a space (020H). { lgth++; @@ -891,8 +891,7 @@ bool gdcmDocument::SetEntryLengthByNumber(guint32 l, guint16 group, guint16 element) { -// TODO : use map methods instead of multimap // JPR - + /// \todo use map methods, instead of multimap JPR TagKey key = gdcmDictEntry::TranslateToKey(group, element); if ( ! tagHT.count(key)) return false; @@ -963,7 +962,7 @@ void *gdcmDocument::LoadEntryVoidArea(guint16 Group, guint16 Elem) delete[] a; return NULL; } -// TODO : Drop any already existing void area ! // JPR + /// \todo Drop any allready existing void area! JPR SetEntryVoidAreaByNumber(a, Group, Elem); return a; } @@ -1250,7 +1249,7 @@ long gdcmDocument::ParseDES(gdcmDocEntrySet *set, long offset, long l_max, bool if (NewDocEntry->GetReadLength()==0xffffffff) { // Broken US.3405.1.dcm - Parse7FE0(); // to skip the pixels + Parse7FE0(); // to skip the pixels // (multipart JPEG/RLE are trouble makers) } else @@ -2017,6 +2016,7 @@ uint32_t gdcmDocument::FindDocEntryLengthOB(void) { { dbg.Verbose(1, "gdcmDocument::FindLengthOB: neither an Item tag " "nor a Sequence delimiter tag."); + fseek(fp, PositionOnEntry, SEEK_SET); errno = 1; return 0; } @@ -2392,30 +2392,28 @@ guint32 gdcmDocument::GenerateFreeTagKeyInGroup(guint16 group) return UINT32_MAX; } - /** * \brief Assuming the internal file pointer \ref gdcmDocument::fp - * is placed at the beginning of a tag (TestGroup, TestElement), - * read the length associated to the Tag. + * is placed at the beginning of a tag check whether this + * tag is (TestGroup, TestElement). * \warning On success the internal file pointer \ref gdcmDocument::fp - * is modified to point after the tag and it's length. + * is modified to point after the tag. * On failure (i.e. when the tag wasn't the expected tag * (TestGroup, TestElement) the internal file pointer * \ref gdcmDocument::fp is restored to it's original position. * @param TestGroup The expected group of the tag. * @param TestElement The expected Element of the tag. - * @return On success returns the length associated to the tag. On failure - * returns 0. + * @return True on success, false otherwise. */ -guint32 gdcmDocument::ReadTagLength(guint16 TestGroup, guint16 TestElement) +bool gdcmDocument::ReadTag(uint16_t TestGroup, uint16_t TestElement) { - guint16 ItemTagGroup; - guint16 ItemTagElement; + uint16_t ItemTagGroup; + uint16_t ItemTagElement; long PositionOnEntry = ftell(fp); long CurrentPosition = ftell(fp); // On debugging purposes //// Read the Item Tag group and element, and make - // sure they are respectively 0xfffe and 0xe000: + // sure they are what we expected: ItemTagGroup = ReadInt16(); ItemTagElement = ReadInt16(); if ( (ItemTagGroup != TestGroup) || (ItemTagElement != TestElement ) ) @@ -2430,11 +2428,36 @@ guint32 gdcmDocument::ReadTagLength(guint16 TestGroup, guint16 TestElement) dbg.Verbose(0, "gdcmDocument::ReadItemTagLength: wrong Item Tag found:"); dbg.Verbose(0, s.str().c_str()); fseek(fp, PositionOnEntry, SEEK_SET); + return false; + } + return true; +} + +/** + * \brief Assuming the internal file pointer \ref gdcmDocument::fp + * is placed at the beginning of a tag (TestGroup, TestElement), + * read the length associated to the Tag. + * \warning On success the internal file pointer \ref gdcmDocument::fp + * is modified to point after the tag and it's length. + * On failure (i.e. when the tag wasn't the expected tag + * (TestGroup, TestElement) the internal file pointer + * \ref gdcmDocument::fp is restored to it's original position. + * @param TestGroup The expected group of the tag. + * @param TestElement The expected Element of the tag. + * @return On success returns the length associated to the tag. On failure + * returns 0. + */ +uint32_t gdcmDocument::ReadTagLength(uint16_t TestGroup, uint16_t TestElement) +{ + long PositionOnEntry = ftell(fp); + + if ( !ReadTag(TestGroup, TestElement) ) + { return 0; } //// Then read the associated Item Length - CurrentPosition=ftell(fp); + long CurrentPosition = ftell(fp); guint32 ItemLength; ItemLength = ReadInt32(); { @@ -2447,206 +2470,111 @@ guint32 gdcmDocument::ReadTagLength(guint16 TestGroup, guint16 TestElement) return ItemLength; } -/** - * \brief Read the length of an exptected Item tag i.e. (0xfffe, 0xe000). - * \sa \ref gdcmDocument::ReadTagLength - * \warning See warning of \ref gdcmDocument::ReadTagLength - * @return On success returns the length associated to the item tag. - * On failure returns 0. - */ -guint32 gdcmDocument::ReadItemTagLength(void) -{ - return ReadTagLength(0xfffe, 0xe000); -} - -/** - * \brief Read the length of an expected Sequence Delimiter tag i.e. - * (0xfffe, 0xe0dd). - * \sa \ref gdcmDocument::ReadTagLength - * \warning See warning of \ref gdcmDocument::ReadTagLength - * @return On success returns the length associated to the Sequence - * Delimiter tag. On failure returns 0. - */ -guint32 gdcmDocument::ReadSequenceDelimiterTagLength(void) -{ - return ReadTagLength(0xfffe, 0xe0dd); -} - - /** * \brief Parse pixel data from disk for multi-fragment Jpeg/Rle files * No other way so 'skip' the Data * */ - - -void gdcmDocument::Parse7FE0 (void) { - +void gdcmDocument::Parse7FE0 (void) +{ gdcmDocEntry* Element = GetDocEntryByNumber(0x0002, 0x0010); if ( !Element ) return; - std::string Transfer = ((gdcmValEntry *)Element)->GetValue(); - if (Transfer == UI1_2_840_10008_1_2 ) - return; - if ( Transfer == UI1_2_840_10008_1_2_1 ) - return; - if ( Transfer == UI1_2_840_10008_1_2_2 ) //1.2.2 ??? A verifier ! - return; - if ( Transfer == UI1_2_840_10008_1_2_1_99 ) + if ( IsImplicitVRLittleEndianTransferSyntax() + || IsExplicitVRLittleEndianTransferSyntax() + || IsExplicitVRBigEndianTransferSyntax() /// \todo 1.2.2 ??? A verifier ! + || IsDeflatedExplicitVRLittleEndianTransferSyntax() ) return; - - int nb; - std::string str_nb=GetEntryByNumber(0x0028,0x0100); - if (str_nb == GDCM_UNFOUND ) { - nb = 16; - } else { - nb = atoi(str_nb.c_str() ); - if (nb == 12) nb =16; - } - - guint16 ItemTagGr,ItemTagEl; - int ln; - long ftellRes; - // -------------------- for Parsing : Position on begining of Jpeg/RLE Pixels + // ---------------- for Parsing : Position on begining of Jpeg/RLE Pixels - if ( Transfer != UI1_2_840_10008_1_2_5 ) { // !RLELossLessTransferSyntax + //// Read the Basic Offset Table Item Tag length... + guint32 ItemLength = ReadTagLength(0xfffe, 0xe000); + + //// ... and then read length[s] itself[themselves]. We don't use + // the values read (BTW what is the purpous of those lengths ?) + if (ItemLength != 0) { + // BTW, what is the purpous of those length anyhow !? + char * BasicOffsetTableItemValue = new char[ItemLength + 1]; + fread(BasicOffsetTableItemValue, ItemLength, 1, fp); + for (unsigned int i=0; i < ItemLength; i += 4){ + guint32 IndividualLength; + IndividualLength = str2num(&BasicOffsetTableItemValue[i],guint32); + std::ostringstream s; + s << " Read one length: "; + s << std::hex << IndividualLength << std::endl; + dbg.Verbose(0, "gdcmDocument::Parse7FE0: ", s.str().c_str()); + } + } + + if ( ! IsRLELossLessTransferSyntax() ) + { // JPEG Image - ftellRes=ftell(fp); - fread(&ItemTagGr,2,1,fp); //Reading (fffe):Basic Offset Table Item Tag Gr - fread(&ItemTagEl,2,1,fp); //Reading (e000):Basic Offset Table Item Tag El - if(GetSwapCode()) { - ItemTagGr=SwapShort(ItemTagGr); - ItemTagEl=SwapShort(ItemTagEl); - } - ftellRes=ftell(fp); - fread(&ln,4,1,fp); - if(GetSwapCode()) - ln=SwapLong(ln); // Basic Offset Table Item Length - - if (ln != 0) { - char * BasicOffsetTableItemValue= new char[ln+1]; - fread(BasicOffsetTableItemValue,ln,1,fp); - guint32 a; - for (int i=0;i1) { // skipping (not reading) RLE Segments - for(unsigned int k=1; k<=nbRleSegments-1; k++) { + // skipping (not reading) RLE Segments + if (nbRleSegments>1) { + for(unsigned int k=1; k<=nbRleSegments-1; k++) { RleSegmentLength[k]= RleSegmentOffsetTable[k+1] - RleSegmentOffsetTable[k]; ftellRes=ftell(fp); - - fseek(fp,RleSegmentLength[k],SEEK_CUR); + printf (" Segment %d : Length = %d x(%x) Start at %x\n", + k,(unsigned)RleSegmentLength[k], + (unsigned)RleSegmentLength[k], (unsigned)ftellRes); + SkipBytes(RleSegmentLength[k]); } } + RleSegmentLength[nbRleSegments]= fragmentLength - RleSegmentOffsetTable[nbRleSegments]; ftellRes=ftell(fp); - fseek(fp,RleSegmentLength[nbRleSegments],SEEK_CUR); - - // ------------------ end of scanning fragment pixels - - ftellRes=ftell(fp); - fread(&ItemTagGr,2,1,fp); // Reading (fffe) : Item Tag Gr - fread(&ItemTagEl,2,1,fp); // Reading (e000) : Item Tag El - if(GetSwapCode()) { - ItemTagGr=SwapShort(ItemTagGr); - ItemTagEl=SwapShort(ItemTagEl); - } + printf (" Segment %d : Length = %d x(%x) Start at %x\n", + nbRleSegments,(unsigned)RleSegmentLength[nbRleSegments], + (unsigned)RleSegmentLength[nbRleSegments],(unsigned)ftellRes); + SkipBytes(RleSegmentLength[nbRleSegments]); } + + // Make sure that at the end of the item we encounter a 'Sequence + // Delimiter Item': + if ( ! ReadTag(0xfffe, 0xe0dd) ) + { + dbg.Verbose(0, "gdcmDocument::Parse7FE0: no sequence delimiter item"); + dbg.Verbose(0, " at end of RLE item sequence"); + } } - return; } - + + /** * \brief Compares two documents, according to \ref gdcmDicomDir rules diff --git a/src/gdcmDocument.h b/src/gdcmDocument.h index b20b8491..2bab7aba 100644 --- a/src/gdcmDocument.h +++ b/src/gdcmDocument.h @@ -3,8 +3,8 @@ Program: gdcm Module: $RCSfile: gdcmDocument.h,v $ Language: C++ - Date: $Date: 2004/06/29 14:38:29 $ - Version: $Revision: 1.19 $ + Date: $Date: 2004/06/29 23:43:20 $ + Version: $Revision: 1.20 $ Copyright (c) CREATIS (Centre de Recherche et d'Applications en Traitement de l'Image). All rights reserved. See Doc/License.txt or @@ -180,7 +180,6 @@ public: virtual std::string GetEntryByNumber (guint16 group, guint16 element); virtual std::string GetEntryVRByNumber(guint16 group, guint16 element); virtual int GetEntryLengthByNumber(guint16 group, guint16 element); - protected: virtual bool SetEntryByName (std::string content, std::string tagName); virtual bool SetEntryByNumber(std::string content, @@ -229,9 +228,8 @@ private: guint16 ReadInt16(); guint32 ReadInt32(); void SkipBytes(guint32); - guint32 ReadTagLength(guint16, guint16); - guint32 ReadItemTagLength(); - guint32 ReadSequenceDelimiterTagLength(); + bool ReadTag(uint16_t, uint16_t); + uint32_t ReadTagLength(uint16_t, uint16_t); void Initialise(); bool CheckSwap(); -- 2.48.1