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

[opencv] Add opencv extension #1331

Merged
merged 2 commits into from
Nov 4, 2021
Merged

Conversation

frankfliu
Copy link
Contributor

Change-Id: I738397d316ae128dd66f6ff5de8d52bfc56927e2

Description

Brief description of what this PR is about

  • If this change is a backward incompatible change, why must this change be made?
  • Interesting edge cases to note here

@codecov-commenter
Copy link

codecov-commenter commented Oct 30, 2021

Codecov Report

Merging #1331 (1516156) into master (bb5073f) will decrease coverage by 0.39%.
The diff coverage is 61.64%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #1331      +/-   ##
============================================
- Coverage     72.08%   71.69%   -0.40%     
- Complexity     5126     5177      +51     
============================================
  Files           473      480       +7     
  Lines         21970    22432     +462     
  Branches       2351     2410      +59     
============================================
+ Hits          15838    16083     +245     
- Misses         4925     5126     +201     
- Partials       1207     1223      +16     
Impacted Files Coverage Δ
api/src/main/java/ai/djl/modality/cv/Image.java 69.23% <ø> (-4.11%) ⬇️
...i/djl/modality/cv/translator/BigGANTranslator.java 21.42% <ø> (-5.24%) ⬇️
...odality/cv/translator/BigGANTranslatorFactory.java 33.33% <0.00%> (+8.33%) ⬆️
...nslator/InstanceSegmentationTranslatorFactory.java 14.28% <0.00%> (-3.90%) ⬇️
.../modality/cv/translator/YoloTranslatorFactory.java 8.33% <0.00%> (-1.67%) ⬇️
...i/djl/modality/cv/translator/YoloV5Translator.java 5.69% <0.00%> (ø)
...odality/cv/translator/YoloV5TranslatorFactory.java 8.33% <0.00%> (-1.67%) ⬇️
...n/java/ai/djl/modality/nlp/bert/BertTokenizer.java 92.68% <0.00%> (-2.32%) ⬇️
...modality/nlp/embedding/TrainableWordEmbedding.java 45.00% <0.00%> (-3.34%) ⬇️
...pi/src/main/java/ai/djl/ndarray/BytesSupplier.java 54.54% <0.00%> (-12.13%) ⬇️
... and 98 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 11d4013...1516156. Read the comment docs.

Copy link
Contributor

@zachgk zachgk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you also add a README for the opencv extension and add that README to the mkdocs?

Change-Id: I738397d316ae128dd66f6ff5de8d52bfc56927e2
@frankfliu frankfliu merged commit 4a94cff into deepjavalibrary:master Nov 4, 2021
@frankfliu frankfliu deleted the opencv branch November 4, 2021 01:30
@saudet
Copy link

saudet commented Dec 29, 2021

Why haven't you considered the JavaCPP Presets for OpenCV? Could you please explain why you are trying so hard to avoid using anything related to JavaCPP? What's wrong with JavaCPP? I would be more than happy to fix anything that prevents you from using JavaCPP, if only you could express those needs! Please do speak up!

@frankfliu
Copy link
Contributor Author

@saudet

I want to make it clear that I'm not against JavaCPP at all. We do considered about JavaCPP, and we are still interested in using JavaCPP with PyTorch (Due to resource constraint, we paused this initiative. I happy to schedule a meeting with you if you can lead this initiative).

I actually started with JavaCPP Presets for OpenCV. Everything works perfect except the default dependency size is huge (480M). I didn't figure out an easy way to reduce the size (e.g. I failed exclude openblas)

I'm actually not happy with org.openpnp:opencv size (80M) either, but I don't have time to create a build pipeline and only compile needed feature myself.

@saudet
Copy link

saudet commented Dec 30, 2021

If you look only at the binaries of the presets for Linux, Mac, and Windows (x86_64), those are also about 80 MB: https://repo1.maven.org/maven2/org/bytedeco/opencv/4.5.3-1.5.6/
You don't need to depend on all of platforms in your application! Also, the average user is not going to be interested by binaries other than their development platform anyway, so you can have it download just that by default.

As for the dependency on OpenBLAS, it does accelerate OpenCV and PyTorch quite a bit, but neither OpenCV nor PyTorch distribute binaries with BLAS enabled, which I find weird. Are users of DJL also not concerned with performance? If you can say for sure that your users are unconcerned with performance, please let me know, and we'll see what we can do.

Thank you for clarifying your needs!

@saudet
Copy link

saudet commented Jan 7, 2022

BTW, when PyTorch is used, an alternative to OpenCV could be TorchVision. When all symbols are properly stripped from the libraries, the archives are only 1~2 MB per platform: https://pypi.org/project/torchvision/#files

@frankfliu
Copy link
Contributor Author

@saudet
torchvision depends on python, which blocks us from integrating with torchvision, see: #1394

@saudet
Copy link

saudet commented Jan 8, 2022

Right, it does come with a few operations in C++, but looking more closely, it doesn't look too useful. It actually looks mostly like a wrapper around Pillow... In any case, we can enable building the Python libraries as part of the build for the JavaCPP Presets for PyTorch, and that would get us that missing library. BTW, I was thinking about this, and I think DJL could use the JavaCPP Presets for PyTorch as is, but simply rebundle it as necessary, similarly to how you're already doing it for TensorFlow for Java. Although I don't make releases frequently because it needs to some testing, I plan to continue updating the snapshots regularly, so you could pick those binaries and test those independently on your own, before making frequent releases of DJL. How does that sound?

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

Successfully merging this pull request may close these issues.

None yet

4 participants