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

Make opencv dependency optional #23

Open
artyom opened this issue Dec 21, 2015 · 7 comments
Open

Make opencv dependency optional #23

artyom opened this issue Dec 21, 2015 · 7 comments
Assignees

Comments

@artyom
Copy link

artyom commented Dec 21, 2015

Having a native Go smartcrop implementation can be more desired in some cases; looking at the code it shoud be quite trivial to move opencv dependency to a separate (sub)package, this way main smartcrop package can be made pure-Go and if one needs improved face detection, this can be done importing separate Analyzer from an opencv-dependent package.

@muesli
Copy link
Owner

muesli commented Dec 22, 2015

This could be made optional at run-time, but I'm not sure I want to make it optional at compile-time as this would require a much more complex build system.

@artyom
Copy link
Author

artyom commented Dec 22, 2015

Right now it's problematic to build smartcrop without having opencv installed, this lead to sub-optimal solutions like fork-and-customize. IMO having a pure-Go library can be beneficial in many aspects, taking into account Go strong sides — native concurrency, fast/easy build system, native cross-compiling.

This can easily be improved by splitting package into main (pure Go) packege + subpackage depending on opencv providing additional Analyzer with face detection.

Consider the following package layout:

github.com/muesli/smartcrop

type Analyzer
func NewAnalyzer() Analyzer
func NewAnalyzerWithCropSettings(cropSettings CropSettings) Analyzer
type Crop
func SmartCrop(img image.Image, width, height int) (Crop, error)
type CropSettings
type Score

github.com/muesli/smartcrop/opencv

func NewAnalyzer() smartcrop.Analyzer
func NewAnalyzerWithCropSettings(cropSettings CropSettings) smartcrop.Analyzer
type CropSettings

Having all opencv-dependent code moved into separate subpackage github.com/muesli/smartcrop/opencv it would be possible to use go get github.com/muesli/smartcrop without messing with opencv setup if one does not need face detection. It would only be required if one would like to use opencv-dependent code which can be installed like go get github.com/muesli/smartcrop/opencv.


Another option to remove mandatory opencv dependency without any API changes can be introducing a build-flag based condition: i.e. file with +build opencv can hold imlementation of faceDetect() function whereas file with code having +build !opencv tag can have fallback to current skinDetect() code.

Of course it may be desirable to keep current behavior (build with opencv support) for the sake of existing user-base and compatibility, so the build flag logic can be inverted by using +build noopencv flag.

If you like this idea, I can come up with CL in the upcoming days.

@muesli
Copy link
Owner

muesli commented Dec 22, 2015

Personally I like the idea of a subpackage better - feels a bit more go-ish than build-flags. If you're willing to work on that, I'd certainly accept that PR. We should probably also do a new release then, which can break backwards compatibility. We're not 1.0 yet after all :-)

@artyom
Copy link
Author

artyom commented Dec 22, 2015

Ok then, I'll try to come up with proper PR in the upcoming days.

@muesli
Copy link
Owner

muesli commented Aug 6, 2016

Curious, have you ever started hacking on that?

@muesli
Copy link
Owner

muesli commented Aug 8, 2017

I've merged most of your changes upstream tonight. Thanks for the effort, it's really appreciated.

I tried to contact you several times in the past year, but I guess you lost the interest and/or need for smartcrop?

@muesli muesli self-assigned this Aug 8, 2017
@muesli
Copy link
Owner

muesli commented Apr 18, 2018

Also see #35.

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

No branches or pull requests

2 participants