added 3 more setter / getter for Encoder Settings

[dh1tw]

Hi,

I added the following methods for the Opus Encoder:

• set/get bitrate
• set/get complexity
• set/get max bandwidth

I took also the liberty to suggest a slightly different way for using the opus defines. Instead of using the specific type Application (var int), I suggest to use directly C.opus_int32. This allows to define
the opus defines as const (what they should be IMHO).

All new methods have their respective unit tests.

[hraban]

Hi @dh1tw , thanks for the PR! There's a few problems with it. First off, I just changed the naming convention to be more go-like at Brad Fitzpatrick's behest (see #7). This is a bit unfortunate and I don't want to leave you with the work, so I'd be more than happy to fix this in your PR.

With the patch itself though, there are a few points of feedback I'd like to address before merging it in:

• copyright notices removed in encoder_test.go (can't do that)
• we can't export C.anything. I don't want C nor the underlying opus lib (so especially not C.opus_int32) to leak to our users. See it this way: today, this is a wrapper around the C libraries. But tomorrow, we should be able to implement the opus codec in pure go without breaking the API.
• aside from that, your proposed solution only works on mac, not in Linux / Docker (this would have been clear from Travis CI had it built, but it didn't, because of the merge conflict. Once those are solved Travis will automatically run and you will see the result there.);

+ exec go install -v
app
# app
./opus.go:21: const initializer *_Cvar_CONST_APPLICATION_VOIP is not a constant
./opus.go:23: const initializer *_Cvar_CONST_APPLICATION_AUDIO is not a constant
./opus.go:25: const initializer *_Cvar_CONST_APPLICATION_RESTRICTED_LOWDELAY is not a constant
The command '/bin/sh -c go-wrapper install' returned a non-zero code: 2

• try to adhere to the naming convention for the bridge functions (bridge_...)
• some of the bridge functions use an opus_int32 to store and return the response value from the ctl function, but opus_encoder_ctl returns a plain C.int.
• I like the error checks for the ctl() calls. We didn't do that in the DTX ones but we should have.
• however, the spec is not ret < 0 but ret != OPUS_OK. So:

// SetBitrate sets the bitrate of the Encoder
func (enc *Encoder) SetBitrate(bitrate int) error {
	res := C.bridge_encoder_set_bitrate(enc.p, C.opus_int32(bitrate))
	if res != C.OPUS_OK {
		return opusError(res)
	}
	return nil
}

• The variable names for the bandwitdh and bitrate constants should not have an OPUS_ prefix. That's implicit from being in the opus package. Consider how it would look for a user: opus.OPUS_BITRATE_MAX.
• the type for bitrate should be int (not even C.int), given that that is the type of the function you're supposed to pass it to (SetBitrate)
• bandwidth is an enum, so it should have a type defined for it. Then you can also drop the Bandwidth part entirely from the name. Which leaves you with:

type Bandwidth int

var (
	// 4 kHz passband
	Narrowband = Bandwidth(C.OPUS_BANDWIDTH_NARROWBAND)
	// 6 kHz passband
	Mediumband = Bandwidth(C.OPUS_BANDWIDTH_MEDIUMBAND)
	// 8 kHz passband
	Wideband = Bandwidth(C.OPUS_BANDWIDTH_WIDEBAND)
	// 12 kHz passband
	SuperWideband = Bandwidth(C.OPUS_BANDWIDTH_SUPERWIDEBAND)
	// 20 kHz passband
	Fullband = Bandwidth(C.OPUS_BANDWIDTH_FULLBAND)
)

• and since these are encoder specific, they go in encoder.go
• thanks for the tests! :)

Lovely work, thanks again! If you can have a look at these points, I'll fix the naming collisions that occurred after #7 and merge it in.

Cheers!

---

ps if you want to try it in Docker, put this in a Dockerfile in the same dir:

FROM golang:1


RUN apt-get update && apt-get -y install libopus-dev libopusfile-dev

# Everything below is copied manually from the official -onbuild image,
# with the ONBUILD keywords removed.

RUN mkdir -p /go/src/app
WORKDIR /go/src/app

CMD ["go-wrapper", "run"]
COPY . /go/src/app
RUN go-wrapper download
RUN go-wrapper install

and run:

$ docker build -t test-opus .

[hraban]

PS also feel free to add yourself to the AUTHORS file please. Thanks :)

[dh1tw]

Hi @hraban,

yeah, I realised too late that you already added a few new commits with Brat Fitzpatricks PRs. I will adopt the naming conventions.

copyright notice:
Sorry, didn't meant to remove it. Should of course remain in the file

exporting consts instead of vars:
I can understand that you don't want to export C.anything. However I consider it an heroic goal to reimplement the opus codec in go. I think the codec is extremely well written and very easy bindable to any language. I personally doubt that anyone will undergo this major task to rewrite the entire codec, just to have the satisfaction of having it written entirely in go.

IMHO your additional layer just adds unnecessary complexity.

const (
	// Optimize encoding for VOIP
	APPLICATION_VOIP = C.OPUS_APPLICATION_VOIP
	// Optimize encoding for non-voice signals like music
	APPLICATION_AUDIO = C.OPUS_APPLICATION_AUDIO
	// Optimize encoding for low latency applications
	APPLICATION_RESTRICTED_LOWDELAY = C.OPUS_APPLICATION_RESTRICTED_LOWDELAY
	// Auto bitrate
	AUTO = C.OPUS_AUTO
	// Maximum bitrate
	BITRATE_MAX = C.OPUS_BITRATE_MAX
	//4 kHz passband
	BANDWIDTH_NARROWBAND = C.OPUS_BANDWIDTH_NARROWBAND
	// 6 kHz passband
	BANDWIDTH_MEDIUMBAND = C.OPUS_BANDWIDTH_MEDIUMBAND
	// 8 kHz passband
	BANDWIDTH_WIDEBAND = C.OPUS_BANDWIDTH_WIDEBAND
	// 12 kHz passband
	BANDWIDTH_SUPERWIDEBAND = C.OPUS_BANDWIDTH_SUPERWIDEBAND
	// 20 kHz passband
	BANDWIDTH_FULLBAND = C.OPUS_BANDWIDTH_FULLBAND
)

The const declaration casts the C preprocessor #defines automatically to a go native int. It easy to verify:

fmt.Println("opus c #define as go const; Type:", reflect.TypeOf(BANDWIDTH_NARROWBAND))

so IMHO the following part is not needed:

/ Access the preprocessor from CGO
const int CONST_APPLICATION_VOIP = OPUS_APPLICATION_VOIP;
const int CONST_APPLICATION_AUDIO = OPUS_APPLICATION_AUDIO;
const int CONST_APPLICATION_RESTRICTED_LOWDELAY = OPUS_APPLICATION_RESTRICTED_LOWDELAY;

I will prepare a new PR.

[hraban]

I did some research into the cgo + c preprocessor thing and it turns out something mysterious changed since last year. Note that OPUS_APPLICATION_VOIP is not a symbol but a macro and so it wasn't available in CGO, initially. Now, somehow, this changed, since last year. I'm not sure what happened, whether it is maybe the underlying compiler or linker that is doing something extra, or if it's an actual change in CGO. Either way, I'd like to keep this as-is until I'm 100% that this is intended behavior, and not just "accidentally working." Go (especially cgo) breaks "backwards compatibility" all the time when the original behavior wasn't officially in the spec (see the new GC and passing go heap pointers to C in 1.6).

It's definitely interesting, and I'll keep an eye out for what exactly is happening here, but long story short: it's outside the scope of this PR.

[hraban]

By the way, some context for the part about rewriting: golang/go#13432 and https://github.com/golang/proposal/blob/master/design/13432-mobile-audio.md.

[hraban]

superseded by #10