Conquest 1.4.16alpha...1.4.16alpha5 released

    Hi Marcel,


    More problems. This time the problem was in nkiqrsop.cxx. I used DeleteVR() in places I thought I would not have a problem, wrong! I have removed them all from nkiqrsop.cxx, except for the Philips fix, where I do not think it will be a problem, and in 2 places where I know it will not be a problem. I think before this is released, DICOMObject::DeleteVR(VR* pVR) should be fixed as to not corrupt LastVR and LastVRG. Do you want to do this, or should I try?


    Posted the file here:
    http://www.bitsltd.net/Software/Softwar ... /index.php


    Bruce

    Hi Bruce,


    I appreciate your good work, but the warning stuff is going too far. I am now done except 'just' dgate.cpp and nkiqrsop.cpp. Fixing warnings may be a good thing - but I must now check several hundreds of changes in these files that you made just to get rid of the warnings, and these changes may introduce bugs by themselves. And the unrelevant changes hide some real changes as well. This is the last time I will do this - I will NOT accept warning changes for newer compilers after this. I spent about 6 hours checking your changes sofar; and I fear that I must spend several hours more on the last two files.


    On a less upset note: I feel your 00010101 date change does not really belong where you put it, Conquest should not be default change data just because another client will have trouble with it. I feel this is more work for an ImportConverter. And I will answer the queries you made in the code another time.


    Marcel

    Hi Marcel


    Sorry about all of the time fixing the warnings, but I think you will see a speed difference if you use member initialization. Also, it helps me trace out problems when I write my code. I think I spent over a week fixing them myself, but I did show me some possible bugs ( some were fixed and some are in the questions ). I also like it when Xcode give's me back "Success" instead of 2000+ warnings. When I was looking up what the warning meant and why they should be fixed, I saw a lot of commercial coders mentioning the their company had a no warning policy. I think dgate is a better product the most, if not all, of the pacs systems out-there. I want to make the code as clean and bug free as possible.


    Another question, did anyone use RLE encoding and If so, do you have any samples?


    Bruce

    Hi Marcel,


    Sorry to be a bother. I would like to explain some of the ways I use dgate, first would be a standard pacs server being sent and sending images, the second thing I do is take data from other pacs servers that no longer function, point dgate at them and rebuild the data base. dgate's feature to be able to recover images from servers that corrupt after the databases become to large ( Ex: eFilm, K-Pacs, OsiriX ) is a great one that I would like to keep. That is why the date is being fixed where I did it. I agree that we should fix the dates when the data comes in and I think where I did it will do that too.


    I am currently running version 16 on three 64 bit systems, one 32 bit and I do some coding on my power pc laptop (32 bit, big endian). One thing that bothers me is lines like this:
    iMaxRowsColumns = (unsigned int)(*((unsigned int*)pVR->Data));
    because of the size of int is unknown. I would like to change them to something like this:
    iMaxRowsColumns = 0;
    For (int i = 0; i < 4; i++) iMaxRowsColumns = (iMaxRowsColumns << 8) + *(char *)pVR->Data;
    Here, iMaxRowsColumns still gets to stay a faster int, but pVR->Data length is fixed at 4. This also fixes any endian problems. I could also use this method to remove most of the "#if NATIVE_ENDIAN == "'s.
    I have found this in the following files
    dgate.cxx:
    iVrSizeLimit = (unsigned int)(*((unsigned int*)pVR->Data)); //2 places.
    if ( *((unsigned int*)vrsilent->Data) == 0x44414544)
    nkiqrsop.cxx
    iVrSliceLimit = (unsigned int)(*((unsigned int*)vr->Data));
    if (vr) MaxCompression = (unsigned int)(*((unsigned int*)vr->Data));
    if (pVR->Length>8 && ((unsigned int*)pVR->Data)[1]>MAXNKICOMPRESSION)
    iMaxRowsColumns = (unsigned int)(*((unsigned int*)pVR1->Data));
    ExtractFrame(pDDO, (unsigned int)(*((unsigned int*)pVR2->Data)));
    piHeader = (int*)pVR->Data;
    piSrc = (int*)pVR1->Data;
    iMaxRowsColumns = (unsigned int)(*((unsigned int*)pVR->Data));
    trnsyn.cxx
    CurrentGroupLength = *(unsigned int *)vr->Data; //2 places.
    *((int*)vr->Data) = *((int*)vr->Data) + 4;//strlen("DICM");
    I would really like to fix all of these and also get rid of the "#if NATIVE_ENDIAN =="'s if possible, however, if that seems like to much work to check after I'm done, I could just changes the ones listed above or just make the "#if NATIVE_ENDIAN =="'s in nkiqrsop.cxx. Let me know what you want me to do.


    It looks easy to put the RLE decoder back, I could also do that when fixing nkiqrsop.cxx.


    Bruce

    Hi Bruce,


    About the date change: I see what you are getting at: but I believe it is a radical change of philosophy to let conquest fix data while it is being processed without at least being able to switch this option on or off. I guess that if we add a command "fixdates" that could be called in RetrieveResultConverter0 and QueryResultConverter0 (to be added) the result would be the same and configurable (albeit slower).


    About the (*int) change: I program under the assumption that int is 32 bits on all supported platforms (prove me wrong, although I used to program for DOS where this was not true). So the sizeof argument should not hold. The NATIVE_ENDIAN check is indeed not nice. I believe the correct way to improve this code is to add a suite of access functions to the VR class like:


    unsigned char Get_OB(index)
    INT16 Get_OW(index)
    UINT32 Get_UL(index)
    UINT16 Get_US(index)
    INT32 Get_SL(index)
    INT16 Get_SS(index)


    Where the chosen access type function reflects the given DICOM typecode. This would centralize the conversion and byte order issues. I generally code byte order independent read functions like you do, but this style would be inefficient in the parse functions that are called zillions of times.


    I will post an update of all files for you soon, and be my guest to work on those. But wait for a short while now... I really want to reread any change that you make. Experience has made me a defensive programmer: I do not beautify old working code unless there is a great need and/or I am otherwise working on it and testing all features being changed. I have seen lots of people do otherwise and introduce bugs in perfectly working code this way.


    An automated test suite would work wonders here - but that remains on the wish list.


    With kind regards,


    Marcel

    Hi Marcel,


    I look for the 8 byte ints, you are right, on 64 bit cpu, ints are put on the stack as 8 bytes, but all functions only use the lower 4.


    I love the idea of expanding the VR class.


    I hate the word slower, we could add a global or pass a bool to "parse*". I still like fixing illegal data, in or out. We could add a FIX_DATE_LZ.


    Bruce

    Hi Bruce,


    just a progress report: I merged the code and it seems to run without leaks now for jpeg2000. I will need a couple of hours extra to double check all changes with mine and your version, and make a list of issues to discuss.


    Before I release a WIP version, there is an issue with your ToJPG function that you might look at - it reports illegal jpg state 100, and fails to create jpg images (this is most easily tested from the ServerSide web viewer, select jpg as graphic format).


    Regards, and I do really appreciate your work ;->>>


    Marcel

    Hi Marcel,


    Easy fix.
    Change in ToJPG, at line 4746, from:


    jpeg_stdio_dest(&cinfo, f);
    rowWidth = cinfo.image_width * cinfo.num_components;
    while (cinfo.next_scanline < cinfo.image_height)


    to:


    jpeg_stdio_dest(&cinfo, f);
    rowWidth = cinfo.image_width * cinfo.num_components;
    jpeg_start_compress(&cinfo, TRUE);
    while (cinfo.next_scanline < cinfo.image_height)


    I forgot to start the conversion ( I have total recall, except when I forget ).
    Also forgot to free out if it errors!
    Change in ToJPG, at line 4721, from:


    if(f != NULL)
    {
    fclose(f);
    if (!append)unlink(filename);
    }

    to:
    if(f != NULL)
    {
    fclose(f);
    if (!append)unlink(filename);
    free(out);
    }

    Should we add ToJ2k? I can code just that part and send it to you to add to the file.


    Bruce

    Bruce,


    Here is a list of changes and questions for your code:


    - DICOM LIBRARY -
    trnsyn.cxx: the iOrgmode change breaks the reader-removed; removed date fix code (to discuss).
    cctypes.h: added another ifdef to fix for WIN64
    array.thh: Q6: agreed change to =operator; Q7: hm, unclear where this is used?
    aarq.hpp: (in many files) fixed ifndef UNUSED_ARGUMENT; Q1 ok
    dimsen.hpp: my question: what are the virtual destructors for?
    deivr.cxx: !!!Q2/Q5: keep ReleaseMemory default to TRUE; Q3: changed use Data[Length-1] to if Data && cLast;
    Q6: yes we need to fix it sometime - or at least detect the problem and give a DICOMError message


    - DGATE -
    dbsql.cpp: I kept the very common atof - your change would not compile
    dgate.cpp: I undid the checklargestmalloc change: it worked above 4GB for 64 bit systems, and the overflow would never occur
    The imagelistfiles had a correct %%s but incorrect second parameter
    I swapped some shadowed variable names to make less changes
    nkiqrsop.cpp: I returned the crude repackage, is is simpler, more efficient, and it does not leak - the memory is deleted very shortly after
    Do not set pVR->Length for a sequence, that is done nowhere else (2 times)
    Shorter fix for setting number of frames
    This if is really incorrect (msc warning): if (0x30 <= ((unsigned char*)pVR->Data)[ui] <= 0x39); replaced code block 4 times by much simpler atoi
    Merged your ToJpg fix


    Here is the WIP linux dist with all changed files:


    ftp://ftp-rt.nki.nl/outbox/mar…ver/linux1416alpha4WIP.7z


    Marcel

    Hi Marcel,


    I included a file with my changes: dgateAlpha4updates.


    Here is a direct link to the zipped files.


    http://www.bitsltd.net/images/stories/file/linux.zip


    What I would really like you do is to try and remove as many "#ifdef __GNUC__"s as possible. I put most of them there because it is easier to delete them rather than add them, they show where my changes were made, and they leave the old code there. For examples:, I added member initializations, they are faster and should be used in all builds; protected copy functions are great to find memory leaks and invalid pointers, they should be in all builds. For QUES7:, I made the copy private and watched for the errors and warnings. The warning came in pdu.cxx, the is where the copy function is used. You will find comments in there as to what I think is happening. I would like to see what you think. For the virtual destructors, I liked this blog http://blogs.msdn.com/b/oldnew…ve/2004/05/07/127826.aspx , comment # 1. Most of your changes I kept, but I had to remove most of the DeleteVR() changes because they cause a crash when doing a JPG to J2K conversion.


    What is the current windows system you are using to build, I'm thinking of setting up a windows box (or at least a virtual machine) to build on. Is the compiler open source?


    Bruce

    Hi Bruce,


    just looked at your changes and they are OK to me, as well as some contiuuous development on you side. I agree that I should try and delete the __GNUC__ ifdefs, but I first try to get it compiling and working again. I am not really convinced that the member initialisations make a speed difference, most are in objects that are not very often constructed.


    Note sure what DeleteVR() changes you are refering to, i did not see it in the changed code. I will have to look at the other stuf in more detail, but I am a bit short on time.


    I use microsoft compiler 14.00.50727.762. For testing conquest a virtual machine with win2000 or xp would be more than adequate. You can find the compiler for free in the Microsoft Windows SDK Update for Windows Vista. I use a command line only version, and debug with visual studio 2008 express.


    Marcel

    Hi Bruce,


    Thanks. I merged your code into my master sources; I found one bug (a shadow variable rename of 's' to 'str' left a single variable 's' unchanged). Please use these files as your master source if possible:


    ftp://ftp-rt.nki.nl/outbox/mar…questlinux1416alpha4_2.7z


    I have not yet played with it, nor had any time to look at the previously discussed aspects. I think it is now time to start working on the bug and wish list above.


    Marcel

    Hi Marcel.


    I think I have fixed AddAbstractSyntaxAlias, but is is only used in PrinterSupport and I have no way to test it. Maybe one day I can add unix printer support.


    I did most of my work in deivr and nkiqrsop.cpp. I think I fixed the LastVR and LastVRG problems. I also add some routines to deivr now used throughout nkiqrsop.cpp. I hope you like them. It made my code a lot easier to debug. I did not spread them throughout the other files, but I could if you want. I also put RLE back. After looking closely at ReplaceVR, I realized that it does not replace the VR, but instead replaces with a copy of the VR. So after each use, I deleted the original. I would like to remove all uses of ReplaceVR from the code, it looks like a mess in deivr.cxx and hard to follow. Are any of the Special Value Code Of VR ([[FIX_TMS]],[[RGB_TO_MONO_PLUS]],[[RGB_TO_MONO]]) used?
    Sorry, but I have made a mess of the nkiqrsop.cpp tabs and will try to fix them later, for now, just try to look at what I have done. :oops:
    I would move and build on 3 different systems (32,64,32BE).


    Changes were only made to your last set of files posted and are here:
    http://www.bitsltd.net/images/stories/file/dgateAlpha4Up.zip


    Bruce

    Hi Bruce,


    the printer support works on linux, but instead of printing it just writes the images into the printer_files directory. The windows GUI picks them up from there based on the log lines it reads.


    I believe the special ReplaceVR codes are used but maybe only in our internal software.


    Thanks for all the other work - but I have little time the next week to work on them...


    Marcel

    Bruce,


    I am testing 1.4.16alpha5 now. It is up for download on the usual place, but on linux the (lower) query option in the web server returns gibberish. I expect to solve this tonight. I like almost all your changes, but please do not modify any files for the time being - keeping up with you takes so much time that I cannot fix any reported issues. I would like to turn this version into a beta for general download.


    Marcel

    Hi Marcel,


    I would like to make one more small change. I have a system that has many instances of dgate running on it. Every time I update, I have to move several copies of dgate into each directory. With the small changes I have made alpha 4, dgate can be given the command -wDIR, where DIR is the location of dicom.ini and dgate.dic files. The command sets DicomDict and ConfigFile file locaters. Now I can put dgate in a bin directory and only have to update one copy. In dicom.ini, I can set the location of any of the other files, like maybe etc. I would like to make this change to the alpha 5 I just grabbed, last change, really.
    If you want, I can look into web server problem too. Also, if you want I can look into any issues that I can duplicate on my Mac (unix). Which one(s) do you want me to look at?


    Bruce

Participate now!

Don’t have an account yet? Register yourself now and be a part of our community!

Register Yourself Login