-
Notifications
You must be signed in to change notification settings - Fork 138
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
core/audio/RawSampleBuffer: allow using external storage #109
base: master
Are you sure you want to change the base?
Conversation
GStreamer uses its own Buffers which are handed to the downstream nodes in the processing graph. The nodes (elements) can be implemented in mutiple languages and the Buffers can be allocated elsewhere or be part of a Buffer pool. RawSampleBuffer provides a convenient mechanism to export decoded buffers to an unaligned bytes stream. In order to ensure data safety, the RawSampleBuffer manages its own sample storage and provides an accessor to obtain an immutable byte slice on the resulting data. When used in frameworks such as GStreamer, the byte slice must be copied to the target Buffer. The changes in this commit allow using the RawSampleBuffer with an external byte slice, thus avoiding the last copy.
Hi @fengalin, This would be a nice improvement to I will have to take a closer look at this once I get more time, but I do see one issue.
Fundamentally, this is the reason why I guess we could use |
On a different note. Since this change will cause a semver breaking change, and version 0.5 was just released, it may be a while before we can release a 0.6 since I want to roll-up a bunch of other not-yet-started breaking changes in 0.6. |
Indeed, I totally missed the initial alignment constraint. Given that the caller's "storage" allocator must be properly parametrized, I tend to shift position for the constructor slice type. Maybe we should impose an I'll push an update this weekend or early next week. I'll also take care of implementing a similar pattern for Noted for the semver change. One thing I thought about though: we could use a type alias so that existing use of |
Almost forgot: there's something else I need to address. In existing GStreamer Vorbis decoders, there is a mapping between the codec's channel positions and the framework's conventions. I think we could also add a channel mapping feature to |
... instead of [u8]. This ensures that caller has properly handled alignement constraints upfront.
413bca3
to
3025743
Compare
Hi @fengalin, Thanks for your work on this. Please give me some time to go over the changes more thoroughly, unfortunately I'm a bit short on time at the moment!
Does GStreamer output in the native Vorbis channel order? Symphonia outputs in the That being said, channel mapping(/mixing) was brought up in #43, and was a feature I had in my todo list. My idea was that on any audio buffer copy an optional channel matrix could be provided and the framework would take care of remapping and/or mixing the channels during the copy. However, I'm now thinking that it would be better for performance and source compatibility if separate copy functions are provided for such cases. |
No worries! I had this integration in the pipeline since more than a year, so it can wait all right! Just to clarify (and hopefully simplify) the channel mapping stuff: I was only referring to re-ordering identical number of channels. Channel mixing would not be part of the requirements for a GStreamer audio decoder as this is handled by dedicated elements. Anyway, I still need to dig a bit more in the Vorbis implementations to make sure I really need this, so let's just keep it aside for the moment. :) |
@pdeljanov: many thanks for the awesome crates!
I'm working on a Symphonia based GStreamer plugin (WIP branch). So far, I've integrated the FLAC and MP3 decoders, which can already be used in pipelines as drop-in replacements for any other similar decoders. My plan would be to integrate the other coders with "excellent" status and probably demuxers too. But before going any further, I'd like to get your feeling about this.
I'd also want to propose a modification that would make the integration a bit more optimal IMHO. Quoting the commit message:
I left open questions in the code (see
FIXME
s). After a back and forth, I decided to use an&mut [u8]
for the external storage type, but I also considered&mut [S::RawType]
. Reasons why I settle to&mut [u8]
are:Symphonia
already makes use of such a crate.RawSampleBuffer::as_bytes
.Is this acceptable to you?