Removal of DMIP::Image copy constructor?

[Rosenstiel__Marcus]

Hi,

we started to use the DMIP extensions extensively with the introduction of DMIP 1.0. Recently (Ipp 7.0) the interface of the DMIP::Image class was changed and the copy constructor was moved to a protected scope

// image.h
protected:
Image(Image&) {}

Our image classes have been extended by a function to convert them to a DMIP::Image. Due to the above change all code was rendered useless and there seems to be no appropriate method to return a DMIP::Image from own functions as this always incorporates the use of the copy constructor.

Alternatively we tried to use the copy-by-pointer function but this creates a deep copy of the image data as well.

For sure there is a problem with dangling pointers but this problems also occurs using the standard constructor. Is there any other convenient way to convert own image classes which provide the needed data pointers etc. to a DMIP::Image using a member method? Calling the complete constructor of DMIP::Image every time is really annoying.

What was the reason for this drastic change?

Best regards,

Marcus

[SergeyKostrov]

>>...Is there any other convenient way to convert own image classes which provide the needed
>>data pointers etc. to a DMIP::Image using a member method?

Hi Marcus,

What about a C++ operator '='? Take a look at a short example. Even if it does the opposite conversion,
that is CDMIPImage -> CImage, you can use a similar technique:

...
class CDMIPImage
{
public:
CDMIPImage( void )
{
m_piData = ( int * )new int[2];

m_piData[0] = 555;
m_piData[1] = 777;
};

virtual ~CDMIPImage( void )
{
if( m_piData !=NULL )
{
delete []m_piData;
m_piData = NULL;
}
};

int *m_piData;
};

class CImage
{
public:
CImage( void )
{
m_piData = NULL;
};

virtual ~CImage( void )
{
};

CImage & operator = ( const CDMIPImage & dim )
{
m_piData = dim.m_piData;

return ( CImage & )*this;
};

int *m_piData;
};
...

void main( void )
{
CDMIPImage dim;
CImage im;

im = dim;

printf( "%ld %ld\n", im.m_piData[0], im.m_piData[1] );
}

Don't forget about correctmemory management because both objectswill share the same memory:

int *m_piData

Best regards,
Sergey

[Rosenstiel__Marcus]

Hi Sergey,

To assign CImage to DMIP::Image using operator=() we must implement the assignment within image.h that is changing intel sources which at the same time breaks the coherence of the 3rd party code. To get a short term solution we removed the copy constructor from the header to get the default copy constructor back which also breaks our 3rd party code coherence :-(.

However it may be possible to derive from DMIP::Image, implement the copy constructor, use only the derived class and use the DMIP::Image assignment operator to assign the derived class to the base class. Nevertheless there will be still always multiple lines of code:
DMIP::MyImage mim = cImage.ToMyImage();
DMIP::Image im;
im = myderivedObject;
instead of
DMIP::Image im = cImage.ToMyImage();
which is still not possible :-(.

I still have no idea why the copy constructor was removed. The responsibility for correct memory management starts at the moment when a DMIP::Image is constructed using a data-pointer.

Best regards,

Marcus

[Igor_B_Intel1]

Hi,
Before IPP 7.0 there was only default copy-constructor in DMIP::Image and it didn't work correctly. Thats why it using was prohibited.

Igor

[SergeyKostrov]

Hi Marcus,

Please take a look at a 2nd Test-Case and it does assignment from CImage -> CDMIPImage.Compare it
with the 1st Test-Case.

...
class CImage
{
public:
CImage( void )
{
m_piData = ( int * )new int[2];

m_piData[0] = 555;
m_piData[1] = 777;
};

virtual ~CImage( void )
{
if( m_piData != NULL )
{
delete [] m_piData;
m_piData = NULL;
}
};

int *m_piData;
};

class CDMIPImage
{
public:
CDMIPImage( void )
{
m_piData = NULL;
};

virtual ~CDMIPImage( void )
{
};

CDMIPImage & operator = ( const CImage &im )
{
m_piData = im.m_piData;

return ( CDMIPImage & )*this;
};

int *m_piData;
};
...

void main( void )
{
CImage im;
CDMIPImage dim;

dim = im;

printf( "%ld %ld\n", dim.m_piData[0], dim.m_piData[1] );
}

I tested both Test-Cases and I think some finalsolution could be based on 'operator=(...)' approach.
Anyway, I would try-test several "prototypes" beforeselecting a best one.

Best regards,
Sergey

[SergeyKostrov]

>>...instead of
>>DMIP::Image im = cImage.ToMyImage();
>>which is still not possible :-(.

You could consider thesesolutions:

//1
...
MyImage im;
DMIPDerived dim;
dim.ToMyImage( &im );
...

// 2 - as a functionwith 2parameters
...
MyImage im;
DMIP dim;
DMIPtoMyImage( [in] &dim, [in-out] &im );
...

Best regards,
Sergey

[Rosenstiel__Marcus]

@Sergey
Thank you very much for your help. Unfortunately this still means we have to touch each line which calls the copy constructor at the moment :-(.

@Igor
Could you explain what kind of error the old copy constructor will produce or even give an example? We already did a software rollout to our customers which includes the use of the copy constructor :-(.

Best regards,

Marcus

[SergeyKostrov]

>>...Unfortunately this still means we have to touch each line which calls the copy constructor at the moment :-(

If you would provide a small example ofhow it looks like in a real form it would be helpful. Code modifications are inevitable when some new functionality replacessome old functionality.

Good Luck and keep everybody informed!

Best regards,
Sergey

PS:
Sorry for another proposal... :) Take a look at the Test-Case 2 and I think it could be changed to ( or something like this ):

...
CDMIPImage & operator >>= ( const CImage &im )
{
im.m_piData = m_piData;// I have not tested it andI'll do itsome time later...

return ( CDMIPImage & )*this;
};

int *m_piData;
};
...

void main( void )
{
CImage im;
CDMIPImage dim;

dim >>= im; // If you don't like '>>' you can still use '='
...
}

[Rosenstiel__Marcus]

Ok,

here is my explanation why this change hurts me so explicitly:
Our image classes are not fully template based, that is the datatype and channels are burried in a base class ImageBase and all typed classes are inherited from that class i.e. ImageBase<-Image8u<-Image8uC1.

I implemented the conversion using the NVI pattern. So there is a function DMIP::Image ToDMIPImage() in ImageBase which calls the non-public virtual function DMIP::Image ToDMIPImageImpl() which leads to the call of the overridden implementation in the derived classes e.g.
// Image8uC1
DMIP::Image ToDMIPImageImpl()
{
return ImageBase::ToDMIPImageGen();
}

as each derived class has typedef'ed value_type and ipp_channels to avoid switch/case blocks. Then the non-public ImageBase::ToDMIPImageGen is called which is:
template
DMIP::Image ToDMIPImageGen()
{
return DMIP::Image(DataPtr(),
static_cast(type2ippdatatype::value) ,
static_cast(C),
Size(), Step());
}
There are multiple calls of the copy-constructor during the "conversion". Finally it looks like this in the algorithms:

Image8uC1 myImage
DMIP::Image myDmip = myImage.ToDMIPImage(); // calling the copy constructor

I have already refactored the code returning a class which wraps a std::tr1::shared_ptr<:IMAGE> and overloads the operator*() so that the copy-by-pointer constructor is called while non of the lines like the two above have to be changed...unfortunately the copy-by-pointer constructor leads to a deep copy of the image. I'm willing to change the backend of our image library if the above two lines are still working and I don't have to change them to something like

Image8uC1 myImage;
DMIP::Image myDmip; // calling the default constructor
myDmip = myImage.ToDMIPImage(); // calling the assignment operator

which is obviously possible as we have discussed in the above postings.

At the moment I'm a little bit worried that our released code may be unstable if the default copy constructor is not working correctly.

Best regards,

Marcus

[SergeyKostrov]

Thanks, Marcus!

>>...Unfortunately the copy-by-pointer constructor leads to a deep copy of the image...

Possibly affecting performance and this is not a good thing.

>>..I'm a little bit worried that our released code may be unstable if the default copy constructor is not working correctly.

In that case I wouldn't delay testing of Release configuration of applications.

Best regards,
Sergey

[SergeyKostrov]

Quoting Sergey Kostrov

PS:
Sorry for another proposal... :) Take a look at the Test-Case 2 and I think it could be changed to ( or something like this ):

...
CDMIPImage & operator >>= ( const CImage &im )
{
im.m_piData = m_piData;// I have not tested it andI'll do itsome time later...

return ( CDMIPImage & )*this;
};

int *m_piData;
};
...

Just tested: 'const' specificator has to be removed. It works.

Anyway, I really like your template-based solution because it easily takes into account different types of images!

Best regards,
Sergey