Add opus custom support by emlynmac · Pull Request #18 · alta/swift-opus

emlynmac · 2022-04-19T19:14:58Z

I've been working on an implementation of Jamulus' audio protocol, which uses a custom Opus setting.

The PR contains changes to get the custom mode up and running.

…es; add these to the custom implementation

ydnar · 2022-04-19T22:30:17Z

Big PR, thanks! Some initial feedback that I think should be addressed before I can do a thorough review:

I think there should be a CustomMode Swift type that wraps or is the underlying Opus C type.
The constructors for Encoder and Decoder could accept a CustomMode, or implicitly handle it if the specified frame size isn’t standard.
Please format according to the swiftformat spec in this repo. Changes with unformatted or differently formatted code will not be accepted.
Please separate out any submodule changes into another PR (or remove them).

Thanks!

emlynmac · 2022-04-19T22:42:12Z

Big PR, thanks! Some initial feedback that I think should be addressed before I can do a thorough review:

I think there should be a CustomMode Swift type that wraps or is the underlying Opus C type.

The constructors for Encoder and Decoder could accept a CustomMode, or implicitly handle it if the specified frame size isn’t standard.

Please format according to the swiftformat spec in this repo. Changes with unformatted or differently formatted code will not be accepted.

Please separate out any submodule changes into another PR (or remove them).

Thanks!

I forgot to update the submodule; done
Just ran swiftformat too

To your first points around breaking those out further, I originally started down this path but stopped as it didn't really help the module be more usable from an API perspective. The underlying calls are different enough that to wrapping them up together was clearer and didn't need to change the existing code at all.

ydnar · 2022-04-20T15:53:27Z

To your first points around breaking those out further, I originally started down this path but stopped as it didn't really help the module be more usable from an API perspective. The underlying calls are different enough that to wrapping them up together was clearer and didn't need to change the existing code at all.

As I understand it, Opus custom encoders/decoders exist primarily (or only) to support nonstandard frame sizes. I don’t think that justifies a new, somewhat duplicative expansion of this package’s public API.

How about this: could you propose a minimal API change that enables nonstandard frame sizes? We can start right here in the PR comments.

Once we have a solid API proposal, then implement it.

emlynmac · 2022-04-20T16:37:59Z

To your first points around breaking those out further, I originally started down this path but stopped as it didn't really help the module be more usable from an API perspective. The underlying calls are different enough that to wrapping them up together was clearer and didn't need to change the existing code at all.

As I understand it, Opus custom encoders/decoders exist primarily (or only) to support nonstandard frame sizes. I don’t think that justifies a new, somewhat duplicative expansion of this package’s public API.

How about this: could you propose a minimal API change that enables nonstandard frame sizes? We can start right here in the PR comments.

Once we have a solid API proposal, then implement it.

If there's a preferred API you'd like to make, I'm all up for that. I agree the almost duplication of some of the helper functions to expand the pointers is not ideal, but as I said - I did not want to disturb the other parts of the code here.
The crux of the issue is being able to handle the fame size and also to call opus_custom_encode rather than opus_encode. I've also had to ensure that the expected packet size and nils are handled by the decoder too.

Speaking of the existing code, it looks like it's not handling dropped packets. My understanding is that if you have a dropped packet you should feed nil into the decoder to cover that case.

The decode method in Opus.Decoder isn't able to handle that:

private func decode(_ input: UnsafeBufferPointer<UInt8>, to output: UnsafeMutableBufferPointer<Float32>) throws -> Int {
		let decodedCount = opus_decode_float(
			decoder,
			input.baseAddress!,
			Int32(input.count),
			output.baseAddress!,
			Int32(output.count),
			0
		)
		if decodedCount < 0 {
			throw Opus.Error(decodedCount)
		}
		return Int(decodedCount)
	}

emlynmac · 2022-04-20T19:58:23Z

Ok, I found some time :)

I've merged the coding / decoding changes in with the existing classes.
See what you think of the changes!

emlynmac · 2022-04-20T20:00:50Z

+		} else {
+			decodedCount = opus_decode(
+				decoder,
+				input.isEmpty ? nil : input.baseAddress,


This change means that if you pass an empty data packet, opus decoder gets the nil it needs to signify a dropped packet.

ydnar

I think this can be simplified further. Key detail is hiding the existence and ownership of the OpusCustomMode within Encoder and Decoder.

ydnar · 2022-04-21T16:32:02Z

@@ -0,0 +1,95 @@
+import AVFoundation


What’s the purpose of this file? Can it be deleted?

(I think it should be, if Opus.Encoder and Opus.Decoder can support nonstandard frame sizes.

ydnar · 2022-04-21T16:43:56Z

 			}
 		}

+		public init(customOpus: OpaquePointer,


Given the encoder and decoder typically exist on different machines, I think Decoder should own the OpusCustomMode and create it here.

It can be freed in deinit.

I disagree - custom encoders and decoders need the custom object; you need to have a custom object in order to use either.
It maybe in your use case you have encoding on one machine and decoding on another, but you have to have the same custom instance for both if you're using them together.

The constructor for an OpusCustomMode takes two arguments: a sample rate and a target frame size in number of samples, which must be 64-1024 plus some additional constraints on prime factors.

The sample rate can be derived from the AVAudioFormat in the constructor along with a customFrameSize parameter to the custom constructor for an Encoder and Decoder.

It’s probably useful to have common helper code to aid in the creation of a valid OpusCustomMode that throws an error, but isn’t part of this package’s public API.

It’s probably useful to have common helper code to aid in the creation of a valid OpusCustomMode that throws an error, but isn’t part of this package’s public API.

That is the Opus.Custom.swift file.

ydnar · 2022-04-21T16:46:25Z

 	class Decoder {
 		let format: AVAudioFormat
 		let decoder: OpaquePointer
+		let customFrameSize: Int32?


This can probably be a non-optional int that defaults to 0.

We’ll also need to add a let customMode: OpaquePointer here (see below).

Actually no, this is a way to avoid having a duplicate un-needed pointer.
If you have a custom frame size, then your decoder is a custom one.

There’s no function in the Opus library to extract the frame size used to initialize an OpusCustomMode, hence storing it here.

I don't follow your point here; can you clarify?

ydnar · 2022-04-21T16:50:07Z

+			decodedCount = opus_custom_decode(
+				decoder,
+				input.isEmpty ? nil : input.baseAddress,
+				size,


Why is packetSize passed as an argument here (and other decode methods) when it should just be equal to Int32(input.count)?

packet size is needed for the custom decoder to know how much data it has.
In the case of a dropped packet, input is nil, but the encoder still needs to know how much data got dropped

See comment at the top-level of this PR regarding dropped packets.

If the decoder was initialized with a custom frame size, then either use that, or have decodeDroppedPacket accept an (optional?) frame size argument.

Custom frame size and compressed packet size are not the same thing. Custom encode / decode need to know both in order to work.

ydnar · 2022-04-21T16:51:17Z

+	                    packetSize: Int32?) throws -> Int
+	{
+		var decodedCount: Int32 = 0
+		if let size = packetSize {


If possible, this should branch on the presence of a non-nil customMode instance variable, which implies the decoder was initialized with a custom frame size.

I've implemented this a different way; using the optional presence of a custom frame size.

When decoding a stream of packets, is the frame size consistent, or does it vary? If it’s consistent, then this parameter shouldn’t exist.

The size can be multiples of the underlying frame size

Why does it need the packetSize argument? Can that be inferred from input.count?

OK, now I've had my coffee...

Compressed packet size != frame size.

Both are needed.

OK, great. Then can that be taken from the customFrameSize instance variable?

ydnar · 2022-04-21T16:52:14Z


-		// TODO: throw an error if format is unsupported
 		public init(format: AVAudioFormat, application _: Application = .audio) throws {
+			customFrameSize = nil


If this is made non-optional, this can be customFrameSize = 0, with customMode = nil.

And then you need 2 new vars; customMode and customFrameSize.
Using an optional for customFrameSize means you don't need a separate variable.

ydnar · 2022-04-21T17:03:50Z

Speaking of the existing code, it looks like it's not handling dropped packets. My understanding is that if you have a dropped packet you should feed nil into the decoder to cover that case.

The decode method in Opus.Decoder isn't able to handle that:

Good observation. I’d be game to add support for this in a separate PR.

My instinct says that a decodeDroppedPacket method (rather than interpreting nil) is probably the right approach. Thoughts?

emlynmac · 2022-04-21T17:05:17Z

Speaking of the existing code, it looks like it's not handling dropped packets. My understanding is that if you have a dropped packet you should feed nil into the decoder to cover that case.
The decode method in Opus.Decoder isn't able to handle that:

Good observation. I’d be game to add support for this in a separate PR.

My instinct says that a decodeDroppedPacket method (rather than interpreting nil) is probably the right approach. Thoughts?

I've implemented a fix in the PR; no need to make a different method to handle the case of a pretty regular occurrence. Will make the API kinda strange to have to call a different decode API for nil.

ydnar · 2022-04-21T17:08:06Z

Speaking of the existing code, it looks like it's not handling dropped packets. My understanding is that if you have a dropped packet you should feed nil into the decoder to cover that case.
The decode method in Opus.Decoder isn't able to handle that:

Good observation. I’d be game to add support for this in a separate PR.
My instinct says that a decodeDroppedPacket method (rather than interpreting nil) is probably the right approach. Thoughts?

I've implemented a fix in the PR; no need to make a different method to handle the case of a pretty regular occurrence. Will make the API kinda strange to have to call a different decode API for nil.

But from the caller’s perspective, a dropped packet isn’t nil. It just never arrived, hence can be handled with a more specific API. The goal of this package was to present a Swift-like interface to the Opus library, and not necessarily mirror its C API.

emlynmac · 2022-04-21T17:10:20Z

Speaking of the existing code, it looks like it's not handling dropped packets. My understanding is that if you have a dropped packet you should feed nil into the decoder to cover that case.
The decode method in Opus.Decoder isn't able to handle that:

Good observation. I’d be game to add support for this in a separate PR.
My instinct says that a decodeDroppedPacket method (rather than interpreting nil) is probably the right approach. Thoughts?

I've implemented a fix in the PR; no need to make a different method to handle the case of a pretty regular occurrence. Will make the API kinda strange to have to call a different decode API for nil.

But from the caller’s perspective, a dropped packet isn’t nil. It just never arrived, hence can be handled with a more specific API. The goal of this package was to present a Swift-like interface to the Opus library, and not necessarily mirror its C API.

If you look at how you'd be using this, it would be from a jitter buffer. Typically that will just have the read method called periodically as the audio system requests more data.
The buffer would either return a compressed packet or a nil if no data. Simply feeding that into the encoder is somewhat cleaner than calling a separate method that ultimately calls the same opus method under the hood.

ydnar · 2022-04-21T17:18:46Z

If you look at how you'd be using this, it would be from a jitter buffer. Typically that will just have the read method called periodically as the audio system requests more data. The buffer would either return a compressed packet or a nil if no data. Simply feeding that into the encoder is somewhat cleaner than calling a separate method that ultimately calls the same opus method under the hood.

Right now, there are two public decode methods. One accepts Data, and the other accepts UnsafeBufferPointer<UInt8>.

Adding support for nil packets to both of these methods doesn’t seem great, and adding two additional methods to decode a dropped packet also doesn’t seem great.

emlynmac · 2022-04-21T17:20:40Z

If you look at how you'd be using this, it would be from a jitter buffer. Typically that will just have the read method called periodically as the audio system requests more data. The buffer would either return a compressed packet or a nil if no data. Simply feeding that into the encoder is somewhat cleaner than calling a separate method that ultimately calls the same opus method under the hood.

Right now, there are two public decode methods. One accepts Data, and the other accepts UnsafeBufferPointer<UInt8>.

Adding support for nil packets to both of these methods doesn’t seem great, and adding two additional methods to decode a dropped packet also doesn’t seem great.

Both of those methods already have support in my PR. No changes needed; just using an empty data signifies a nil, which is then passed into the decode method.
This has the added benefit of not crashing in the case where the bufferPtr is nil.

ydnar · 2022-04-21T17:27:42Z

@emlynmac thanks for your efforts here, I appreciate it.

I think we should split out decoding dropped packets into a separate thread of work (probably a new PR), which is relevant for both custom and non-custom modes. I think this could help improve/inform the public API design for custom mode support.

emlynmac · 2022-04-21T17:30:48Z

@emlynmac thanks for your efforts here, I appreciate it.

I think we should split out decoding dropped packets into a separate thread of work (probably a new PR), which is relevant for both custom and non-custom modes. I think this could help improve/inform the public API design for custom mode support.

You're welcome, I needed it to get jamulus working on swift, so I figured I'd pass it back.

If you want to refactor the dropping into another method, then that's def another PR. I need it to work for now, but that's why we have forks :)

ydnar · 2022-04-21T17:37:46Z

I want this to land. Supporting dropped packets and supporting custom modes are two distinct and valuable features, and should be treated as such. Given that dropped packet support is relevant for all users of this package, I think it’s a good candidate to extract and land first. Then we can streamline and simplify the discussion around the design for support for custom modes.

emlynmac · 2022-04-21T17:47:46Z

I want this to land. Supporting dropped packets and supporting custom modes are two distinct and valuable features, and should be treated as such. Given that dropped packet support is relevant for all users of this package, I think it’s a good candidate to extract and land first. Then we can streamline and simplify the discussion around the design for support for custom modes.

From my perspective, the cleanest way would be to let the public decode methods take optionals.
That way, you can have a separate internal method to handle the dropped packet case, but the external API is unchanged.

emlynmac added 28 commits August 22, 2021 18:45

Enable Opus CUSTOM_MODES

b96e0ad

Add comma

144063f

Add CELT headers / source

fb5c31c

Include base header / source makefiles

04fa515

Wrap variadic ctl functions for swift to import

db32719

Wrap up some variadic functions

d1be9a6

rename to copuswrapper

80f7e0f

Move external header to export directory

dc5f841

Add custom mode extensions to support Jamulus

4e452fd

Add jamulus custom configuration

6104386

Enable custom encoder / decoders to create higher level wrapper class…

7273180

…es; add these to the custom implementation

Enable access outside the module

40be05d

Add custom decode / encode functions and contain frameSize

593766d

Fix up encoder bug

605b9ec

Set frame size in encode

fefebd7

Generalize the ioctl call

3204c1a

make the framesize public

b0c9e3b

Enable sample count to be passed in as a multiplier of frameSize

c898e20

Rename method parameter to reflect correct usage

61b6a4b

Ensure error code is passed back

9fa606f

Add explicit return

21164e1

Allocate a buffer properly

bada8cf

Handle the case of null data, aka packet loss

ed39aac

Add documentation; add some safety

2c72a72

Merge branch 'alta:main' into main

3ff24bf

Merge branch 'main' into jamulus-coders

566f056

Tidy some changes prior to PR back to fork source

b41d414

More whitespace formatting

40fb047

Update submodule to correct (master branch version)

e3788e3

Apply swiftformat rules

8b656b3

emlynmac added 2 commits April 19, 2022 15:45

Revert swiftformat changes that cause changes to master

e7cb649

One more swiftformat change revert

a7f8272

ydnar reviewed Apr 20, 2022

View reviewed changes

Comment thread Sources/Opus/Opus.Custom.swift Outdated

emlynmac added 2 commits April 20, 2022 09:52

Change return type on ctl wrapper method

a5b29aa

Merge opus custom with existing en/de-coders

712b0ff

Apply swiftformat

33c68f4

emlynmac commented Apr 20, 2022

View reviewed changes

ydnar requested changes Apr 21, 2022

View reviewed changes

Rename to customFrameSize

033520f

emlynmac and others added 5 commits April 21, 2022 11:12

One more rename

aa3db6f

Ensure that the compressed size is actually passed down

42d044f

Always build optimized builds of Opus

f5f8735

Merge branch 'main_upstream'

00d42cf

Merge branch 'main' into jamulus-coders

a48104d

Uh oh!

Conversation

emlynmac commented Apr 19, 2022

Uh oh!

ydnar commented Apr 19, 2022

Uh oh!

emlynmac commented Apr 19, 2022

Uh oh!

ydnar commented Apr 20, 2022

Uh oh!

Uh oh!

emlynmac commented Apr 20, 2022

Uh oh!

emlynmac commented Apr 20, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ydnar left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

emlynmac Apr 21, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ydnar commented Apr 21, 2022

Uh oh!

emlynmac commented Apr 21, 2022

Uh oh!

ydnar commented Apr 21, 2022

Uh oh!

emlynmac commented Apr 21, 2022

Uh oh!

ydnar commented Apr 21, 2022

Uh oh!

emlynmac commented Apr 21, 2022

Uh oh!

ydnar commented Apr 21, 2022

emlynmac Apr 21, 2022 •

edited

Loading

emlynmac commented Apr 21, 2022 •

edited

Loading