-
Notifications
You must be signed in to change notification settings - Fork 118
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
Expose an AsyncReadOnlySource #256
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.
At first glance this looks reasonable enough to me; thanks for putting it together. AsyncReadOnlySource
is a useful utility to expose.
One minor nit/question. Otherwise, I would ignore the CI failure on missing documentation. Those are benchmark-harness specific methods which should be marked s.t. the missing_docs
lint skips them. I'm not sure what's changed in nightly to cause this, but I'll tackle that shortly.
Makefile.toml
Outdated
@@ -30,7 +30,7 @@ command = "cargo" | |||
dependencies = ["format"] | |||
|
|||
[tasks.build-variants] | |||
dependencies = ["build", "build-gateway", "build-driver", "build-simd"] | |||
dependencies = ["build", "build-gateway", "build-driver"] |
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.
Can I ask why you've done this? async-simd
is still supported as of 0.4.x.
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.
This makes some tests run when rebased on top of serenity-next. Probably due to this commit:
05cf36b#diff-9375fd04332c86472d7be397ef09428cb86babd8826880a5835bd1d1c1bdbc08L11
I have removed the commit.
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.
Yeah, the support is removed from next
but still present here. Thanks for fixing that up.
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.
Looks good to me, thanks; happy that this is non-breaking since most of the HlsStream
internals are private. Feel free to merge when you're ready!
EDIT: D'oh, I'll pull it in.
HlsStream
already wraps any struct with theAsyncRead
trait into anAsyncMediaSource
, so rename it toAsyncReadOnlySource
and expose it in the crate. This is very similar to the sync versionsymphonia_core::io::ReadOnlySource
.A possible use case would be streaming PCM samples to a discord voice channel:
Unfortunately, doing
cargo make ready
results in a lot of errors and I am not familiar enough with the codebase to add docs or fix them. However, since this is mostly shuffling around existing code, there shouldn't be any additional errors.