home-assistant-libs / voip-utils

Apache License 2.0
1 stars 5 forks source link

Opus payload hard set to "123" in RTP stream even through mutually agreed payload type is different #6

Closed BaTnz closed 1 year ago

BaTnz commented 1 year ago

With the latest PR I have put through we are building our SDP correctly but when sending RTP the payload type is still had set to 123...

https://github.com/home-assistant-libs/voip-utils/blob/b0b752b3816059de368e3aeabe746b42ec08748f/voip_utils/rtp_audio.py#L117

We'd need to modify the function send_audio to include a reference to our agreed upon payload type for that call and then modify this to provide it...

https://github.com/home-assistant/core/blob/a9d992c2def8c3da7e97a3100efc8e3b3f5e8531/homeassistant/components/voip/voip.py#L354

Not too sure of the inner workings of Home Assistant here and how it would get pulled from one to the other but that is what needs to happen in short.

synesthesiam commented 1 year ago

Thanks! I can take a look at this.

synesthesiam commented 1 year ago

@BaTnz What do you think of this? https://github.com/home-assistant-libs/voip-utils/pull/7

BaTnz commented 1 year ago

This actually breaks MizuDroid as now they elect payload type 113 but the code is expecting it to be 123 and borks..,

Traceback (most recent call last):
File "/usr/local/lib/python3.10/asyncio/events.py", line 80, in _run
self._context.run(self._callback, *self._args)
File "/usr/local/lib/python3.10/asyncio/selector_events.py", line 1035, in _read_ready
self._protocol.datagram_received(data, addr)
File "/usr/local/lib/python3.10/voip_utils/voip.py", line 131, in datagram_received
raise err
File "/usr/local/lib/python3.10/voip_utils/voip.py", line 121, in datagram_received
audio_bytes = self._rtp_input.process_packet(
File "/usr/local/lib/python3.10/voip_utils/rtp_audio.py", line 61, in process_packet
raise RtpError(
voip_utils.error.RtpError: Expected payload type 123, got 113

I'm not sure where the mapping of the dynamically assigned payload types for Opus comes from but I can't see in mentioned in any RFCs, do you know where 123 came from? MizuDroid sets the SDP as such...

v=0
o=Ntig 338 245 IN IP4 192.x.x.x
s=mizudroid
c=IN IP4 192.x.x.x
t=0 0
m=audio 14542 RTP/AVP 113 101
a=rtpmap:113 opus/48000/2
a=fmtp:113 maxplaybackrate=24000; sprop-maxcapturerate=24000
a=rtpmap:101 telephone-event/8000
a=fmtp:101 0-16
a=sendrecv

So ends up sending mono due to a=fmtp:113 maxplaybackrate=24000; sprop-maxcapturerate=24000 which creates audio issues for the integration as that is not what it is expecting.

I'm no expert on Opus so may need input from someone else.

agners commented 1 year ago

It seems that Opus uses dynamic payload type numbers (see https://en.wikipedia.org/wiki/RTP_payload_formats), so I guess 123 or 113 are just something random and implementation specific what it ends up being typically?

Maybe this has some hints to get this going? https://lists.cs.columbia.edu/pipermail/sip-implementors/2010-November/025993.html

BaTnz commented 1 year ago

Yea that's where I got to and RFC5787 lays it out pretty clearly so I think it has no intrinsic meaning and basically if you're using Opus you should be able to accept what ever parameters the other end wants to use that Opus allows.

We probably just need to document the exact parameters we're expecting Opus to have on our UA so that we can set them, and to set the media attributes we send so the other end knows what to expect from us.

So may be worth adding this so sip.py...

        body_lines = [
            "v=0",
            f"o={self.sdp_info.username} {self.sdp_info.id} 1 IN IP4 {call_info.server_ip}",
            f"s={self.sdp_info.session_name}",
            f"c=IN IP4 {call_info.server_ip}",
            "t=0 0",
            f"m=audio {server_rtp_port} RTP/AVP {call_info.opus_payload_type}",
            f"a=rtpmap:{call_info.opus_payload_type} opus/48000/2",
            f"a=fmtp:{call_info.opus_payload_type} maxplaybackrate=48000; stereo=1; sprop-maxcapturerate=48000; sprop-stereo=1",
            "a=ptime:20",
            "a=maxptime:150",
            "a=sendrecv",
            _CRLF,
        ]

But doing say didn't make anything better or may end, may help once the other stuff is fixed though.

With #8 merged it should send audio again but we're still setting the payload type on our outbound RTP incorrectly. I can see how it's in call_info now but it needs to be set in the Home Assistant integration when it creates RtpDatagramProtocol. Until then the RTP is not being built correctly so I can't say if the issues I'm seeing are related to that or other things.

synesthesiam commented 1 year ago

@BaTnz Can you confirm that this is fixed with the latest HA now?

BaTnz commented 1 year ago

Yes can confirm this now works, MicroSIP on Windows now working and MizuDroid works again.

Might want to still consider using the above mentioned attributes to try get the right type of Opus audio but that's a separate issue.