Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Image masking #715

Merged
merged 51 commits into from
Jan 20, 2020
Merged

Image masking #715

merged 51 commits into from
Jan 20, 2020

Conversation

monsieurgustav
Copy link
Contributor

@monsieurgustav monsieurgustav commented Nov 20, 2019

Description

Support image masking to control 3D reconstruction.

Features list

  • Add imageMasking app implementing an HSV based keying
  • Update featureExtraction to filter features based on image masks
  • Update prepareDenseScene to output undistorted images with alpha channel based on masks

Implementation remarks

Federica Mauriello and others added 30 commits October 4, 2019 10:39
[doc] INSTALL.md: add boost-timer and lz4 to vcpkg install command
As they can be included in the SfMData
* always use the file extension in the UID
* use file system date/time if there is no date/time metadata
…rting with valid poses

When starting with existing poses:
1/ If we don't have any landmark, we need to triangulate them from the known poses.
2/ But even if we already have landmarks, we need to try to triangulate new points with the current set of parameters.
Workaround for a build incompatibility between prettyprint and eigen
Add Oneplus 5T sensor width to the database
Added working infos in LG G6 camera sensors
…olume

If we have a mask in input to filter the values, all the similarity voxels will have exactly the worst value 255. So the output of retrieveBestZ should be "no valid depth" instead of returning the last plane of the similarity volume.
@fabiencastan
Copy link
Member

It works fine to mask features in FeatureExtraction, but I get troubles when using it in input of PrepareDenseScene:
PrepareDenseScene gives me: "Invalid image mask size: mask is ignored." probably due to image orientation.

The surprising thing is that the orientation of the mask is the same when used on FeatureExtraction where there is no problem...

- move to c++14 to use auto in function return type without explicit
information.
- additional typename/using needed to resolve template type value
@natowi natowi added the feature label Nov 20, 2019
@monsieurgustav
Copy link
Contributor Author

The issue in PrepareDenseScene is fixed

template<> class DataType<aliceVision::image::RGBColor>
{
public:
typedef uchar value_type;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

prefer using value_type = uchar; rather than typedef

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's an OpenCV template specialization.
Other specializations of this template in OpenCV are written using typedef, so I prefer to use the same syntax.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

semantically they are the same thing, it's just a raccomended way to use using
https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#Rt-using

I think it's better to be consistent in our code, maybe at some point they will change that in opencv too

namespace imageMasking{

namespace{
float clamp(float value)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do not indent for namespace

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did indent anonymous namespace to make obvious that included functions are local. I feel it's useful and does not hurt readability too much because only few functions are defined in anonymous namespace.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for completely new file I'd prefer to stick with the new formatting rules, we need to start at some point. But it\s true that we need to do that in batch at some point.

src/aliceVision/imageMasking/imageMasking.cpp Show resolved Hide resolved
src/aliceVision/imageMasking/imageMasking.cpp Outdated Show resolved Hide resolved
src/aliceVision/imageMasking/imageMasking.cpp Show resolved Hide resolved
/**
* @brief Create a mask based on the hue of each pixel.
*
* @param[in] result The resulting binary mask.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[out]

*/
void hsv(OutImage& result, const InImagePath& inputPath, float hue, float hueRange, float minSaturation, float maxSaturation, float minValue, float maxValue);

void postprocess_invert(OutImage& result);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please add the doxygen if these are meant to be exposed

endif(NOT CXX11_COMPILER)
set(CMAKE_CXX_STANDARD 11)
set(CMAKE_CXX_STANDARD_REQUIRED ON)
## ==============================================================================
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

# C++11
set(CMAKE_CXX_STANDARD 11)
# C++14
set(CMAKE_CXX_STANDARD 14)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if we go with this, please update INSTALL.md, at the beginning

C/C++ compiler (gcc or visual studio or clang) with C++14 support (i.e. gcc >= 5, clang >= 3.4, msvc >=19).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, good point. I will fix that.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just checking, I guess it does not affect MeshroomMaya, if i remember correctly the gcc version used to compile maya plugins tended to be very conservative in the past. Dunno if it is still the case, just wanted to remind that in case.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For maya 2019
https://around-the-corner.typepad.com/adn/2019/01/maya-2019-api-updates-guide.html
Seems to be fine actually, maybe to check windows because they require Visual Studio 2015 Update 3 that sports msvc 19.00 while the full c++14 comes with later versions. I don't know if cmake check the compiler must fully support the standard or just some of it

for previous versions i guess it depends again on how strict is cmake and if the needed feature was already implemented
https://around-the-corner.typepad.com/adn/2018/02/compiler-versions-for-maya-2018.html

@@ -162,26 +162,29 @@ function(alicevision_add_software software_name)
list(APPEND SOFTWARE_SOURCE "${CMAKE_CURRENT_BINARY_DIR}/${software_name}_version.rc")
endif()

add_executable(${software_name} ${SOFTWARE_SOURCE})
add_executable(${software_name}_exe ${SOFTWARE_SOURCE})
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not a big fan of this. I imagine it's because of name conflicts.
I think it makes the cmake code less readable, as one expect that the target name given is used to define the target with that name. And then in the code if u need to do something with the target you need to know that a suffix is added.

Maybe instead of masking we can use segmentation, as in this case we are more generating the mask by processing the image rather than performing actions with a mask. That does not solve the name conflict but maybe we give the library a different, more general, broader meaning (eg imageProcessing on the same line as imgproc of opencv)

Copy link
Member

@fabiencastan fabiencastan Nov 21, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't like the idea of having to play with name variantes, just to avoid stupid cmake conflicts.
I agree that the exe suffix is hidden, but we cannot make the alias explicit as the output will get this name. It looks to me as the least worst solution, but I'm open to other suggestions if you have better idea... other than playing with name variantes which is a pain we have for too long.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • i was not suggesting to use alias or variants, i was just arguing that in cv masking and segmentation are different, and here we are doing segmentation (ie generating the mask) rather than masking (performing operations with existing masks, as eg in feature extraction). Nothing to do with the cmake conflict. That being said, the suggestion of having a lib of broader spectrum like imageProcessing (other algo may coming up in the future) would help to avoid the conflict.

  • to at least preserve the encapsulation (ie not see the use of suffix _exe outside the call to add_software) we can do this for the targets with optional dependencies:

set(<targetname>_LINKS aliceVision_sfmData aliceVision_sfmDataIO etc etc)
if(ALICEVISION_HAVE_CCTAG)
  list(APPEND <targetname>_LINKS CCTag::CCTag)
endif()
alicevision_add_software(<targetname>
      SOURCE main_<targetname>.cpp
      FOLDER ${FOLDER_SOFTWARE_PIPELINE}
      LINKS <targetname>_LINKS ...

at least everything stays in alicevision_add_software and we have not to go back to the target and use target_link_libraries. A bit more verbose but it seems cleaner to me. :-)

…aleThreshold

- option to load another image than the original for creating the mask
like an input depth map provided by a capture device. In that case,
rescale the output mask to the original image size.
- new algorithm autoGrayscaleThreshold using Otsu binarization
- add to_lower in algorithm stringToEnum conversion
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants