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

Basic image processing module #24

Closed
syncle opened this issue May 25, 2017 · 6 comments
Closed

Basic image processing module #24

syncle opened this issue May 25, 2017 · 6 comments
Assignees

Comments

@syncle
Copy link
Contributor

syncle commented May 25, 2017

During the process of implementing OdometryRGBD, we found a need of basic image processing modules (e.g., pixel pointer, downsampling, filtering, creating pyramid image) that are generally useful for OdometryRGBD, texturing meshes, and so on.

Plans:

  • Put image processing functions into core/geometry/image.cpp/h
  • Some useful image functions are distributed in the individual projects, we are going to move them too after discussion.
  • I am not going to introduce heavy library for image processing. Instead, I will implement reasonably efficient functions: such as separable 1D filtering instead of 2D filtering.
  • I will add TestImageProcess.cpp/h in Test module for debugging and demonstrating implemented functions.
  • If OpenCV is available, CMake tool-chain will link it for utilizing higher level functions such as ORB.

Jaesik

@syncle syncle self-assigned this May 25, 2017
@qianyizh
Copy link
Collaborator

In test, the module name can be TestImage

Also, since they are in Core now, please add python bindings to these functions. I suggest we split it into two pull requests: first is just the change to the cpp files; second is the python binding (and tests in python).

If OpenCV is available, CMake tool-chain will link it for utilizing higher level functions such as ORB.

This is lower priority, we don't need to fix it in near future (i.e., before the first launch).

@syncle
Copy link
Contributor Author

syncle commented May 25, 2017

Questions.

  • I guess we need a function that transforms FloatImage to uint_8 or uint_16 image. WriteImage does not have this functionality.
  • I am bit confused with CreateImageFromFile and ReadImage. CreateImageFromFile is only used in TestTriangleMesh.cpp. What is recommended way to read an image? Do we need both function?
  • All the image processing functions assumes the image is float type. How about constrain input arguments of downsampling, filtering, creating pyramid image functions to be FloatImage?

@syncle syncle closed this as completed May 25, 2017
@syncle syncle reopened this May 25, 2017
@qianyizh
Copy link
Collaborator

I guess we need a function that transforms FloatImage to uint_8 or uint_16 image. WriteImage does not have this functionality.

Good point. Play with it. I did not thoroughly design the Image module. Also, I have been considering removing the FloatImage class for a long time. It only has an sub-pixel functionality and used in one app. Maybe we just remove it and use Image with 4-bit channel instead.

I am bit confused with CreateImageFromFile and ReadImage. CreateImageFromFile is only used in TestTriangleMesh.cpp. What is recommended way to read an image? Do we need both function?

ReadImage is in IO. It is actually bad design (due to some history reasons). My current plan is to wrap these IO functions into good design, like the factory functions. An analog is in dealing with PointCloud. I use ReadPointCloud in the IO module only internally in Open3D, and expose CreatePointCloudFromFile as external interface (e.g., in python binding).

All the image processing functions assumes the image is float type. How about constrain input arguments of downsampling, filtering, creating pyramid image functions to be FloatImage?

I am actually thinking of removing FloatImage class. I don't know, maybe it makes sense to keep it? And use it as the type of parameters used in the image processing functions? In this way I would vote for keeping it, since now it has a clear meaning: Image is for conventional images; FloatImage is for images used for image processing in Open3D; it is recommended to use Image just for IO and related stuff, and always convert it to FloatImage before any processing.

@syncle
Copy link
Contributor Author

syncle commented May 25, 2017

I actually agree with your original idea: removing FloatImage because

  • It might be confusing and not consistent with other data structures: we don't have FloatPointCloud, or we don't want to have DoubleImage. I had this confusion when I tried to use Image class in Open3D for the first time.
  • In many cases, we can write an app only using FloatImage, which means Image can be isolated.

I think we may unite FloatImage and Image, and simply the image processing functions are only activated if its data type is 32bit float. I originally thought we make FloatImage is only for internal usage and not unveiled to users, but existence of FloatImage still would give confusion.

How do you think?

@qianyizh
Copy link
Collaborator

Good to me. Remove FloatImage.

Can you make a small pull request first, just doing this.
Then make another pull request to address the other changes we discussed for Image.

@syncle
Copy link
Contributor Author

syncle commented Jun 6, 2017

The module is implemented and merged. Closing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants