muesli / smartcrop

smartcrop finds good image crops for arbitrary crop sizes
MIT License
1.82k stars 114 forks source link

Make opencv dependency optional #23

Open artyom opened 8 years ago

artyom commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

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

muesli commented 8 years ago

Curious, have you ever started hacking on that?

muesli commented 7 years ago

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 commented 6 years ago

Also see #35.