Native Memory Helpers, take 2 #19
Conversation
* adds mediapipe_core package * adds makefile for all packages * fixes typo in Makefile * Apply suggestions from code review * Update Makefile * Apply suggestions from code review * updated generated code's location and license (for 3P status) * code review responses including: * comments * licensing * resolving testing nits * updated README * setup sharing of base analysis_options * Update packages/mediapipe-core/lib/src/containers.dart Co-authored-by: Kate Lovett <katelovett@google.com> * Update packages/mediapipe-core/lib/src/containers.dart Co-authored-by: Kate Lovett <katelovett@google.com> * add free methods to core structs * code review updates to options * adds publish blocker * Add GitHub actions for CI/CD (#14) * adds CI/CD for mediapipe_core * add newlines * moves file into workflows dir * uncomment flutter doctor * testing PR config to run this now * added master and beta CI scripts * add executable permissions to CI scripts * adds ffiwrapper to ci/cd --------- Co-authored-by: Kate Lovett <katelovett@google.com>
* adds cmd to pull header files from google/mediapipe * polish and missing parts from git surgery * More comments and touch ups * Apply suggestions from code review * moves build command into `tool/` directory and renames folder `build_cmd` -> `builder` * complete build_cmd -> builder rename * Update readme * Added licenses * Adds DownloadModelCommand --------- Co-authored-by: Kate Lovett <katelovett@google.com>
* adds sdks_finder command to builder utility * propagates changes to existing commands * adds manifest files generated by new sdks_finder command * updates in response to code review
|
CI is failing because this refactors code that was in use elsewhere, but I want to make sure the utilities are correct before propagating their changes everywhere. However, those updates are quite trivial. |
| counter++; | ||
| /// Helpers to convert a [Pointer<Char>] into a [Uint8List]. | ||
| extension DartAwareChars on Pointer<Char> { | ||
| /// Creates a Dart view on a pointer to utf8 chars in native memory, cast as |
There was a problem hiding this comment.
I believe the terminology might be wrong here.
The string characters are represented by code points (a 24 bit integer) which is subsequently encoded as bytes.
So maybe you tried to say: a Dart view on the bytes of a UTF-8-encoded string?
I'd say characters are a concept of the string.
The 'chars' in C only map into String characters for ASCII, not for UTF-8.
There was a problem hiding this comment.
This one is actually returning a Uint8List, not a String. How does this look?
Creates a Dart view on native memory, cast as unsigned, 8-bit integers.
There was a problem hiding this comment.
That looks good. You might want to explicitly state that the length argument is the length of the encoded string in bytes, not the length of the String. (Probably if you get the length from C as well, it will be the length in bytes, but it can't hurt to be explicit in the Dart documentation here.)
|
|
||
| /// Converts the C memory representation of a string into a Dart [String]. | ||
| /// | ||
| /// If this is called on a `nullptr`, the method returns `null`. |
There was a problem hiding this comment.
It is fishy that you're handling nullptr here. On the call sites you're likely going to have to free the underlying char* if it's not nullptr and not free it if is a nullptr. So I expect your call site to branch on the native resources already. So it might be cleaner to branch on the call sites on nullptr explicitly. Do you have examples of use cases where using this extension method makes sense?)
| /// | ||
| /// See also: | ||
| /// * [toDartString], for a non-list equivalent. | ||
| List<String?>? toDartStrings(int length) { |
There was a problem hiding this comment.
Also here, automatically dealing with nullptrs makes me worried about whether the native memory is freed correctly on the call site of this place. Examples of uses?
| /// Copies a [Float32List] into native memory as a `float*`. | ||
| /// | ||
| /// Returns an [allocator]-allocated pointer to the result. | ||
| Pointer<Float> toNative({Allocator allocator = malloc}) { |
There was a problem hiding this comment.
I'd probably call this copyToNative.
And yes maybe this should be a PR to package:ffi. Let's first use it here and then later upstream some useful abstractions once we have proven them here.
| bool get isNotNullAndIsNotNullPointer => _isNotNull && isNotNullPointer; | ||
|
|
||
| /// Returns true if this is either [null] or is a `nullptr`. | ||
| bool get isNullOrNullPointer => _isNull || isNullPointer; |
There was a problem hiding this comment.
In my experience we only really use nullptr, we don't really have null for Pointer objects in code using dart:ffi. Basically dart:ffi always returns you a Pointer object, never a Pointer?. With what kind of code patterns do you end up with Pointer??
There was a problem hiding this comment.
I'm using this around Pointer<T>? types that are lazily instantiated, so months ago, this arose out of a desire to stop using ! everywhere; but I can revisit that thought.
craiglabenz
force-pushed
the
ffi-wrapper-ffi-utils
branch
from
98edc78
to
cdaccc5
Compare
|
Obsolete. |
Adds helpers for converting between Dart and native memory for the following scenarios:
Float32List<->Pointer<Float>Uint8List<->Pointer<Char>String<->Pointer<Char>List<String><->Pointer<Pointer<Char>>Some of this functionality already existed, and some of it is new to this PR. Where the functionality already existed, the implementations were consolidated on extension methods instead of free-floating functions. Also, methods to convert from native memory to Dart types now return nullable types instead of throwing exceptions on null pointers.
Replaces #18
cc @Piinks