search

thumbor / opencv-engine

Thumbor engine for the openCV imaging library.
MIT License
26 stars 26 forks source link

Add support for opencv3 and the cv2 interface. #27

Closed christianjgreen closed 6 years ago

christianjgreen commented 8 years ago

This PR is directed towards issue #15

openCV 3.x dropped the deprecated C API interface cv for the currently maintained C++ interface cv2.

A new engine was created using cv2 and numpy to support ongoing openCV development.

philipbjorge commented 8 years ago

What's needed to get this merged into master? Looking to get webp support with opencv as well...

christianjgreen commented 8 years ago

Waiting on the owners to review it. Might have to update travis so its compatible. I use a custom build of opencv 3.0.1 with this engine, not sure if the python-opencv package will work (runs 2.x) @scorphus

philipbjorge commented 8 years ago

Yeah it probably won't... FWIW, this guy has opencv binaries for ubuntu. https://launchpad.net/~orangain/+archive/ubuntu/opencv

christianjgreen commented 8 years ago

@philipbjorge This could potentially work for the travis builds, but thumbor does not have support for python 3.x so I'm not sure how valid these tests would be.

christianjgreen commented 7 years ago

The current issue with getting this to work with travis is the testing environment requirements. The testing suite is using thumbor which requires python 2.x to run. The current packages on travis contain either openCV (C interface) with python 2.x or openCV2/3 (C++ interface) with python 3.x Until we get a package that has openCV2/3 with python 2.x bindings the automated checks will fail. The second option would be to rewrite to testing suite to not use thumbor.... but that almost completely voids the purpose of the testing as this is an engine for thumbor.

philipbjorge commented 7 years ago

@Arthien -- Python3 support was added a year ago to Thumbor? https://github.com/thumbor/libthumbor/releases/tag/1.2.0

Either way, we are no longer leveraging the opencv engine as PIL seems to be the best supported so this isn't a pressing issue for us anymore.

christianjgreen commented 7 years ago

@philipbjorge That is the libthumbor project, it's only used to generated urls for thumbor. In fact I believe the project is being deprecated and brought into future thumbor releases. While PIL is easier to support, if you want proper smart detection it would be best to move off the old C interface as it is not fully deprecated and much less efficient. Hence the reason to upgrade the opencv engine.

philipbjorge commented 7 years ago

Oops thanks for the catch -- Looks like I need to move our production thumbor instances to python2 :D.

lucasgomide commented 7 years ago

@philipbjorge Can you merge it, or there are pendencies yet ?

ghost commented 7 years ago

actually the rotation seems to be wrong.. using the following exif orientation image will show you a weird output landscape_by_exif_rotation

ghost commented 7 years ago

please see my referenced commit to see a version with a working rotation.. it would be great if both can be combined and the pull request could be merged after review and cleanup!

christianjgreen commented 7 years ago

@seffenberg-naspers Nice work the the rotation fix :)

christianjgreen commented 7 years ago

So I was working on getting the CI to work. Works locally for me, but not on travis. Anyone want a shot at the travis file? https://travis-ci.org/Arthien/opencv-engine/builds/192926720

christianjgreen commented 7 years ago

make test with this PR averages 2.72s vs PILs 3.25s

ghost commented 7 years ago

@Arthien I realized that adding self.exif = None introduced another problem depending on the order. Please see: https://github.com/seffenberg-naspers/opencv-engine/commit/bad30d695b58622f3c88620fef85b23ae1c5cdd3

savar commented 7 years ago

@Arthien there is another bug which will happen if somebody tries to use the smart stuff (detectors).. see the convert_to_grayscale parts in thumbor/thumbor#886 .. it would be good to fix this stuff as well ;)

savar commented 7 years ago

besides that @scorphus and @masom can we somehow get this stuff done? The opencv-engine as it is right now doesn't work with the opencv stuff or thumbor anymore in my opinion. So it would be nice to find somehow a way to integrate that stuff?!

savar commented 7 years ago

@Arthien please ignore the first link to 'a2d79c8' .. numpy objects have a different kind of logic when using if not XXX so I had to switch to if XXX is None ..

christianjgreen commented 7 years ago

@ianko

masom commented 7 years ago

Thanks @Arthien for all the work on getting OpenCV to work again!

I'm not too familiar with the OpenCV stuff in Thumbor but I'll be happy to review this PR.

savar commented 7 years ago

@masom just saying, we use the opencv-engine.. we have around (in peak) 2000 req/s against around 55 thumbors (docker container with 1.2 cpus each) .. so it works at least generally ;)

masom commented 7 years ago

Could this PR be rebased into smaller number of commits? The changes all seem reasonable.

savar commented 7 years ago

@Arthien do you have time for that? Or shall I have a look and re-create the pull-request with your changes (or I get write access to your fork ;) )

christianjgreen commented 7 years ago

I'll Rebase it!

savar commented 6 years ago

Hey @Arthien any chance to get this rebased soon , so that this really works with the latest thumbor+opencv out of the box?

Am 16. Dezember 2017 20:31:23 MEZ schrieb Utsav Kumar notifications@github.com:

utsav2307 approved this pull request.

-- You are receiving this because you commented. Reply to this email directly or view it on GitHub: https://github.com/thumbor/opencv-engine/pull/27#pullrequestreview-83982352

-- Diese Nachricht wurde von meinem Android-Gerät mit K-9 Mail gesendet.

christianjgreen commented 6 years ago

@savar Sorry I've been very busy with work. Just rebased into smaller commits yes?

christianjgreen commented 6 years ago

Damn that was butchered...

christianjgreen commented 6 years ago

Closing in favor of pr #32 cleaned up