-
-
Notifications
You must be signed in to change notification settings - Fork 4.3k
Allow Reader to implement AsyncSeek and provide a way for loaders to "ask" for a compatible reader.
#22104
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
Conversation
f515166 to
8c5977d
Compare
… communicate what features they need from the read.
atlv24
left a comment
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.
nice job, looks good to me
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 see the vision, and I think this makes the right UX tradeoff. The alternative is forking out a separate set of AssetLoader traits, which would be a big UX downgrade.
The big "risk" here is that loaders won't request seeking behavior by default, but they will get seeking behavior by default. Then when they are used in a non-seeking context, they will fail at runtime. I expect this to be a common source of bugs in practice.
The big use case here is skipping the intermediate "read into buffer" behavior for the web / wasm providers, when a loader doesnt request seekability. If we start doing that, then loaders that don't request the appropriate feature will fail at load-time. Fortunately, I think this is the kind of thing that will be caught quickly. If/when we land that, we'll probably want a break-glass configuration item that devs can use to force seekability via the intermediate Vec, so they can quickly work around offending loaders they don't control.
I just removed all of the unnecessary "default" impls to cut down on the noise. I also merged with main.
Objective
AsyncSeektrait byAsyncSeekForwardforReaderto address #12880 #14194, we replacedAsyncSeekwithAsyncSeekForward. This allowed us to support more kinds of sources, but the cost was that loaders were more limited in what they could do with the reader.AsyncSeekForwardmade it difficult to deal with this (or impossible in some cases).AsyncSeektrait bound onReadermay limit options to stream bytes #12880.Solution
AssetLoaderto say which "features" of a reader it needs.AssetReaderso it can decide how to handle it.AssetReaderError::UnsupportedFeature.This design is kind of "weak" - there's no guarantee that a loader that requests
AnySeekwill get a reader that actually implementsAsyncSeek. Or on the other side, there's no guarantee that a loader actually requests the features that it uses. However, in practice it's likely enough: errors are likely to guide users to the correct situation. In the future, we could perhaps have a "sieve reader", which blocks any features the loader didn't explicitly request. Perhaps this is a debug only feature, or something that can be toggled.Testing