3 new encoder methods #10

dh1tw · 2016-12-30T15:11:12Z

this PR supersedes the original PR. Is has incorporated the from @hraban.

It compiles on MacOS and Linux.

hraban

Very nice PR, thanks for updating the naming. I'm being critical because I want to make sure this patch is stand alone and we don't start pulling unrelated stuff in. But even those const things I agree, in principle, I just want to keep it separated from this (good) patch.

Thank you very much Tobias, your help is definitely appreciated 🙂 👍

hraban · 2016-12-30T15:16:14Z

opus.go

 	// Optimize encoding for low latency applications
-	AppRestrictedLowdelay = Application(C.CONST_APPLICATION_RESTRICTED_LOWDELAY)
+	AppRestrictedLowdelay = C.OPUS_APPLICATION_RESTRICTED_LOWDELAY


I'm not thrilled about these changes. They're outside the scope of this PR. I have no problem with a separate PR for this, but not as part of this patch. If this turns out to break something, I can't roll it back without deleting your other changes, too.

hraban · 2016-12-30T15:17:30Z

encoder.go

+	BandwidthSuperWideBand = C.OPUS_BANDWIDTH_SUPERWIDEBAND
+	// 20 kHz passband
+	BandwidthFullband = C.OPUS_BANDWIDTH_FULLBAND
+)


Give the bandwidths a separate type and you can remove the Bandwidth prefix from the variable names.

hraban · 2016-12-30T15:18:53Z

encoder.go

+	// Auto bitrate
+	Auto = C.OPUS_AUTO
+	// Maximum bitrate
+	BitrateMax = C.OPUS_BITRATE_MAX


Is this the maximum bitrate? Then MaxBitrate is a better name, like math.MaxInt for example.

Yes. I can sure rename it, although I thought it tried to be with the names as close as possible with the opus macros from C. It makes life easier understanding the documentation.

hraban · 2016-12-30T15:25:39Z

encoder.go

@@ -62,7 +129,7 @@ func NewEncoder(sample_rate int, channels int, application Application) (*Encode
 // Init initializes a pre-allocated opus encoder. Unless the encoder has been
 // created using NewEncoder, this method must be called exactly once in the
 // life-time of this object, before calling any other methods.
-func (enc *Encoder) Init(sample_rate int, channels int, application Application) error {
+func (enc *Encoder) Init(sample_rate int, channels int, application int) error {


here, too, I'm not a fan of including these unrelated changes in this patch. this patch is about adding encoder settings, but these changes are not necessary for that. I'm not opposed to them (I agree; they should be const, not var), but this PR is not the place to do that.

Ok - I understand. I will revert it.

hraban · 2016-12-30T15:32:59Z

forgot to mention that I'm especially grateful for the unit tests. They're a pain to ask for and unrewarding, but these are thorough and good. thanks

hraban · 2016-12-30T16:38:31Z

I understand, it's about the balance between the original libvorbis and Go standards. However, in this project, the convention is: when they clash, go with Go. It looks nicer for users of the library (whom I expect to not actually be familiar with the libvorbis API at all). It's not objectively better or worse, it's just what this library has a convention (hence also the recent rewrite of external names).

…

On Fri, Dec 30, 2016 at 4:35 PM, Tobias Wellnitz ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In encoder.go <#10>: > */ import "C" +const ( + // Auto bitrate + Auto = C.OPUS_AUTO + // Maximum bitrate + BitrateMax = C.OPUS_BITRATE_MAX Yes. I can sure rename it, although I thought it tried to be with the names as close as possible with the opus macros from C. It makes life easier understanding the documentation. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#10>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAIafNZiOPuTqiGXGIocAT45C4xEFMTnks5rNTLGgaJpZM4LYO3v> .

dh1tw · 2016-12-30T17:02:31Z

So I guess you would also expect a type Bitrate ? And the methods (enc * Encoder) Bitrateand (enc *Encoder) SetBitrate to accept / return Bitrate types, right?

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

// Bitrate returns the bitrate of the Encoder
func (enc *Encoder) Bitrate() (Bitrate, error) {
	var bitrate C.opus_int32
	res := C.bridge_encoder_get_bitrate(enc.p, &bitrate)
	if res != C.OPUS_OK {
		return 0, Error(res)
	}
	return Bitrate(bitrate), nil
}

hraban · 2016-12-30T17:16:41Z

No the bitrate is fine as an int because it is an actual integer. It's not an enum, it's the actual number used to represent the bitrate. So an int makes sense here.

As a rule of thumb you can try this: does it make sense to use + or - on the value? Then it's a number.

dh1tw · 2016-12-30T17:28:04Z

Ok. But as far as I can see there are also two Enums available for SetBitrate:

#define OPUS_AUTO                           -1000 /**<Auto/default setting @hideinitializer*/
#define OPUS_BITRATE_MAX                       -1 /**<Maximum bitrate @hideinitializer*/

So in this case there are Enums and integers.

hraban · 2016-12-30T17:36:04Z

Good point, however these are not enums but magic values. It's a common practice in C but it's not Go-like. Especially AUTO, which is just a terribly confusing thing because it's an int but not really, and you can pass it to two different functions, and its semantics depend on the function you pass it to, it's really kind of weird. I was planning to remove Auto from your PR after it's done and replace it with this, which, imo, is more Go like:

SetBitrate(br int) error { /* normal */ }
SetBitrateAuto() error { /* sets it to Auto. */ }
SetBitrateMax() error { /* sets it to max */ }

This completely hides the implementation details of that ugly OPUS_AUTO value from the caller and gives a nice, clean, Go-like API.

…Auto

dh1tw · 2016-12-30T18:09:44Z

updated the Pull Request. I hope that it matches now your expectations!

hraban

two small typos left.

hraban · 2016-12-30T18:14:47Z

encoder.go

+	return nil
+}
+
+// Complexity returns the bitrate of the Encoder


typo in the comment :)

hraban · 2016-12-30T18:16:08Z

encoder_test.go

+
+	err = enc.SetBitrateMax()
+	if err != nil {
+		t.Error("Error setting Auto bitrate:", err)


wrong error message

hraban · 2016-12-30T18:19:53Z

it looks great. thanks for implementing the Auto() and Max() variations.

dh1tw · 2016-12-30T18:48:49Z

fixed the typos... tnx for carefully checking!

dh1tw added 2 commits December 30, 2016 15:57

added new encoder methods & tests

df37abd

checking also error string

8b36630

hraban requested changes Dec 30, 2016

View reviewed changes

reverted to using types for enums; added SetBitrateMax and SetBitrate…

008490c

…Auto

dh1tw mentioned this pull request Dec 30, 2016

Set encoder bitrate #6

Closed

hraban approved these changes Dec 30, 2016

View reviewed changes

hraban requested changes Dec 30, 2016

View reviewed changes

hraban mentioned this pull request Dec 30, 2016

added 3 more setter / getter for Encoder Settings #8

Closed

fixed typos

f0e0e66

hraban approved these changes Dec 30, 2016

View reviewed changes

hraban merged commit dc3e8b1 into hraban:master Dec 30, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

3 new encoder methods #10

3 new encoder methods #10

dh1tw commented Dec 30, 2016

hraban left a comment

hraban Dec 30, 2016

hraban Dec 30, 2016

dh1tw Dec 30, 2016

hraban Dec 30, 2016

dh1tw Dec 30, 2016

hraban Dec 30, 2016

dh1tw Dec 30, 2016

hraban commented Dec 30, 2016

hraban commented Dec 30, 2016 via email

dh1tw commented Dec 30, 2016

hraban commented Dec 30, 2016

dh1tw commented Dec 30, 2016

hraban commented Dec 30, 2016

dh1tw commented Dec 30, 2016

hraban left a comment

hraban Dec 30, 2016

hraban Dec 30, 2016

hraban commented Dec 30, 2016

dh1tw commented Dec 30, 2016

3 new encoder methods #10

3 new encoder methods #10

Conversation

dh1tw commented Dec 30, 2016

hraban left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hraban commented Dec 30, 2016

hraban commented Dec 30, 2016 via email

dh1tw commented Dec 30, 2016

hraban commented Dec 30, 2016

dh1tw commented Dec 30, 2016

hraban commented Dec 30, 2016

dh1tw commented Dec 30, 2016

hraban left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hraban commented Dec 30, 2016

dh1tw commented Dec 30, 2016