-
Notifications
You must be signed in to change notification settings - Fork 26
[WIP] Using c++ templates #18
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
base: master
Are you sure you want to change the base?
Conversation
|
Nice to see such a quick start! Please see my comments in #17. Please remember to write tests to demonstrate that these new methods indeed work with Templates. |
|
I have added the test cases but couldn't test it locally on my laptop. How about we get travis up and running ? |
|
Sure, we can get Travis CI running, but you and I should definitely be able to test everything locally first. I am looking into the error you reported with |
|
I have now built the latest In the meantime, I wanted to note that I had already implemented templates for |
maxruby
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This statement won't be reached. There is conditional statement any longer in this function.
src/OpenCV_Mat.jl
Outdated
| return (val) | ||
|
|
||
| # RGB images | ||
| vec = @cxx ptr_val3(img, row, col); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This statement will not be reached because there is no conditional statement anymore in the function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops ! I never considered that. Made the changes.
src/OpenCV_Mat.jl
Outdated
| vec = @cxx ptr_val3(img, row, col); | ||
| println([int(at(vec, 0)), int(at(vec, 1)), int(at(vec, 2))]) | ||
| return (vec) | ||
| image = Images.separate(img); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should generally avoid more dependencies than absolutely necessary (e.g., Images), especially when there are already methods in OpenCV.jl that can achieve the same goal.
The following lines:
image = Images.separate(img);
cd = Images.colordim(image)
can be a single line as follows:
channels(img)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Points worth keeping in mind:
separateandcolordimare deprecated- ImageCore.jl is a much smaller dependency
- In modern Images.jl (which is built on top of ImageCore), the color dimension always has to be placed first
- There's a
permuteddimsviewto lazily-permute the dimensions when that's not true. - The various new views (
normedview,rawview,channelview,colorview) willreinterpreta block of memory when it's feasible to do so, and otherwise return a wrapper that gives the same functionality. So if you use those functions, getting out anArraymeans that you can safely pass it directly to C++ without making copies. - If you don't get out an
Array, but get some kind ofAbstractArray, beware that you probably can't pass such an array as aPtrto C++ routines and expect it to work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@timholy
Thanks for sharing the info here. Is this documented in the Images project Wiki? It would be good to keep such relevant information in a Wiki. Perhaps we can have a new section either on OpenCV.jl or Images.jl that documents how to interact between these and other relevant graphics/image-processing Julia packages.
Or should we document such info in the JuliaImages Wiki?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's pretty thoroughly documented at http://juliaimages.github.io/latest/. That said, if you notice stuff that's missing, by all means file an issue.
I'm not against the idea of a Wiki, but that's not where I've put my efforts thus far.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The docs at JuliaImages look good. I have no time myself for the Wiki, but thought that it might be something kvmanohar22 could help with if useful for the project...
test/jl/tests.jl
Outdated
| println("test 6: Conversion of images from Images.jl to OpenCV Mat") | ||
| filename = joinpath(Pkg.dir("OpenCV"), "./test/images/mandrill.jpg") | ||
| image = imread(filename) | ||
| dst = Mat(int(rows(image)), int(cols(image)), CV_8UC1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believeint is deprecated in 0.6.0, so we need to use Int(round(rows(image))) instead. Please check this and confirm that test5 and test6 pass on your system.
| dst = Mat(Int(round(rows(image))), Int(round(cols(image))), CV_8UC1) | ||
| dst = convertToMat(image) | ||
| end | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have you run test5 and test6 on your system?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, I was trying to get over this error. I'll test them now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Test 5 :
Video is streamed but I get this error in the terminal
test 5: Basic video display: set video properties
Frames per second: 10.0
:1:1: error: no matching constructor for initialization of 'cv::Mat'
^
/usr/local/include/opencv2/core/mat.inl.hpp:339:6: note: candidate constructor not viable: no known conversion from 'long *' to 'int' for 2nd argument; dereference the argument with *
Mat::Mat(int _rows, int _cols, int _type)
^
/usr/local/include/opencv2/core/mat.inl.hpp:373:6: note: candidate constructor not viable: no known conversion from 'long *' to 'const int *' for 2nd argument
Mat::Mat(int _dims, const int* _sz, int _type)
^
/usr/local/include/opencv2/core/mat.inl.hpp:364:6: note: candidate constructor not viable: no known conversion from 'long' to 'Size' (aka 'Size_<int>') for 1st argument
Mat::Mat(Size _sz, int _type, const Scalar& _s)
^
/usr/local/include/opencv2/core/mat.inl.hpp:398:6: note: candidate constructor not viable: no known conversion from 'long' to 'const std::vector<int>' for 1st argument
Mat::Mat(const std::vector<int>& _sz, int _type, const Scalar& _s)
^
/usr/local/include/opencv2/core/mat.hpp:894:5: note: candidate constructor not viable: no known conversion from 'long' to 'const std::vector<int>' for 1st argument
Mat(const std::vector<int>& sizes, int type, void* data, const size_t* steps=0);
^
/usr/local/include/opencv2/core/mat.hpp:906:5: note: candidate constructor not viable: no known conversion from 'long' to 'const cv::Mat' for 1st argument
Mat(const Mat& m, const Range& rowRange, const Range& colRange=Range::all());
^
/usr/local/include/opencv2/core/mat.inl.hpp:459:6: note: candidate constructor not viable: no known conversion from 'long' to 'Size' (aka 'Size_<int>') for 1st argument
Mat::Mat(Size _sz, int _type, void* _data, size_t _step)
^
/usr/local/include/opencv2/core/mat.inl.hpp:492:6: note: candidate constructor template not viable: requires at most 2 arguments, but 3 were provided
Mat::Mat(const std::vector<_Tp>& vec, bool copyData)
^
/usr/local/include/opencv2/core/mat.inl.hpp:509:6: note: candidate constructor template not viable: requires at most 2 arguments, but 3 were provided
Mat::Mat(const Vec<_Tp, n>& vec, bool copyData)
^
/usr/local/include/opencv2/core/mat.inl.hpp:525:6: note: candidate constructor template not viable: requires at most 2 arguments, but 3 were provided
Mat::Mat(const Matx<_Tp,m,n>& M, bool copyData)
^
/usr/local/include/opencv2/core/mat.inl.hpp:541:6: note: candidate constructor template not viable: requires at most 2 arguments, but 3 were provided
Mat::Mat(const Point_<_Tp>& pt, bool copyData)
^
/usr/local/include/opencv2/core/mat.inl.hpp:560:6: note: candidate constructor template not viable: requires at most 2 arguments, but 3 were provided
Mat::Mat(const Point3_<_Tp>& pt, bool copyData)
^
/usr/local/include/opencv2/core/mat.inl.hpp:580:6: note: candidate constructor template not viable: requires single argument 'commaInitializer', but 3 arguments were provided
Mat::Mat(const MatCommaInitializer_<_Tp>& commaInitializer)
^
/usr/local/include/opencv2/core/mat.inl.hpp:390:6: note: candidate constructor not viable: requires 2 arguments, but 3 were provided
Mat::Mat(const std::vector<int>& _sz, int _type)
^
/usr/local/include/opencv2/core/mat.hpp:936:5: note: candidate constructor not viable: requires 2 arguments, but 3 were provided
Mat(const Mat& m, const std::vector<Range>& ranges);
^
/usr/local/include/opencv2/core/mat.hpp:926:5: note: candidate constructor not viable: requires 2 arguments, but 3 were provided
Mat(const Mat& m, const Range* ranges);
^
/usr/local/include/opencv2/core/mat.hpp:916:5: note: candidate constructor not viable: requires 2 arguments, but 3 were provided
Mat(const Mat& m, const Rect& roi);
^
/usr/local/include/opencv2/core/mat.inl.hpp:356:6: note: candidate constructor not viable: requires 2 arguments, but 3 were provided
Mat::Mat(Size _sz, int _type)
^
/usr/local/include/opencv2/core/mat.inl.hpp:381:6: note: candidate constructor not viable: requires 4 arguments, but 3 were provided
Mat::Mat(int _dims, const int* _sz, int _type, const Scalar& _s)
^
/usr/local/include/opencv2/core/mat.inl.hpp:347:6: note: candidate constructor not viable: requires 4 arguments, but 3 were provided
Mat::Mat(int _rows, int _cols, int _type, const Scalar& _s)
^
/usr/local/include/opencv2/core/mat.inl.hpp:407:6: note: candidate constructor not viable: requires single argument 'm', but 3 arguments were provided
Mat::Mat(const Mat& m)
^
/usr/local/include/opencv2/core/cuda.inl.hpp:621:6: note: candidate constructor not viable: requires single argument 'm', but 3 arguments were provided
Mat::Mat(const cuda::GpuMat& m)
^
/usr/local/include/opencv2/core/mat.inl.hpp:1179:6: note: candidate constructor not viable: requires single argument 'm', but 3 arguments were provided
Mat::Mat(Mat&& m)
^
/usr/local/include/opencv2/core/mat.hpp:880:5: note: candidate constructor not viable: requires at least 4 arguments, but 3 were provided
Mat(int ndims, const int* sizes, int type, void* data, const size_t* steps=0);
^
/usr/local/include/opencv2/core/mat.inl.hpp:426:6: note: candidate constructor not viable: requires at least 4 arguments, but 3 were provided
Mat::Mat(int _rows, int _cols, int _type, void* _data, size_t _step)
^
/usr/local/include/opencv2/core/mat.inl.hpp:333:6: note: candidate constructor not viable: requires 0 arguments, but 3 were provided
Mat::Mat()
and I was unable to test Test 6, there seems to be a bug with Images.jl which is used in convertToMat function for test 6. I have filed an issue here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know what is going on. You need to provide at least one input with a T declaration from which the C++ compiler can use to infer the type. For example:
If you declare the functions like this:
template <typename T1>
inline T1 ptr_val(cv::Mat &img, int row, int col)
{
return(static_cast<T1>(img.at<T1>(row, col)));
}
template <typename T2>
inline void ptr_val3(cv::Mat &img, int row, int col, std::vector<T2> vec)
{
for(int i = 0; i < 3; ++i)
{
img.at<cv::Vec<T2, 3>>(row, col)[i] = static_cast<T2>(vec[i]);
}
}
// This works
julia> img = Mat(500,500, CV_8UC3)
(class cv::Mat)
julia> @cxx ptr_val(img, 10, 10, 255)
julia> @cxx ptr_val(img, 10, 10, 233.4)
julia> @cxx ptr_val(img, 10, 10, 233.4344343)
// This also works
julia> vec1 = tostdvec([1,2])
(class std::__1::vector<long, class std::__1::allocator<long> >)
julia> @cxx ptr_val3(img, 10, 10, vec1)
julia> vec1 = tostdvec([1,2])Without these changes, I get as expected errors like this:
@cxx ptr_val(img, 10, 10)
:1:1: error: no matching function for call to 'ptr_val'
^
__cxxjl_87.cpp:3:11: note: candidate template ignored: couldn't infer template argument 'T1'
inline T1 ptr_val(cv::Mat &img, int row, int col)
^
__cxxjl_87.cpp:9:13: note: candidate function template not viable: requires 4 arguments, but 3 were provided
inline void ptr_val(cv::Mat &img, int row, int col, T2 val)However, this is surely not the goal - just an illustration of type inference.
The goal here is that the compiler infers the T type based on the Mat type, in other words, we need to use cv::Mat<_Tp> as illustrated in the OpenCV docs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, basically we need to modify src/OpenCV_core.jl to include the wrappers for templates ? Are you working on this ? If not I would like to dive in!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By all means dive in and set up the wrappers!
I did have a quick look yesterday evening at implementing cv::Mat<_Tp> but got errors probably because it was not as straightforward as it first seems.
I do not actually have much time right now to work on this project (definitely not during the week) - but I would like to make sure OpenCV.jl continues to work properly and aim always to enhance it in areas where Julia offers unique advantages.
| @@ -1,4 +1,5 @@ | |||
| using OpenCV | |||
| using Base.Test | |||
| using Images | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be preferable that you first test the new generic Mat constructors with OpenCV.jl native methods. Once completed you can submit a separate PR with support for Images and any other relevant packages.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Currently I'm working on adding the wrappers for template constructors in src/OpenCV_core.jl for Mat. I'll open a new PR for this. Once this is done and merged only then will I be able to continue with the test 6 using support of other packages or native methods. Images dependency was used because convertToMat method uses Images.
|
I came up with a different approach to deal with this error :
something like this in # Grayscale and binary images
if (cvtypeval(img) == CV_8UC1)
@cxx ptr_val<unsigned char>(img, row, col, value);
elseif (cvtypeval(img) == CV_16SC1)
@cxx ptr_val<short>(img, row, col, value);
elseif (cvtypeval(img) == CV_32SC1)
@cxx ptr_val<unsigned short>(img, row, col, value);
elseif (cvtypeval(img) == CV_32FC1)
@cxx ptr_val<float>(img, row, col, value);
elseif (cvtypeval(img) == CV_64FC1)
@cxx ptr_val<double>(img, row, col, value);but this seems not to be supported yet in julia> cxx""" #include <iostream>
template <typename T>
inline void _sum(int const& a, int const& b){
T val = 0;
val += static_cast<T>(a);
val += static_cast<T>(b);
std::cout<<val<<std::endl;
}
"""
true
julia> @cxx _sum<int>(2,3);
ERROR: Unrecognized CPP Expression _sum < int > (2, 3) (comparison)
Simultaneously I'm working on implementing cv::Mat_<_Tp>. I think both these implementations accomplish the same. Any suggestions how to take this forward ? |
|
Did you try the following? cxx""" #include <iostream>
template <typename T>
inline void _sum(T const& a, T const& b){
T val = static_cast<T>(0);
val += static_cast<T>(a);
val += static_cast<T>(b);
std::cout<<val<<std::endl;
}
"""and this one might be tricky because I am not sure how template <typename T>
inline void ptr_val(cv::Mat &img, int row, int col, T val)
{
T value = static_cast<T>(val);
img.at<T>(row, col) = value;
} |
Yep, that works. The only problem is when we don't specify the type in the arguements list. In the case of extending functionality of julia> @cxx _sum<int>(2,3);
we can address the present issue with the above type of implementation without the need of implementing |
Good to know. But the question is why do we need to specify in advance that you are passing an |
|
Yes, with the implementation of |
|
I'm coming across an unusual error : function convertToMat(image)
img = permuteddimsview(channelview(image), (2,3,1))
cd = Base.size(channelview(image))[1] > 3 ? 1 : 3
_rows = Base.size(image, 1)
_cols = Base.size(image, 2)
if (typeof(img[1,1,1].i) == UInt8)
if (cd == 3)
mat = Mat(_rows, _cols, CV_8UC3);
end
end
endI get the following error which is something unexpected. :1:1: error: no matching constructor for initialization of 'cv::Mat'
^
/usr/local/include/opencv2/core/mat.inl.hpp:339:6: note: candidate constructor not viable: no known conversion from 'long *' to 'int' for 2nd argument; dereference the argument with *
Mat::Mat(int _rows, int _cols, int _type)
^
I could boil down the error to this, |
|
I don't have access now to my julia/openCV.jl setup, but the error clearly says that |
I had tried that earlier, but there was no luck with that. |
|
OK. Let me try to understand better the problem. Can you check what |
test 6: Conversion of images from Images.jl to OpenCV Mat
ERROR: LoadError: LoadError: MethodError: no method matching Mat(::Int32, ::Int32, ::Int64)
Closest candidates are:
Mat(::Any, !Matched::Int64, ::Any) at /home/kv/.julia/v0.6/OpenCV/./src/OpenCV_core.jl:368
Mat(::Any, ::Any) at /home/kv/.julia/v0.6/OpenCV/./src/OpenCV_core.jl:385
Mat(!Matched::Int64, !Matched::Ptr{Int64}, ::Int64) at /home/kv/.julia/v0.6/OpenCV/./src/OpenCV_core.jl:375
...
Stacktrace:
[1] convertToMat(::Array{ColorTypes.RGB{FixedPointNumbers.Normed{UInt8,8}},2}) at /home/kv/.julia/v0.6/OpenCV/./src/OpenCV_ImagesSupport.jl:17
[2] test6() at /home/kv/.julia/v0.6/OpenCV/./test/jl/tests.jl:146
[3] include_from_node1(::String) at ./loading.jl:539
[4] include(::String) at ./sysimg.jl:14
[5] include_from_node1(::String) at ./loading.jl:539
[6] include(::String) at ./sysimg.jl:14
[7] process_options(::Base.JLOptions) at ./client.jl:305
[8] _start() at ./client.jl:371
while loading /home/kv/.julia/v0.6/OpenCV/./test/jl/tests.jl, in expression starting on line 149
while loading /home/kv/gsoc/OpenCV.jl/test/runtests.jl, in expression starting on line 6
|
|
That's true on a 64-bit CPU; it's |
|
but I don't get any errors while doing |
What happens when you provide |
I get the following error : :1:1: error: no matching constructor for initialization of 'cv::Mat'
^
/usr/local/include/opencv2/core/mat.inl.hpp:339:6: note: candidate constructor not viable: no known conversion from 'long *' to 'int' for 2nd argument; dereference the argument with *
Mat::Mat(int _rows, int _cols, int _type)
^
|
|
See the following posts (old but still relevant) |
| template <typename T2> | ||
| inline void ptr_val(cv::Mat<T2> &img, int row, int col, double val) | ||
| template <typename T> | ||
| inline void ptr_val(cv::Mat &img, int row, int col, double val) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have had a look at the possible implementation of cv::Mat templates as well, but as discussed before the implementation here does not work. When executed in the REPL, I get couldn't infer template argument T:
julia> @cxx ptr_val(img, 10,10, 200.5)
:1:1: error: no matching function for call to 'ptr_val'
__cxxjl_93.cpp:2:13: note: candidate template ignored: couldn't infer template argument 'T'
inline void ptr_val(cv::Mat &img, int row, int col, double val) {I will add some comments on my observations and suggestion for an implementation using a combination of std::vector and _InputArray. OpenCVby design (due to performance reasons) uses non-templated Mat constructors.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that would be great. I'm stuck with the implementation of Mat_<_Tp> constructers.
|
I found a solution for conversion of image arrays from JuliaImages to Mat using templates and have written wrappers to test it. The key is to use |
In Ref #16
Adding functionality of templates. Will be updated with the usage of pointers for faster algorithm.