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

Implement Sobel and Scharr operators #392

Merged
merged 15 commits into from Oct 29, 2019

Conversation

@simmplecoder
Copy link
Contributor

simmplecoder commented Sep 24, 2019

Description

This commit adds Sobel and Scharr
operators with support for 0th and 1st
degrees with other degrees planned for
later

References

https://www.researchgate.net/publication/239398674_An_Isotropic_3_3_Image_Gradient_Operator

https://www.researchgate.net/profile/Hanno_Scharr/publication/220955743_Optimal_Filters_for_Extended_Optical_Flow/links/004635151972eda98f000000/Optimal-Filters-for-Extended-Optical-Flow.pdf

Tasklist

  • Implement the operators
  • Add test case(s)
  • Ensure all CI builds pass
  • Review and approve
@simmplecoder simmplecoder requested review from mloskot and stefanseefeld Sep 24, 2019
@simmplecoder simmplecoder force-pushed the simmplecoder:sobel-scharr branch from 6c2f289 to ec75792 Oct 1, 2019
example/Jamfile Outdated Show resolved Hide resolved
include/boost/gil/detail/math.hpp Outdated Show resolved Hide resolved
include/boost/gil/image_processing/numeric.hpp Outdated Show resolved Hide resolved
test/core/image_processing/sobel_scharr.cpp Outdated Show resolved Hide resolved
@stefanseefeld

This comment has been minimized.

Copy link
Member

stefanseefeld commented Oct 1, 2019

yes, yes, I still plan to refactor the code so the numeric extension will be fused into core. Sorry for my slowness... :-(

@mloskot

This comment has been minimized.

Copy link
Member

mloskot commented Oct 1, 2019

@stefanseefeld

I still plan to refactor the code so the numeric extension will be fused into core

Okay, so if @simmplecoder doesn't prefer to do it now, then we can split numeric.hpp into multiple headers during or after the fuse.

@simmplecoder simmplecoder force-pushed the simmplecoder:sobel-scharr branch from fe91129 to f4f50bf Oct 2, 2019
@simmplecoder

This comment was marked as outdated.

Copy link
Contributor Author

simmplecoder commented Oct 8, 2019

@mloskot please remind me to re-enable failing tests before merge. I commented out the compile-fail targets, as those were failing. I wanted to find problems in my own code first. I found one, and am in the process of fixing it.

@mloskot mloskot added the cat/feature label Oct 8, 2019
@mloskot

This comment was marked as outdated.

Copy link
Member

mloskot commented Oct 8, 2019

@simmplecoder

@mloskot please remind me to re-enable failing tests before merge.

AFAIS, this request is no longer valid as you've restored the tests.

@mloskot
mloskot approved these changes Oct 8, 2019
Copy link
Member

mloskot left a comment

@simmplecoder LGTM.

However, please ask and wait for @stefanseefeld 's review and consider his review superior to mine.

@simmplecoder

This comment has been minimized.

Copy link
Contributor Author

simmplecoder commented Oct 22, 2019

@stefanseefeld , would you like me to unify the interface of all kernel generator functions in this PR?

@stefanseefeld

This comment has been minimized.

Copy link
Member

stefanseefeld commented Oct 22, 2019

@simmplecoder , sorry, I don't entirely understand the question. What do you mean by "unify" ?

@simmplecoder

This comment has been minimized.

Copy link
Contributor Author

simmplecoder commented Oct 22, 2019

@stefanseefeld, earlier functions that generate Gaussian and mean kernels return image_view, while these return kernel_2d. Would you like me to make all functions return kernel_2d?

@stefanseefeld

This comment has been minimized.

Copy link
Member

stefanseefeld commented Oct 22, 2019

Ah, yes, I think that would be preferable. Thanks,

@mloskot

This comment has been minimized.

Copy link
Member

mloskot commented Oct 22, 2019

@stefanseefeld

Ah, yes, I think that would be preferable.

I agree that it's better to (create and) use dedicated types for job than trying to (bend to) re-use existing types where they don't necessarily fit, neither conceptually nor physically.
Another aspect is that if we use types like image_view as general-purpose types we are somewhat tightening the coupling between functions operating on kernels and image_view preventing treating those as completely independent entities. This may cause problems in future (e.g. it may be hard to change either).

@simmplecoder simmplecoder force-pushed the simmplecoder:sobel-scharr branch from f4d7ca4 to 3b33f9b Oct 23, 2019
@simmplecoder simmplecoder requested a review from miralshah365 Oct 23, 2019
@simmplecoder

This comment has been minimized.

Copy link
Contributor Author

simmplecoder commented Oct 23, 2019

@miralshah365 , could you please check if I broke your code? I migrated small part of your code that relied on old image_view interface for kernel generators.

@simmplecoder

This comment has been minimized.

Copy link
Contributor Author

simmplecoder commented Oct 23, 2019

The output of Harris and Hessian seem to be differ from before the migration to kernel_2d. I will investigate that further, but I believe the interface should be more or less stable now. Hopefully I will be able to find what's wrong until the deadline.

@mloskot

This comment has been minimized.

Copy link
Member

mloskot commented Oct 23, 2019

@simmplecoder If there is anything to update in @miralshah365 's code and she's offline, then don't hesitate to nudge me. I will try to take care of updating Miral's code.

AFAIS, current failures are in the kernel tests, see https://dev.azure.com/boostorg/gil/_build/results?buildId=701&view=logs&jobId=2517ed61-6924-508d-087f-7c02f775cbba

 "D:\a\1\s\boost-root\libs\gil\_build\test\core\image_processing\test_core_image_processing_simple_kernels.vcxproj" (default target) (26) ->
       (ClCompile target) -> 
         d:\a\1\s\boost-root\libs\gil\test\core\image_processing\simple_kernels.cpp(20): error C2664: 'boost::gil::kernel_2d<float,std::allocator<T>> boost::gil::generate_normalized_mean<float,std::allocator<T>>(size_t)': cannot convert argument 1 from 'boost::gil::image_view<boost::gil::gray32f_loc_t>' to 'size_t' [D:\a\1\s\boost-root\libs\gil\_build\test\core\image_processing\test_core_image_processing_simple_kernels.vcxproj]
         d:\a\1\s\boost-root\libs\gil\test\core\image_processing\simple_kernels.cpp(39): error C2664: 'boost::gil::kernel_2d<float,std::allocator<T>> boost::gil::generate_normalized_mean<float,std::allocator<T>>(size_t)': cannot convert argument 1 from 'boost::gil::image_view<boost::gil::gray32f_loc_t>' to 'size_t' [D:\a\1\s\boost-root\libs\gil\_build\test\core\image_processing\test_core_image_processing_simple_kernels.vcxproj]
         d:\a\1\s\boost-root\libs\gil\test\core\image_processing\simple_kernels.cpp(51): error C2664: 'boost::gil::kernel_2d<float,std::allocator<T>> boost::gil::generate_unnormalized_mean<float,std::allocator<T>>(size_t)': cannot convert argument 1 from 'boost::gil::image_view<boost::gil::gray32f_loc_t>' to 'size_t' [D:\a\1\s\boost-root\libs\gil\_build\test\core\image_processing\test_core_image_processing_simple_kernels.vcxproj]
         d:\a\1\s\boost-root\libs\gil\test\core\image_processing\simple_kernels.cpp(69): error C2664: 'boost::gil::kernel_2d<float,std::allocator<T>> boost::gil::generate_unnormalized_mean<float,std::allocator<T>>(size_t)': cannot convert argument 1 from 'boost::gil::image_view<boost::gil::gray32f_loc_t>' to 'size_t' [D:\a\1\s\boost-root\libs\gil\_build\test\core\image_processing\test_core_image_processing_simple_kernels.vcxproj]
         d:\a\1\s\boost-root\libs\gil\test\core\image_processing\simple_kernels.cpp(81): error C2664: 'boost::gil::kernel_2d<float,std::allocator<T>> boost::gil::generate_gaussian_kernel<float,std::allocator<T>>(size_t,double)': cannot convert argument 1 from 'boost::gil::image_view<boost::gil::gray32f_loc_t>' to 'size_t' [D:\a\1\s\boost-root\libs\gil\_build\test\core\image_processing\test_core_image_processing_simple_kernels.vcxproj]
         d:\a\1\s\boost-root\libs\gil\test\core\image_processing\simple_kernels.cpp(114): error C2664: 'boost::gil::kernel_2d<float,std::allocator<T>> boost::gil::generate_gaussian_kernel<float,std::allocator<T>>(size_t,double)': cannot convert argument 1 from 'boost::gil::image_view<boost::gil::gray32f_loc_t>' to 'size_t' [D:\a\1\s\boost-root\libs\gil\_build\test\core\image_processing\test_core_image_processing_simple_kernels.vcxproj]
@simmplecoder simmplecoder requested a review from mloskot Oct 23, 2019
@simmplecoder

This comment has been minimized.

Copy link
Contributor Author

simmplecoder commented Oct 23, 2019

@mloskot, I made a lot of changes after your approval, could you please have a look at new changes?

@simmplecoder

This comment has been minimized.

Copy link
Contributor Author

simmplecoder commented Oct 23, 2019

FYI, I checked the output of the Harris and Hessian with cmp --silent $old $new || echo "files are different" of commits 5abd20e (needs fixing cmake due to my bad rebase) and 42a3ea0, the files are identical

@simmplecoder

This comment has been minimized.

Copy link
Contributor Author

simmplecoder commented Oct 23, 2019

@stefanseefeld , I believe I did all the changes I wanted. The PR is ready for review.

Copy link
Member

mloskot left a comment

LGTM. Thank you!

@@ -42,7 +44,7 @@ void compute_harris_responses(
" tensor from the same image");
}

auto const window_length = weights.dimensions().x;
auto const window_length = weights.size();

This comment has been minimized.

Copy link
@mloskot

mloskot Oct 23, 2019

Member

Minor naming remark: wouldn't width or x_size fit better as name here than length?

inline void compute_hessian_responses(
GradientView ddxx,
GradientView dxdy,
GradientView ddyy,
Weights weights,
const kernel_2d<T, Allocator>& weights,

This comment has been minimized.

Copy link
@mloskot

mloskot Oct 23, 2019

Member

Minor stylistic remark: since in C++ this reads from right to left as "weights is a reference to const kernel_2d object", as we do it for pointers, this should be the East Const way: kernel_2d<T, Allocator> const& weights

You may want to join the revolution! 😉

image

/// \brief Generate mean kernel
/// \ingroup ImageProcessingMath
///
/// Fills supplied view with normalized mean
/// in which all entries will be equal to
/// \code 1 / (dst.size()) \endcode
inline void generate_normalized_mean(boost::gil::gray32f_view_t dst)
template <typename T = float, typename Allocator = std::allocator<T>>
inline kernel_2d<T, Allocator> generate_normalized_mean(std::size_t side_length)

This comment has been minimized.

Copy link
@mloskot

mloskot Oct 23, 2019

Member

Again, wouldn't width or x_size fit better as name here than length?

}
}
}

/// \brief Generate mean kernel
/// \ingroup ImageProcessingMath
///
/// Fills supplied view with normalized mean

This comment has been minimized.

Copy link
@mloskot

mloskot Oct 23, 2019

Member

Perhaps you could run find and replace for /// Fills supplied view changing to /// Fills kernel. This could be done even after merge as single commit with [ci skip] tag in subject line. No need to CI this kind of change.

case 1:
{
kernel_2d<T, Allocator> result(3, 1, 1);
std::copy(dx_sobel.begin(), dx_sobel.end(), result.begin());

This comment has been minimized.

Copy link
@mloskot

mloskot Oct 23, 2019

Member

Since we have compile-time known std::array<float, 9> dx_sobel, this place seems like a good opportunity to add static_assert verifying dx_sobel.size() and BOOST_ASSERT verifying dx_sobel.size() == result.size().

Such assertions would also serve as code comment for a reader who doesn't have to jump to dx_sobel definition.

case 1:
{
kernel_2d<T, Allocator> result(3, 1, 1);
std::copy(dx_scharr.begin(), dx_scharr.end(), result.begin());

This comment has been minimized.

Copy link
@mloskot

mloskot Oct 23, 2019

Member

Similarly, I'd add some assertions.

case 1:
{
kernel_2d<T, Allocator> result(3, 1, 1);
std::copy(dy_sobel.begin(), dy_sobel.end(), result.begin());

This comment has been minimized.

Copy link
@mloskot

mloskot Oct 23, 2019

Member

And assertions here.

case 1:
{
kernel_2d<T, Allocator> result(3, 1, 1);
std::copy(dy_scharr.begin(), dy_scharr.end(), result.begin());

This comment has been minimized.

Copy link
@mloskot

mloskot Oct 23, 2019

Member

And assertions here too 😄

[](gray32f_pixel_t pixel) -> float {return pixel.at(std::integral_constant<int, 0>{}); }
);

kernel_2d<float> kernel = generate_gaussian_kernel(kernel_size, 1.0);

This comment has been minimized.

Copy link
@mloskot

mloskot Oct 23, 2019

Member

kernel_2d<float> const kernel?

This simple addition immediately clears potential reader's question "can a thing be modified by function call that follows?".

@mloskot mloskot modified the milestones: Boost 1.72, Boost 1.72+ Oct 23, 2019
simmplecoder added 15 commits Sep 24, 2019
This commit adds Sobel and Scharr
operators with support for 0th and 1st
degrees with other degrees planned for
later
Generate Harris entries now uses
signed image view.
The Harris corner detector example
now uses the Scharr filter generator
and convolve_2d to reduce amount
of code needed.
The Hessian example now uses signed
image views and uses newly added kernel
generators to compute gradients
The tests broke due to migration to
signed views in algorithms, but tests
were not adjusted
In Harris and Hessian tests, unsigned
pixel values was used to construct
signed image, which was causing
appveyor to error out.
This commit makes all kernel
generator functions to return kernel_2d
and adapts dependant threshold
function to use the new interface
Migrate Hessian and Harris tests to new
interface for kernel generators
Harris and Hessian examples now use
new interface for kernel generation
simple_kernels are now using kernel_2d
interface
Normalized mean generation had missing
return at the end of the function
This commit reacts to kernel_2d,
convolve_2d being moved to
namespace detail
@simmplecoder simmplecoder force-pushed the simmplecoder:sobel-scharr branch from 42a3ea0 to d2479c4 Oct 29, 2019
@simmplecoder simmplecoder merged commit 62379dd into boostorg:develop Oct 29, 2019
9 checks passed
9 checks passed
boostorg.gil Build #20191029.3 succeeded
Details
boostorg.gil (macos1013_xcode91_cmake) macos1013_xcode91_cmake succeeded
Details
boostorg.gil (ubuntu1604_gcc5_cxx11_cmake) ubuntu1604_gcc5_cxx11_cmake succeeded
Details
boostorg.gil (ubuntu1604_gcc8_cxx14_cmake) ubuntu1604_gcc8_cxx14_cmake succeeded
Details
boostorg.gil (win2012_vs2015_cmake) win2012_vs2015_cmake succeeded
Details
boostorg.gil (win2016_vs2017_cxx14_cmake) win2016_vs2017_cxx14_cmake succeeded
Details
boostorg.gil (win2016_vs2017_cxx17_cmake) win2016_vs2017_cxx17_cmake succeeded
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.