Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

No Location header in response when using WHIP #19

Open
lminiero opened this issue Nov 4, 2021 · 7 comments
Open

No Location header in response when using WHIP #19

lminiero opened this issue Nov 4, 2021 · 7 comments

Comments

@lminiero
Copy link

lminiero commented Nov 4, 2021

Hi there, it's me again 😄

I'm still working on those WHIP interop tests, so after getting your WHIP/WHAP client to work with my WHIP server (and Janus), I now wanted to test my WHIP client with deadsfu instead.

A first attempt got my WHIP client to crash, and apparently the cause was a missing Location header for the WHIP resource in the response to the POST. I fixed the crash in my client, but this now means I have no way to trickle candidates to deadsfu the WHIP way. Is this indeed not supported anymore? Should candidates be added as part of the SDP offer instead? If so I'll have to update the WHIP client as I think I don't support this yet.

Thanks!

@cameronelliott
Copy link
Member

@lminiero glad to see you again! 😄

You are right!

  • I need to add the Location header (it appears to be required)
  • I also need to implement PATCH, DELETE support

But, to your questions, Lorenzo, I have been carefully re-reading the current spec,
and it seems the server/resource can choose not to accept trickle candidates via PATCH.
So, it seems like there are two kinds of WHIP servers:

  • trickle-supporting
  • non-trickle-supporting

I think client developers who want to support both models, would need to provide an option at the client side: "all-candidates-in-sdp", or some thing like this.

See the my bolded text:

The initial offer by the WHIP client MAY be sent after the full ICE gathering is complete with the full list of ICE candidates, or only contain local candidates or even an empty list of candidates.

In order to simplify the protocol, there is no support for exchanging gathered trickle candidates from media server ICE candidates once the SDP answer is sent. The WHIP Endpoint SHALL gather all the ICE candidates for the media server before responding to the client request and the SDP answer SHALL contain the full list of ICE candidates of the media server. The media server MAY use ICE lite, while the WHIP client MUST implement full ICE.

The WHIP client MAY perform trickle ICE or an ICE restart {{!RFC8863}} by sending a HTTP PATCH request to the WHIP resource URL with a body containing a SDP fragment with MIME type "application/trickle-ice-sdpfrag" as specified in {{!RFC8840}} with the new ICE candidate or ICE ufrag/pwd for ICE restarts. A WHIP resource MAY not support trickle ICE (i.e. ICE lite media servers) or ICE restart, in that case, it MUST return a 405 Method Not Allowed response for any HTTP PATCH request.

@cameronelliott
Copy link
Member

I also posted a question regarding this to the list.

@lminiero
Copy link
Author

lminiero commented Nov 5, 2021

Yeah, and trickling candidates from the WHIP client is in principle not needed, if the server supports prflx candidates for instance. The issue I see, though, is that apparently ICE is never starting on the WHIP client side, and I'm wondering if the fact we never manage to send any candidate to deadsfu (trickle in WHIP not supported, and we're not putting them in our SDP offer) may be the cause. I'll investigate later today.

@lminiero
Copy link
Author

lminiero commented Nov 5, 2021

Mh the issue was actually different, as I was getting this SDP answer back:

v=0
o=- 6535663567843509221 1636104344 IN IP4 0.0.0.0
s=-
t=0 0
a=fingerprint:sha-256 ED:DC:7A:44:66:A4:11:D4:BF:96:15:F3:31:E9:26:AB:CB:B2:3E:93:76:A2:F0:64:2E:24:BA:67:51:BA:69:83
a=group:BUNDLE audio1
m=video 0 UDP/TLS/RTP/SAVPF 0
a=candidate:4056045782 1 udp 2130706431 192.168.1.232 60127 typ host
a=candidate:4056045782 2 udp 2130706431 192.168.1.232 60127 typ host
a=candidate:574494503 1 udp 2130706431 192.168.122.1 53410 typ host
a=candidate:574494503 2 udp 2130706431 192.168.122.1 53410 typ host
a=candidate:2425237054 1 udp 1694498815 2.232.93.8 55283 typ srflx raddr 0.0.0.0 rport 49809
a=candidate:2425237054 2 udp 1694498815 2.232.93.8 55283 typ srflx raddr 0.0.0.0 rport 49809
a=end-of-candidates
m=audio 9 UDP/TLS/RTP/SAVPF 100
c=IN IP4 0.0.0.0
a=setup:active
a=mid:audio1
a=ice-ufrag:kbcAxIXydVFKqALk
a=ice-pwd:xYcOITNCaTecemWIXmhLxZrLEAonWVeP
a=rtcp-mux
a=rtcp-rsize
a=rtpmap:100 OPUS/48000/2
a=fmtp:100 sprop-maxcapturerate=48000;sprop-stereo=0
a=rtcp-fb:100 nack pli
a=recvonly

The reason was I was offering VP8, that's apparently unsupported? Anyway, that caused audio not to be published either. Changing the pipeline to send H.264 instead did work ✌️

@lminiero
Copy link
Author

lminiero commented Nov 5, 2021

PS: just FYI, the hardcoded H.264 requirement in deadsfu (not sure if this can be changed/overridden?) prevented tests with free-whip and whip-go, unfortunately, as both are hardcoded to send VP8 instead.

@cameronelliott
Copy link
Member

@lminiero Lorenzo, thanks for trying those!!!
I have been avoiding the non-264 support, but it's time, I need to go implement VP8!

@cameronelliott
Copy link
Member

I may not get to this immediately, unfortunately, but it's clearly very important.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants