Forward error correction flag in decode (decode_fec flag) about opus

[hraban]

Hi elinor, thanks for opening the discussion before submitting a patch :) ! and for your interest in contributing.

I've had a good look at this flag and how it should work, and tbh: I'm not completely clear on how it should be used, even in the original C API. Do you know what the point is, exactly? As far as I understand it: if you have lost a packet, and your incoming packet contains error correcting data (i.e. FEC enabled on the encoder, with appropriate assumed packet loss), it can be used to restore (an approximation?) of the lost packet. So, say your encoder sends packets: A, B, C, D. You only receive A, C, D. You call the decoder:

decode(A, pcm, decode_fec=false)
decode(C, pcm, decode_fec=true) // pcm must have the exact frame size of the missed data
decode(C, pcm, decode_fec=false)
decode(D, pcm, decode_fec=false)

Do you know if this is exactly how it's supposed to be used?

[stop-start]

Hi hraban,

As a disclaimer I'm not expert and haven't used this feature yet.
But yes, basically that's what it should do and how it should do it. From the documentation if the next packet doesn't contain the info needed then missing packet will simply be considered lost.
In my previous post I was mistaken when I suggested to let the flag set to 1. It should be set on the frame following the the missing one.

On a related matter, in the c api, there's an option to send a NULL pointer for "data" in order to indicate a lost packet. Here, the decode function will just return the error "opus: no data supplied".

[hraban]

yeah, the opus source code is basically the only reliable source of info I was able to find on this. it seems buffer = null invokes PLC: packet loss correction (?) , filling the pcm with "silence" of some sorts (is it just 000? or something smarter, based on the bordering wave functions?) and advancing the state of the decoder appropriately. I agree on the empty buffer thing, and while I don't like overloading method semantics, especially not using 'nil', I'm weary to add two whole more methods just for PLC. Especially considering how hybrid it is with FEC (if FEC data cannot fill an entire buffer, the first part of the pcm is filled with PLC, the rest with FEC).

All in all, I think we should probably add two methods:

(d *Decoder) DecodeFEC(next []byte, pcm []int16) error
(d *Decoder) DecodeFECFloat32(next []byte, pcm []float32) error

No need to return int if I understand it correctly, because this will always fill up the pcm buffer exactly, no?

Either which way, this will need some rigurous testing. Maybe full, value based testing can't be added to the test suite (signal -> opus -> signal is hard to test, opus being lossy and all), but we should at least pipe it to excel and see if the error decoded signal makes sense. I'll open another issue for allowing nil buffers, although I'm curious what the actual effect is on the decoded signal of supplying a null buffer.

What do you think?

[stop-start]

From what I read, their doc doesn't mention how their PLC implementation works. I think PLC stands for packet loss concealment (thanks wikipedia: https://en.wikipedia.org/wiki/Packet_loss_concealment)

I agree regarding the two methods and I think you're right about the fact that will fill up the pcm buffer, at least that's what I'm hoping for since my goal with this is to keep the length of the audio as close as possible to the original.

If you're ok with it, I would like to submit a PR in the coming days but I'm not sure I understand you regarding the testing process.

[hraban]

From what I read, their doc doesn't mention how their PLC implementation works. I think PLC stands for packet loss concealment (thanks wikipedia: https://en.wikipedia.org/wiki/Packet_loss_concealment)

Good catch.

I agree regarding the two methods and I think you're right about the fact that will fill up the pcm buffer, at least that's what I'm hoping for since my goal with this is to keep the length of the audio as close as possible to the original.

This should be fine. It does break the convention with the other methods a bit, because for those you could just pass an arbitrarily large buffer and have it filled to whatever is available. Now, the size of your input buffer matters, which means the caller will have to do some predictive slicing or choosing an exact buffer size to begin with. Since this call is likely to be mixed with the regular decode calls, and therefore share a buffer , this warrants a word of caution in the docs (which now suggest to just choose something large to be sure you’ll have space).

If you're ok with it, I would like to submit a PR in the coming days but I'm not sure I understand you regarding the testing process.

That would be lovely, thanks :) and thanks again for the heads up. Points of concern: - docs: those are iterative anyway, so anything which covers the new functionality is fine. It will be improved over time. I don’t have to be backwards compatible here :) - tests: how do we test the behaviour of these methods? Ideally, I’d like to supply a waveform , encode it, pass it through some decode and decodeFEC calls, then evaluate the output somehow. I understand this is a hard problem and I’m willing to cut some corners (see bottom of opus_test.go), but we’ll have to be at least a little thorough. Especially because neither of us is actually sure how these methods work / are supposed to work :) don’t worry too much about the test code being “clean” or “elegant”. I’m more concerned about them actually testing the code. Feel free to reorganise current code as you see fit, or copy/paste from other tests. All in all, I’m making it sound harder than it probably is. Most of the groundwork is already done, this should be a relatively straightforward job. Thanks for taking the time! Feel free to reach out if there are any questions or concerns. And don’t forget to add yourself to the AUTHORS list :)

…

— You are receiving this because you commented. Reply to this email directly, view it on GitHub, or mute the thread.

[stop-start]

I'm having some trouble with testing/using this. Like you said we don't really know what we're supposed to get and I don't really hear any differences between using FEC or telling opus that the packet is lost.
The packets need to be encoded the right way and although it seems they are (I'm currently using code written in C for the encoding part), all I hear when a packet is drop that the sound is cut.
Maybe i don't encode the right way or I don't use the decodeFEC right.
Anyway I'm still looking into how to check this.

[hraban]

The bottom of `opus_test.go` should contain a test routine with some code that can help with this. It generates a stereo signal with two sine waves, encodes it, decodes it and (commented out) prints the result to CSV. You can paste that csv in google sheets and plot a chart to see what’s happening. If you copy/paste that function and adapt it to drop some packets and decode using FEC, you should be able to see the effect of your patch. Let me know how it works out! Good luck Hraban

…

On 19 Mar 2018, at 13:09, elinor ***@***.***> wrote: I'm having some trouble with testing this. Like you said we don't really know what we're supposed to get and I don't really hear any differences between using FEC or telling opus that the packet is lost. The packets need to be encoded the right way and although it seems they are (I'm currently using code written in C for the encoding part), all I hear when a packet is drop that the sound is cut. Maybe i don't encode the right way or I don't use the decodeFEC right. Anyway I'm still looking into how to check this. — You are receiving this because you commented. Reply to this email directly, view it on GitHub, or mute the thread.

[stop-start]

I looked in encode.go and didn't see an option to configure the encoder with OPUS_SET_INBAND_FEC(1) and OPUS_SET_PACKET_LOSS_PERC.
It can be easily added though

[hraban]

Yeah, this will have to be part of your patch for it to work.

…

On 19 Mar 2018, at 13:29, elinor ***@***.***> wrote: I looked in encode.go and didn't see an option to configure the encoder with OPUS_SET_INBAND_FEC(1) and OPUS_SET_PACKET_LOSS_PERC. It can be easily added though — You are receiving this because you commented. Reply to this email directly, view it on GitHub, or mute the thread.