[dart:ui] add an explicit API for loading assets #21153
Conversation
lib/ui/assets.cc
Outdated
| return; | ||
| } | ||
|
|
||
| Dart_Handle byte_buffer = |
There was a problem hiding this comment.
DBC
Overall design probably needs review, but in the meantime consider creating external typed data here to avoid a copy if possible. See: https://github.com/flutter/engine/blob/master/lib/ui/painting/image_encoding.cc#L62
There was a problem hiding this comment.
This is something I looked at initially, but I wasn't sure if it was safe to using the data from a mapping object like that.
There was a problem hiding this comment.
Yeah, if the underlying memory region is mapped as read-only, then making this external typed data might have to wait on dart-lang/sdk#42785 if returning UnmodifiableUint8ListView causes problems.
There was a problem hiding this comment.
Yeah, since I can only get a const void * to the underlying data, and that API accepts void *, per my basic understanding of C++ there is no real way to make that work - since writing to the typed data would presumably cause some sort of awful runtime exception.
There was a problem hiding this comment.
The const qualifier can be casted away. It is safe to wrap the read-only buffer in a Dart type that disallows writes. The issue is whether other bits of dart:ui and the framework can use that type.
There was a problem hiding this comment.
ahh TIL, maybe some sort of lint we have is preventing const_cast? At any rate, I guess that would have to wait for another day anyway
There was a problem hiding this comment.
So I needed to static_cast, then const_cast.
lib/ui/assets.dart
Outdated
| /// into the flutter application. This API is not supported on the Web. | ||
| /// | ||
| /// If the [assetKey] does not correspond to a real asset, returns `null`. | ||
| ByteData? loadAsset(String assetKey) { |
There was a problem hiding this comment.
If we use ImmutableBuffer here, we could have a nice fast path for loading these without relying on Dart.
But right now, that class is only useful when creating images. We'd want to consider extending it in some way so you could get a ByteData from it, or a Uint8List or something.
There was a problem hiding this comment.
Today, we ahve ImmutableBuffer.fromUint8List, but this could allow for creating ImmutableBuffers directly, which would definitely be useful for e.g. Image.asset.
There was a problem hiding this comment.
Sorry I'm not remembering all of the context around ImmutableBuffer. If we had a ReadonlyUint8List from the core Dart SDK, would ImmutableBuffer still be needed?
There was a problem hiding this comment.
Ultimately we need a way to somewhat compatible with the existing API and expose the bytes to users applications. Assets are used for more than just images today, we have APIs to load strings/bytedata/json
There was a problem hiding this comment.
that's not to say we couldn't add a special API for images though, they are one of the most common cases
There was a problem hiding this comment.
Agreed.
ImmutableBuffer was added to avoid having to do multiple copies of image data from a Uint8List to an SkData buffer if it ends up being reused.
If the Dart SDK has an appropriate type that's useful outside of dart:ui, that's probably better. But if not we could expand ImmutableBuffer to have access as ByteData (which, when Dart added some type appropriate for it, could just become a pass through to that type).
| static ImmutableBuffer? fromAsset(String assetKey) { | ||
| final ImmutableBuffer buffer = ImmutableBuffer._(0); |
There was a problem hiding this comment.
I think we'd want to make this a factory so we could have _fromAsset return the length of the asset in bytes, and set the lenght properly.
There was a problem hiding this comment.
Alternatively, we could make the length a getter on a private field that is mutable.
| } | ||
| return buffer; | ||
| } | ||
| static void _fromAsset(String assetKey, ImmutableBuffer buffer, _Callback<void> callback) native 'ImmutableBuffer_fromAsset'; |
There was a problem hiding this comment.
The more typical signature for this is String? _fromAsset(ImmutableBuffer outBuffer, String assetKey, _Callback<void> callback) native 'ImmutableBuffer_fromAsset;. The String is used to return an error message rather than throwing in C++, which has undesirable runtime overhead/stack trace strangeness. You can then use _futureize with it.
It doesn't need to be static, and if the callback calls back with an int it could return the length.
| @@ -30,6 +32,9 @@ ImmutableBuffer::~ImmutableBuffer() {} | |||
| void ImmutableBuffer::RegisterNatives(tonic::DartLibraryNatives* natives) { | |||
| natives->Register({{"ImmutableBuffer_init", ImmutableBuffer::init, 3, true}, | |||
| FOR_EACH_BINDING(DART_REGISTER_NATIVE)}); | |||
| natives->Register({{"ImmutableBuffer_fromAsset", ImmutableBuffer::fromAsset, 3, true}, | |||
There was a problem hiding this comment.
If we do it this way, we don't have to pass the ImmutableBuffer itself as an argument. We also don't want it to be static.
| final ImmutableBuffer buffer = ImmutableBuffer._(0); | ||
| bool success = false; | ||
| _fromAsset(assetKey, buffer, (void result) { | ||
| success = true; |
There was a problem hiding this comment.
This is racy - see below. We want to use _futurize for this, or make our own completer.
There was a problem hiding this comment.
is this actually racy? I'm not really familiar with the native interop but it appears to be fully synchronous to me
There was a problem hiding this comment.
If someone sets the return value in the C++ side before calling the completer here I believe it will have unexpected results.
| std::unique_ptr<fml::Mapping> data = asset_manager->GetAsMapping(asset_name); | ||
|
|
||
| if (data == nullptr) { | ||
| return; |
There was a problem hiding this comment.
Return a Dart String here with something like "Asset not found".
There was a problem hiding this comment.
The contract for asset loading is already that we return null if it isn't found, and the framework can turn this into an exception
| Dart_PropagateError(result); | ||
| return; |
There was a problem hiding this comment.
I think we try to avoid this and instead use a String error.
| SkData::ReleaseProc proc = [](const void* ptr, void* context) { | ||
| // TODO: what do | ||
| }; | ||
|
|
||
| auto sk_data = SkData::MakeWithProc(bytes, size, proc, peer); |
There was a problem hiding this comment.
We might want to just store the mapping object in our heap which should keep it alive, and then reset it in the release proc. I think that matches the semantics of the fml::Mapping, but @jason-simmons or @chinmaygarde would know for sure.
There was a problem hiding this comment.
The peer will be passed to the ReleaseProc as the context parameter. The ReleaseProc can cast it back to an fml::Mapping* and delete it.
There was a problem hiding this comment.
Will that be enough to keep the Mapping pointer alive?
There was a problem hiding this comment.
The mapping pointer should remain valid as long as the fml::Mapping and the AssetManager that created it have not been deleted.
|
I don't have time to work on this right now |
Description
create an API which returns an asset ByteData synchronously given the asset key (usually a file path). Improves asset loading time by 5-10x.
This still requires the data to be copied from the mapping, since the Dart VM does not accept read-only extenal typed data. Its unclear whether the ByteData return type is idea either, perhaps it should be a Uint8List or similar.
Example loading times (microseconds)
Built in release mode using the flutter gallery on a pixel4
channel-based
loadAsset based
With unsafe use of Dart_NewExternalTypedDataWithFinalizer
EDIT: diff for unsafe use of the Dart_* API (yes i did not really implement the finalizer):