Forward error correction #18

stop-start · 2018-03-26T07:57:52Z

Issue: #17

Added relevant CTLs in encoder and decoder
Added methods decodeFEC and decodeFECFloat32
Added tests for all of those

For the FEC test, I manually "dropped" packets and used the csv with chart method using the code in the opus_test.go for visualizing the results. It looks good I think.
I attached the graphs pictures.

hraban · 2018-03-27T09:01:53Z

Thanks Elinor, this looks very promising. I'll review it during the week. 👍

hraban

Looks great. Some minor changes and consistency issues here and there, but nothing big. My only real concern is documentation: how do we explain to users how to use this? Are supposed to detect packet loss themselves, how are they to know the length of the lost packets, etc. It doesn't have to be a novel; a quick line or two in the readme + on the godoc string can help someone to figure out the rest for themselves.

Thanks for the great work.

hraban · 2018-03-30T17:57:02Z

encoder.go

+
+// SetInBandFEC configures the encoder's use of inband forward error
+// correction (FEC)
+func (enc *Encoder) SetInBandFEC(fec int) error {


this argument could be a boolean, as with SetDTX

hraban · 2018-03-30T17:57:34Z

encoder.go

+}
+
+// InBandFEC gets the encoder's configured inband forward error correction (FEC)
+func (enc *Encoder) InBandFEC() (int, error) {


return type boolean

hraban · 2018-03-30T18:23:36Z

opus_test.go

+	// This is hard to check programatically, but I plotted the graphs in a
+	// spreadsheet and it looked great. The encoded waves both start out with a
+	// string of zero samples before they pick up, and the G4 is phase shifted
+	// by half a period, but that's all fine for lossy audio encoding.


this comment relates to a stereo signal and isn't accurate here. you can remove it and just say "Commented out for the same reason as in TestStereo", for example.

hraban · 2018-03-30T18:23:56Z

opus_test.go

+	// This is hard to check programatically, but I plotted the graphs in a
+	// spreadsheet and it looked great. The encoded waves both start out with a
+	// string of zero samples before they pick up, and the G4 is phase shifted
+	// by half a period, but that's all fine for lossy audio encoding.


same on this comment being out of sync with the content.

hraban · 2018-03-30T18:27:06Z

encoder_test.go

+			t.Errorf("Unexpected encoder loss percentage value. Got %d, but expected %d",
+				lp, lossPerc)
+		}
+	}


this only test successes, not failures. can you add a test for -1 and 101, ensuring failure?

hraban · 2018-03-30T18:27:58Z

decoder_test.go

+		t.Fatalf("Couldn't decode data: %v", err)
+	}
+	samples, err := dec.LastPacketDuration()
+	if err!=nil{


make sure to run gofmt on all your changes. most editors / ides provide hooks to run it automatically on save, e.g. vim-go does it by default, emacs go-mode has a hook you can enable in .emacs. more info: https://blog.golang.org/go-fmt-your-code . I've added a new test to travis which automatically detects this: if you rebase your changes onto v2 it will show you if it worked:

git remote add origin upstream https://github.com/hraban/opus git fetch upstream git rebase upstream/v2 git push --force-with-lease

your PR will update to show the travis build status

Sorry about that, for whatever reason Goland is always removing the file watchers. I keep having to set "use gofmt on save"

hraban · 2018-03-30T18:30:07Z

decoder.go

@@ -105,3 +111,64 @@ func (dec *Decoder) DecodeFloat32(data []byte, pcm []float32) (int, error) {
 	}
 	return n, nil
 }
+
+// Decode encoded Opus data into the supplied buffer with forward error
+// correction. The supplied buffer will be entirely filled.


The first word of a godoc comment must be the name of the method. It's an odd convention, but a convention nonetheless. Weʼll have to stick to it.

Yeah sorry, that show that copy/paste is not always your friend...

hraban · 2018-03-30T18:30:30Z

decoder.go

+}
+
+// Decode encoded Opus data into the supplied buffer with forward error
+// correction. The supplied buffer will be entirely filled.


first word must be method name

hraban · 2018-03-31T11:38:21Z

decoder.go

+		(*C.uchar)(&data[0]),
+		C.opus_int32(len(data)),
+		(*C.opus_int16)(&pcm[0]),
+		C.int(cap(pcm)),


This brought to my attention a bug in all the Decode* functions :/ I don't remember why I originally used cap, but I think it's a bug. It's asymmetrical with io.Reader, whose signature I tried to emulate. It was probably originally chosen for alignment with the Encode* functions and the suggestion to resize them (data = data[:n]), but it doesn't make sense for Decode.

Anyway, this is all outside the scope of your PR, so it's all good here.

hraban · 2018-03-31T12:06:44Z

opus_test.go

+		}
+		pcmBuffer = pcmBuffer[:n]
+		if len(pcmBuffer) != n {
+			t.Fatalf("Length mismatch: %d samples in, %d out", len(pcmBuffer), n)


I'm not sure about this error, by the way. the length of the pcmBuffer does not seem to be the amount of samples fed to the encoder in this pass? especially after truncating to :n. What exactly is being tested, here?

The idea of the test is to lose two packets (I guess we can do it random but to be honest it was just easier like this). All of the packets have to be decoded without fec, that's where this line comes from.
In addition to that the packets coming after the "lost ones" have to be decoded with fec on (there you can see that I don't check for the length but just the error).

I agree the overall test logic itself is ok, but this specific guard is a tautology. after a = a[:n], you will always len(a) ≤ n, and because you start out with len(a) = FRAME_SIZE ⋙ n this is a tautology. You might be trying to test e.g. if n ≠ PACKET_SIZE?

TBH I just copy/pasted it from the TestCodec test, but I don't mind changing it to n!=PACKET_SIZE or actually n!=FRAME_SIZE since I changed it.

That's fine :) don't forget to change the error message. Currently you're testing for PACKET_SIZE and n, but reporting len(pcmBuffer) and n.

hraban · 2018-03-31T12:23:04Z

opus_test.go

+	const SAMPLE_RATE = 48000
+	const FRAME_SIZE_MS = 60
+	const FRAME_SIZE = SAMPLE_RATE * FRAME_SIZE_MS / 1000
+	const PACKET_SIZE = 480 // 10ms packet


This is in fact the frame, not the 60ms defined further up. Your entire audio stream is 60ms, the frame size is 10ms. Packets are more of a "transport level" term, when you start talking about networks. If you send every frame as a separate network message (which will typically be the case), then frame ~= packet. Either way, you'll want to use 10 as your FRAME_SIZE_MS, and use a different name for the 60 constant you defined above.

Thanks for the explanation, I wasn't sure about that.

hraban · 2018-03-31T13:14:45Z

opus_test.go

+			}
+
+			pcmBuffer := make([]int16, samples)
+			if err = dec.DecodeFEC(encodedData[i], pcmBuffer); err != nil {


this is cheating :) you're passing the "dropped" packet to decodefec. Should be encodedData[i-1]. This will get you a different output:

EDIT: Scratch that—I was wrong! "Forward" error correction doesn't mean the error correcting data is encoded forward, but backward! Which makes total sense, because you can't predict the data to come. So your test was correct.

Checked again the result graph

stop-start · 2018-04-02T12:21:53Z

I still need to add some explanations about FEC in the README.
I'm adding them right now

e-wave added 5 commits March 26, 2018 07:40

Added packet loss and inband fec encoder ctls

01412cb

Added get last packet duration in decoder ctls

5822db3

Added decode with fec + test

5ba421a

Added decodeFECFloat32 + test

68fb7a2

Added myself to autthors

c41fdce

hraban requested changes Mar 31, 2018

View reviewed changes

hraban reviewed Mar 31, 2018

View reviewed changes

e-wave added 9 commits April 2, 2018 13:31

(Set)InBandFEC - set and return bool instead of int

865db73

(Set)InBandFEC - changed tests to use bool instead of int

26bb951

Changed comments in opus_test.go

cec5e27

Added invalid packet loss test in encode_test.go

216d1ce

ran gofmt

bd2f797

Naming convention fix in decoder.go

101f2cb

Changed frame size to 10ms and number of frames const

028b653

Checked again the result graph

Comparing samples returned from decode with frame size in fec tests

479042b

Fixed wrong error message

5745b2b

Readme and godoc strings

5429761

hraban approved these changes Apr 2, 2018

View reviewed changes

hraban merged commit 0f2e0b4 into hraban:v2 Apr 26, 2018

hraban mentioned this pull request Apr 24, 2021

encoder: invalid argument #39

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Forward error correction #18

Forward error correction #18

stop-start commented Mar 26, 2018 •

edited

Loading

hraban commented Mar 27, 2018

hraban left a comment

hraban Mar 30, 2018

hraban Mar 30, 2018

hraban Mar 30, 2018

hraban Mar 30, 2018

hraban Mar 30, 2018

hraban Mar 30, 2018

stop-start Apr 2, 2018

hraban Mar 30, 2018

stop-start Apr 2, 2018

hraban Mar 30, 2018

hraban Mar 31, 2018

hraban Mar 31, 2018

stop-start Apr 2, 2018

hraban Apr 2, 2018 •

edited

Loading

stop-start Apr 2, 2018

hraban Apr 2, 2018

hraban Mar 31, 2018

stop-start Apr 2, 2018

hraban Mar 31, 2018 •

edited

Loading

stop-start commented Apr 2, 2018

Forward error correction #18

Forward error correction #18

Conversation

stop-start commented Mar 26, 2018 • edited Loading

hraban commented Mar 27, 2018

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

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 Apr 2, 2018 • edited Loading

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 Mar 31, 2018 • edited Loading

Choose a reason for hiding this comment

stop-start commented Apr 2, 2018

stop-start commented Mar 26, 2018 •

edited

Loading

hraban Apr 2, 2018 •

edited

Loading

hraban Mar 31, 2018 •

edited

Loading