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.

Sign up for GitHub

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

Jump to bottom

Add CoreML support for Mac #157

Merged
merged 12 commits into from Mar 8, 2023
Merged

Add CoreML support for Mac #157

merged 12 commits into from Mar 8, 2023

Conversation

royshil
Copy link
Collaborator

@royshil royshil commented Mar 8, 2023

To boost inference time and save on CPU cycles on Mac machines

@royshil royshil self-assigned this Mar 8, 2023
@umireon
Copy link
Member

umireon commented Mar 8, 2023

@royshil Can you try #160 which is a PR against this branch?

@umireon
Fix #157 (#160)
dcb1c47 
* Add coreml_proto

* Update BuildMyOnnxruntime.cmake
@umireon
Copy link
Member

umireon commented Mar 8, 2023

It would be great if the version number in CMakeLists.txt and buildspec.json is changed to the correct number before the new release

umireon and others added 2 commits March 8, 2023 00:16
@umireon @royshil
Enable -Werror and /WX (#158)
7f9d508 
* Enable Werror and WX

* Fix errors

* Fix bug
@royshil
cmake warn in opencv
ea2fac2
@royshil
Copy link
Collaborator Author

royshil commented Mar 8, 2023

It would be great if the version number in CMakeLists.txt and buildspec.json is changed to the correct number before the new release

so v0.5.1 or 0.5.2?

@umireon
Copy link
Member

umireon commented Mar 8, 2023

I think v0.5.2

@umireon
Copy link
Member

umireon commented Mar 8, 2023

Builds seem to be fixed 🎉

@royshil
Copy link
Collaborator Author

royshil commented Mar 8, 2023

@umireon yep but it's not working 🤣 .. i'm fixing - it should only be a minute longer.

umireon
umireon reviewed Mar 8, 2023
View reviewed changes
src/background-filter.cpp Outdated
obs_property_list_add_string(p_use_gpu, obs_module_text("GPU - DirectML"), USEGPU_DML);
#endif
#if APPLE
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
#if APPLE
#ifdef __APPLE__

@umireon
Copy link
Member

umireon commented Mar 8, 2023

With CoreML:
スクリーンショット 2023-03-08 14 43 22

With CPU:
スクリーンショット 2023-03-08 14 43 58

Using CoreML results in the FPS deterioration 🤔

@royshil
Copy link
Collaborator Author

royshil commented Mar 8, 2023

With CoreML: スクリーンショット 2023-03-08 14 43 22

With CPU: スクリーンショット 2023-03-08 14 43 58

Using CoreML results in the FPS deterioration 🤔

yes i noticed that too.. and it's not gaining anything in CPU reduction or increasing GPU...

@umireon
Copy link
Member

umireon commented Mar 8, 2023

I suppose almost all the operation of OBS is done with GPU.
GPU is not free but CPU is free so it is better to use CPU for additional computing on Mac.

@umireon
Copy link
Member

umireon commented Mar 8, 2023

CoreML uses CPU when the operator is not supported by CoreML.
Once I investigated this and found that half of the operators In MediaPipe were executed in CPU.

@royshil
Copy link
Collaborator Author

royshil commented Mar 8, 2023

@umireon coreML also does nothing on Intel Macs. Only supported on M1/M2. Which is fine.
we can land this as is, and let the people choose...

@royshil royshil requested a review from umireon March 8, 2023 06:00
umireon
umireon approved these changes Mar 8, 2023
View reviewed changes
Copy link
Member

@umireon umireon left a comment

Choose a reason for hiding this comment

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

LGTM!

@royshil royshil requested a review from umireon March 8, 2023 12:37
@royshil
Copy link
Collaborator Author

royshil commented Mar 8, 2023

@umireon can you plz approve? thanks!

umireon
umireon approved these changes Mar 8, 2023
View reviewed changes
Copy link
Member

@umireon umireon left a comment

Choose a reason for hiding this comment

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

Oh my approval discarded, LGTM!

@royshil royshil merged commit d385309 into main Mar 8, 2023
@royshil royshil deleted the roy.add_coreml_to_mac branch March 8, 2023 21:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@umireon umireon umireon approved these changes

Assignees

@royshil royshil

Labels
Seeking Testers
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@royshil @umireon