feat: add marker bit when packetizing opus.

[ramyak-mehra]

Fixes #125

[xnorpx]

So markerbit should be set at "start" of talkspurt. Is it true that the first packet in a talkspurt is 1 byte?

[xnorpx]

https://datatracker.ietf.org/doc/html/rfc3551#section-4.1

[ramyak-mehra]

I might have read some details wrong, i actually added it at the end of a talk spurt, fixed that

[xnorpx]

You can rename this file to test_talk_startspurt: https://github.com/algesten/str0m/blob/main/tests/rtp_to_frame.rs

Then validate that marker bit get's added.

Ideally test the different combinations of rtp/frame api.

[xnorpx]

@algesten should some api be added for this, so in case dtx is set to false marker bit will never be set and it will not be negotiated over sdp.

[algesten]

Can we really auto-detect talk spurts without some audio analysis? I mean just because there's some noise on the line, doesn't mean there is a talk spurt starting. How does libWebRTC do this? (or pion if they support it)

[xnorpx]

Can we really auto-detect talk spurts without some audio analysis? I mean just because there's some noise on the line, doesn't mean there is a talk spurt starting. How does libWebRTC do this? (or pion if they support it)

Audio analysis is done in Opus so it will handle this.

I need to double check the packet pattern that Opus emits not sure this logic will work.

[xnorpx]

I don't think Pion supports this, best would be to check how libwebrtc set the marker bit on Opus packages.

[ramyak-mehra]

I took the inspiration from rtpopuspayloader

so in case dtx is set to false marker bit will never be set and it will not be negotiated over sdp.

I was thinking of passing down the UseDtx flag from sdp to codec but never found any other codec doing it, so skipped it for now. I did see something like H264CodecExtra to pass down options to de packetizer should something like that exists for packetizer as well?

Or probably at the start of initialising the struct codec params can be passed

[ramyak-mehra]

I didn't find a way to get access to previous packet inside the opus packetizer, since we don't cache it (something about audio packets not being nackable), so i changed the implementation a bit.

I have also added a rtp level test for the same.

[davibe]

For doing this only when dtx is enabled: we have FormatParams::use_dtx inside CodecSpec, maybe a simple solution would be impl this around here https://github.com/algesten/str0m/blob/5c4c9d54bb2331ff8a15a2e62e2575f6ec82286d/src/packet/payload.rs#L58 passing the CodecSpec down from the caller ?

[ramyak-mehra]

For doing this only when dtx is enabled: we have FormatParams::use_dtx inside CodecSpec, maybe a simple solution would be impl this around here

https://github.com/algesten/str0m/blob/5c4c9d54bb2331ff8a15a2e62e2575f6ec82286d/src/packet/payload.rs#L58

passing the CodecSpec down from the caller ?

That would mean all the packetizers would need to accept it, and its more like a initialising info thats once set would not change, we can pass it down at the start somehow

[lolgesten]

@ramyak-mehra when we instantiate the packetizer, the fmtp parameters are available:

            Payloader::new(params.spec)

The spec here is CodecSpec, which has a field for FormatParams, which in turn has the use_dtx setting. https://github.com/algesten/str0m/blob/5c4c9d54bb2331ff8a15a2e62e2575f6ec82286d/src/format.rs#L145

I think all is in place for that already.

[ramyak-mehra]

while we can pass it down i want to understand the rational behing passing extra_codec_params in each depacketize call. Are there para meters that change with each packet thats why its there? Do we want to enable both kinds of approach passing down with each call as well as at initilizing?

[davibe]

That would mean all the packetizers would need to accept

No. I meant it can be done from the caller of is_marker(). Like: marker = if audio && !dtx { false } else { packetizer…

[algesten]

while we can pass it down i want to understand the rational behing passing extra_codec_params in each depacketize call. Are there para meters that change with each packet thats why its there? Do we want to enable both kinds of approach passing down with each call as well as at initilizing?

I don't understand why we are talking about the depacketizer? This PR is about the packetizer, right?

No. I meant it can be done from the caller of is_marker(). Like: marker = if audio && !dtx { false } else { packetizer…

I mean that this has nothing to do with the code calling the packetizer. The use_dtx function is dependent on which packetizer we are talking about. This should be passed into creating the OpusPacketizer (as I illustrated above), and be remembered internally for that packetizer only.

[davibe]

If we do it via OpusPacketizer constructor I think we have take care of dynamic updates somehow. DTX can be activated and deactivated via js apis and user settings during an ongoing call. Currently payloaders are stored based on (pt, rid) so if an SDP renegotiation changes a:fmtp:... adding usedtx I am afraid we would not pick up the change, or do we ?.

[ramyak-mehra]

We can just ignore the sdp use_dtx, if it's off the decoder on the other side should just ignore the marker bit.

Thogh I am not sure how do we handle sdp re negotiations in this library will take a look

[algesten]

I think we have take care of dynamic updates somehow. DTX can be activated and deactivated via js apis and user settings during an ongoing call.

Thogh I am not sure how do we handle sdp re negotiations in this library will take a look

I'm not convinced this is an attribute that can be changed runtime. AFAIK, the only thing you can change on an already negotiated m-line is the direction.

However even if it can be changed runtime, why would you want that? I think we can just say str0m does not change this dynamically and we could potentially detect dynamic changes and reject them in incoming SDP.

[ramyak-mehra]

Updated the PR to pass down use_dtx and added tests on the packetizer itself. On a side note: I see we create a new payloader everytime we want to write. Seems a bit redundant though or am i missing something?