-
Notifications
You must be signed in to change notification settings - Fork 13
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
[WIP] FFI Implementation #7
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not quite finished reviewing, should be able to return this evening or tomorrow morning. :)
Mainly some context questions below.
This is still a draft, so still plenty of TODOs and documentation to fill in.
packages/mediapipe-core/README.md
Outdated
[developing packages and plugins](https://flutter.dev/developing-packages). | ||
--> | ||
|
||
TODO: Put a short description of the package here that helps potential users |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a note to self to come back and review this when it;s filled out. :)
'You must supply either `modelAssetBuffer` or `modelAssetPath`', | ||
), | ||
assert( | ||
!(modelAssetBuffer != null && modelAssetPath != null), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And this ensures both are not provided.
Might be worth unpacking both into a single assertion with a function body so you could use control flow for better readability and error messaging.
} | ||
|
||
class ClassifierOptions extends Equatable { | ||
const ClassifierOptions({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are there requirements that any of these values are set? What happens if they all are null?
packages/mediapipe-core/pubspec.yaml
Outdated
description: A starting point for Dart libraries or applications. | ||
version: 1.0.0 | ||
|
||
repository: https://github.com/google/flutter-mediapipe |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there an issue tracker? In flutter/packages we usually include an issue_tracker
|
||
analyzer: | ||
exclude: | ||
- "lib/src/mediapipe_common_bindings.dart" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why for do we exclude this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's generated code, so occasionally it will fall behind our latest best practices in flutter_lint
and cause IDE analysis server noise.
|
||
analyzer: | ||
exclude: | ||
- "lib/src/mediapipe_text_bindings.dart" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same "its generated code" answer :)
// AUTO GENERATED FILE, DO NOT EDIT. | ||
// | ||
// Generated by `package:ffigen`. | ||
// ignore_for_file: type=lint |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this necessary if it is excluded in the analysis options?
// Generated by `package:ffigen`. | ||
// ignore_for_file: type=lint | ||
import 'dart:ffi' as ffi; | ||
import 'package:mediapipe_core/src/mediapipe_common_bindings.dart' as imp1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is imp1 short for?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe it's "implementation one", but this is generated code.
|
||
typedef TextClassifierResult = imp1.ClassificationResult; | ||
|
||
const int true1 = 1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What are these super fun values? Maybe I am missing context
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the context is that C is bad at booleans 😆
e2d9cbd
to
3046f3e
Compare
Summary
Adds a MediaPipe implementation that talks directly to C++ via FFI. (Technically, indirectly and through a C intermediary, but still fairly directly in comparison to method channels.)
C bridge code uses Dart's native FFI functionality and
ffigen
to automatically generate the required bindings from C headers pulled from the MediaPipe repository.TODOs
mediapipe_core
package (Adds mediapipe_core package #11)mediapipe_text
package (Adds mediapipe_text package #12)native-assets
to vendor MediaPipe SDK #9)mediapipe_vision
package (Adds mediapipe_vision package #13)TESTING
To run this PR locally, complete the following steps (on macOS):
google/mediapipe
with the latest internal changes pulled down, run this build command:libtext_classifier.dylib
file (found in thebazel-out
directory) to this repository at thepackages/mediapipe-task-text/examples/assets
directory.Download
bert_classifier.tflite
and place it in the same directory.Run the example app and click the floating action button to classify text.