core/audio/RawSampleBuffer: allow using external storage

[fengalin]

@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:

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.

I left open questions in the code (see FIXMEs). 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:

• We can still ensure safety by carefully checking the available size.
• It's more convenient for callers: otherwise, they would most likely need to cast their slice using an external crate and Symphonia already makes use of such a crate.
• This is symmetric to the output type returned by RawSampleBuffer::as_bytes.

Is this acceptable to you?

[pdeljanov]

Hi @fengalin,

This would be a nice improvement to RawSampleBuffer since it does save a copy. I also see this being useful for SampleBuffer since it suffers from the same problem.

I will have to take a closer look at this once I get more time, but I do see one issue.

RawSampleStorage::as_mut_slice calls bytemuck::cast_slice_mut to convert the &mut [u8] to a &mut [S::RawType]. This function may panic if the alignment of S::RawType is greater than the alignment of u8. Since the alignment of a u8 is 1, this is usually always true. The reason why this did not panic for you is because the Rust allocator on mainstream operating systems aligns all allocations to 16 byte boundaries. So there is an exception to the aforementioned rule if the pointer underlying the &mut [u8] is a multiple of the alignment of S::RawType. Since every S::RawType has an alignment less-than 16 bytes, and all allocations are aligned to 16 byte boundaries, the panic will never occur. Therefore, in practice there is no issue on these systems. However, this is not guaranteed and this could cause panics on other systems or if the allocator is changed. You can see how bytemuck checks for this here.

Fundamentally, this is the reason why RawSampleBuffer allocated a [S::RawType] and converted it to a [u8]. It can never fail, because a u8 has the least strict alignment of all types.

I guess we could use TryFrom instead of From for RawSampleStorage and do a check there?

[pdeljanov]

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.

[fengalin]

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 &mut [S::RawType] so that the compiler ensures caller dealt with this prior to reaching to Symphonia, which can't do anything if it's not properly aligned.

I'll push an update this weekend or early next week. I'll also take care of implementing a similar pattern for SampleBuffer.

Noted for the semver change. One thing I thought about though: we could use a type alias so that existing use of RawSampleBuffer are kept unchanged. I'll see if I can come up with a proper name for the lifetime dependent type.

[fengalin]

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 [Raw]SampleBuffer.

[pdeljanov]

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!

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 [Raw]SampleBuffer.

Does GStreamer output in the native Vorbis channel order? Symphonia outputs in the WAVEFORMATEXTENSIBLE order which is pretty typical I believe.

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.

[fengalin]

unfortunately I'm a bit short on time at the moment!

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. :)