search

pmoulon / CMVS-PMVS

This software (CMVS) takes the output of a structure-from-motion (SfM) software as input, then decomposes the input images into a set of image clusters of managable size. An MVS software can be used to process each cluster independently and in parallel, where the union of reconstructions from all the clusters should not miss any details that can be otherwise obtained from the whole image set. CMVS should be used in conjunction with an SfM software Bundler and an MVS software PMVS2 (PMVS version 2).
http://opensourcephotogrammetry.blogspot.com/
939 stars 464 forks source link

Optimisation depends on uninitialized memory #35

Closed nh2 closed 5 years ago

nh2 commented 5 years ago

PMVS2 is nondeterministic because its results depend on uninitialised memory.

This can be easily detected by valgrind, which says More than 10000000 total errors detected. I'm not reporting any more. when pmvs2 is run on some inputs.

Reason

The Patch class does not initialise its field

  // scaling factor corresponding to one pixel difference
  float m_dscale;

in the constructor https://github.com/pmoulon/CMVS-PMVS/blob/a0672745a0ba1d33990e623aee60122522eb215d/program/base/pmvs/patch.h#L13-L20

However, this field is read in setScales() in https://github.com/pmoulon/CMVS-PMVS/blob/a0672745a0ba1d33990e623aee60122522eb215d/program/base/pmvs/patchOrganizerS.cc#L678-L689

Initialising m_dscale to 0 or 1 fixes all valgrind errors.

Question

I am not quite sure what the initial value of m_dscale should be. Should it be 0 or 1?

Thanks @pmoulon!

nh2 commented 5 years ago

Likely fix for the issue in PR #37.

pmoulon commented 5 years ago

Since I did not write the code I will have to check which value makes sense and in which context the variable is used.

nh2 commented 5 years ago

@pmoulon Thanks! I am reasonably sure that 0 is the correct initial value (see also #37), but having you confirm it would be a great assurance.