Release 4.2 Code Reviews (Sandbox)

From IGSTK

Jump to: navigation, search

Contents

Classes Scheduled for Code Reviews

These classes are ready for immediate review.

Please fix the code when you do the review

  • SO: Sebastian Ordas
  • ZY: Ziv Yaniv
  • PC: Patrick Cheng
  • AE: Andinet Enquobahire
  • MA: Michel Audette
  • OG: Ozgur Guler

Core igstk components:

Name Author Reviewer 1 Reviewed Code Fixed Reviewer 2 Reviewed Code Fixed Moved
igstkPivotCalibration ZY AE PC
igstkPivotCalibrationFLTKWidget ZY AE PC
PivotCalibrationFLTKWidget example ZY AE PC

Example applications and utility classes:

Name Author Reviewer 1 Reviewed Code Fixed Reviewer 2 Reviewed Code Fixed Moved
Examples/TransformWriters/igstkTransformXMLFileWriterBase ZY MA 05/18/09 05/18/09 PC
Examples/TransformWriters/igstkTransformFileWriter ZY MA 05/18/09 05/18/09 PC
Examples/TransformWriters/igstkRigidTransformXMLFileWriter ZY MA 05/18/09 05/18/09 PC
Examples/TransformWriters/igstkAffineTransformXMLFileWriter ZY MA 05/18/09 05/18/09 PC
Examples/TransformWriters/TransformWriterExample ZY MA 05/18/09 05/18/09 PC
Sandbox/Examples/PivotCalibrationFLTKWidget/PivotCalibrationFLTKWidgetExample ZY AE PC

Code to be moved into main IGSTK

Video Grabber

Source code

Name Author Reviewer 1 Reviewed Code Fixed Reviewer 2 Reviewed Code Fixed Moved
igstkVideoImager OG ZY 6/9/09 Yes
igstkWebCamWinVideoImager OG ZY 6/9/09 Yes
igstkOpenIGTLinkVideoImager OG ZY 6/9/09 Yes
igstkVideoImagerTool OG ZY 6/9/09 Yes
igstkWebCamWinVideoImagerTool OG ZY 6/9/09 Yes
igstkOpenIGTLinkVideoImagerTool OG ZY 6/9/09 Yes
igstkFrame OG ZY 6/9/09 Yes
igstkVideoFrameSpatialObject OG ZY 6/9/09 Yes
igstkVideoFrameRepresentation OG ZY 6/9/09 Ye

Examples

Name Author Reviewer 1 Reviewed Code Fixed Reviewer 2 Reviewed Code Fixed Moved
VideoFrameGrabberAndViewerOpenIGTLink OG
VideoFrameGrabberAndViewerWebcam OG

igstk::Frame

Ziv review

  1. SetFrameDimensions() - It is implicitly assumed that pixel values are in [0,255], memory is allocated using malloc(width*height*numberOfChannels), one byte per channel. Is this the case with all frame grabbers?

<OG> yes, one byte per channel -> comes into VideoImager WIKI page

  1. SetFrameDimensions(), return value of malloc not checked.

<OG> if malloc fails, the igstk::Frame class will log it (logging state FATAL) and throw an exception -> later event from state machine

  1. Data is kept in the m_ImagePtr which has to be consistent with the m_Width and m_Height and m_NumberOfChannels. This can be invalidated using the SetImagePtr method.

<OG> SetImagePtr() is now private only used from VideoImager (friend class)

  1. SetFrameDimensions() Can cause a memory leak as new memory is allocated and the old isn't released.

<OG> SetFrameDimensions() is now private only used from VideoImagerTool once (friend class)

  1. The copy constructor performs shallow copy, results in pointing to the same memory from multiple locations without using a reference counting mechanism. Dangerous when the memory is released.
  2. Destructor causes a memory leak, code commented out. The commented code will crash the system after using the copy constructor as the second object that is still alive can try to access memory that was released in the destruction of the first object. To avoid these problems the copy constructor should either perform deep copy + destructor releases memory or use smart pointers (reference counting).

<OG> not yet fixed

  1. For color images, what color space are we assuming RGB? Do all our image sources produce color images as RGB?

<OG> the VideoComponent works with RGB. If the device provides e.g. UYVY as it is the case with the ImagingSource Converter you can convert it like in igstkImagingSourceVideoImager.cxx

igstk::VideoImagerTool

Ziv review

  1. void SetFrameDimensions( unsigned int * );
    void GetFrameDimensions( unsigned int * );
    Should change the parameter type to ensure safety, perhaps to itk::Size<3> instead of expecting an array with three entries? Also, the number of channels supported is either 1 or 3 at least this is assumed in FrameSpatialObject and it is not validated here.
  2. GetFrameFromBuffer()
    GetTemporalCalibratedFrame()
    AddFrameToBuffer()
    Methods catch exceptions and then print an error to cout. Change to placing error input onto the state machine? Probably requires changes to the state machine.
  3. The ivar m_PixelDepth doesn't seem to be used. This is consistent with the assumption that pixels are in [0,255] see corresponding comment on igstk::Frame::SetFrameDimensions().

<OG> m_PixelDepth will be used later e.g. with 12 bit grayscale images.

igstk::VideoImager

igstk::VideoFrameSpatialObject

Ziv review

  1. m_CalibrationTransform defined in header but doesn't appear to be used. Not clear what is its purpose, probably superseded by the coordinate system found in the spatial object?

<OG> m_CalibrationTransform removed;

  1. Line 189, empty loop?

<OG> empty loop removed;

igstk:: WebcamWinVideoImager

Ziv review

  1. CreateErrorCodeList(), the error codes are with regard to the micron tracker and not relevant to webcams. Perhaps relevant for a separate class that supports grabbing the frames from micron.

<OG> error list removed;

OpenIGTRelated classes

http://public.kitware.com/IGSTKWIKI/index.php/Release_OpenIGTLink_related_Reviews_(Sandbox)

Scene Graph Visualization

  1. Code review
    • Sandbox/Source/igstkSceneGraph.cxx
    • Sandbox/Source/igstkSceneGraph.h
    • Sandbox/Source/igstkSceneGraphNode.cxx
    • Sandbox/Source/igstkSceneGraphNode.h
    • Sandbox/Source/igstkSceneGraphObserver.h
    • Sandbox/Source/igstkSceneGraphUI.cxx
    • Sandbox/Source/igstkSceneGraphUI.h
  2. Add #cmakedefine in igstkConfigure.h.in
  3. Update the "HelloWorld" example with scene graph visualization support. Move the "IGSTKSandbox/IGSTK/Examples/HellowWorld" to main IGSTK


TrackerController and other utility code

  1. Tested building library in the Sandbox
  2. Need to do the same in the main IGSTK, and remove the duplicated code in Sandbox
Retrieved from "http://public.kitware.com/IGSTKWIKI/index.php/Release_4.2_Code_Reviews_%28Sandbox%29"
Personal tools
TOOLBOX
LANGUAGES