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

add JNI library #54

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

add JNI library #54

wants to merge 3 commits into from

Conversation

Acs176
Copy link

@Acs176 Acs176 commented Feb 7, 2025

Merge Request: JNI Library for Android Integration

Overview

This merge request introduces a JNI library that enables WhisperKit’s CLI to integrate with Android environments.

Key Contributions

1. JNI Library for Android Integration

  • Developed a JNI library that facilitates communication between WhisperKit’s CLI and Android environments.

2. Deployment and Testing Scripts

  • run_on_android.sh:
    A new script simplifies running the CLI on an Android device via the ADB shell.
  • adb-push.sh Updates:
    The script has been updated to also push:
    • A test audio file (for verifying audio input and transcription).
    • A CLI binary built for the JNI library.
  • Build Script Enhancements:
    The build script now accepts an additional jni flag, which was added due to the need to embed the SDL3 library into the .so file. This was necessary to resolve issues with loading shared libraries in the JNI environment.

Alignment with Project Goals

  • JNI Support for Android Environments:
    This merge request provides the essential JNI infrastructure needed for WhisperKit’s Android integration.
  • Ease of Use:
    The new scripts and build enhancements streamline the workflow for testing and deploying WhisperKit’s JNI library on Android devices.

@keith4ever keith4ever requested review from bpkeene and a2they February 7, 2025 19:08
@bpkeene
Copy link
Contributor

bpkeene commented Feb 7, 2025

Awesome work, thank you for your PR! Review is in progress, at a glance it looks very nice!

@bpkeene
Copy link
Contributor

bpkeene commented Feb 7, 2025

As a quick task, can you rebase this change to top-of-tree on main branch and then run bash test/test_build_all.sh from the root project directory? This should be invoked from within the Docker environment.

I believe some of the changes here for android may be incompatible with the linux target.

@@ -7,21 +7,12 @@
#include <thread>
#include <condition_variable>
#include <mutex>

#ifdef ANDROID_JNI
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's revert this, headers from android NDK should not be used for linux target

@Acs176 Acs176 force-pushed the jni branch 2 times, most recently from f00ea54 to eeb3d0a Compare February 17, 2025 17:14
@Acs176 Acs176 requested a review from bpkeene February 17, 2025 17:15
@Acs176
Copy link
Author

Acs176 commented Feb 17, 2025

Hi @bpkeene!
First of all, sorry for the delay. I did run into many issues with the rebase and build process so I ended up purging a lot of the code I initially had. You can see now the PR is much cleaner and everything should work as intended.

@Acs176
Copy link
Author

Acs176 commented Feb 17, 2025

I have made sure the compatibility with Linux is kept as it was before.

Regarding the android app, I will add that code later this week, today I managed to run the model with the QNN delegate for the first time, so great news! There are still some issues with the android app but I'm hoping we will manage to solve them with some collaboration :)

Best regards!

config.concurrentWorkerCount = concurrentWorkers;
config.audioPath = audioPath;

runner = new WhisperKitRunner(config);
Copy link
Contributor

Choose a reason for hiding this comment

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

could we use a unique_ptr? Looking to keep C-style memory management in the C API only (WhisperKit.h, WhisperKit.cpp). Otherwise removing any c-style allocations elsewhere, including internal files

@Acs176 Acs176 requested a review from bpkeene February 18, 2025 16:25
@Acs176
Copy link
Author

Acs176 commented Feb 18, 2025

Hi @bpkeene!
First of all, thank you for taking the time to review my code.

I have added a commit having the WhisperKitConfig as a reference. This allows changes in the config after the runner object was created to take effect, which wasn't possible before.
I have also included the all_msgs bugfix that I mentioned in an issue earlier today. Let me know what you think :)

I pushed the app into one of the branches in my fork. I didn't do a PR yet, maybe it's better to merge the changes separately since it's a lot of code?

Thanks for your input,
Best regards!

@bpkeene
Copy link
Contributor

bpkeene commented Feb 18, 2025

Awesome work, thank you for making that change! I'll pull this locally and run through some quick tests, otherwise it LGTM! Really appreciate your contribution :) Excellent work!

I pushed the app into one of the branches in my fork. I didn't do a PR yet, maybe it's better to merge the changes separately since it's a lot of code?

Yes, happy to review this separately

@Acs176 Acs176 changed the title add JNI library and sample android app add JNI library Feb 19, 2025
@Acs176
Copy link
Author

Acs176 commented Feb 19, 2025

I updated this PR to mention only the JNI lib and opened a separate PR for the android app

@bpkeene
Copy link
Contributor

bpkeene commented Feb 26, 2025

Awesome work- thank you for your contribution and patience :)

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.

2 participants