OnDataChannel not triggered by remote peer

[keroserene]

Must be solved before a Go-to-Go data channel can be established.
This was already in email form, but listing this issue in greater detail here:

Currently, go-webrtc only works when it acts as the "offerer". This means datachannel = PeerConnection.CreateDataChannel(...) locally, which triggers ICE negotiation. Once connectivity is established, datachannel.OnOpen callback fires, and then bytes can exchange successfully via datachannel.Send(...) and datachannel.OnMessage(...).

Here's the current connectivity determined through my chat demo:

• Browser-to-browser, in chrome or firefox, works well.
• Go-to-Chrome works, but only when Go initiates.
• Go-to-Firefox works, but only when Go initiates, and also the opened datachannel is only unidirectional from firefox to Go.

When go-webrtc attempts to act as the "answerer", connectivity has so far never succeeded. This means PeerConnection.SetRemoteDescription(...) using received SDP offer from remote peer. However, ICE negotiation never completes, and the OnDataChannel callback does occur as expected.

More info:

Tracing with gdb, webrtc::WebRtcSession::CreateDataChannel is being called in response to setRemoteDescription, as expected. (when the SDP indicates remote peer created a datachannel). After that, there needs to be some sort of initial setup messages for the datachannel internally, once the negotiation succeeds, which should transition the readystate to "open", and trigger .OnDataChannel. But this isn't happening.

Rather, there are just many periodic webrtc::WebRtcSession::OnSentPacket_w(...)'s firing, but something is being lost / failing relatively silently.

[arlolra]

Here's my brain dump for now.

At this point https://github.com/keroserene/go-webrtc/blob/master/demo/chat/chat.go#L204
I'm assuming you want to do start(false)

However, if this isn't true https://github.com/keroserene/go-webrtc/blob/master/demo/chat/chat.go#L169
then WebRtcSession::ConnectDataChannel isn't called and this

  data_channel_->SignalReadyToSendData.connect(webrtc_data_channel,
                                               &DataChannel::OnChannelReady);

doesn't fire.

When it does, however, IsReadyToSend() returns false before calling OnDataChannelReadyToSend(send); which makes

void DataChannel::OnChannelReady(bool writable) {
  writable_ = writable;
  if (!writable) {
    return;
  }

  SendQueuedControlMessages();
  SendQueuedDataMessages();
  UpdateState();

return early and prevents the UpdateState() from being called.

That's because, IsSendContentDirection(local_content_direction_) is false because when DataChannel::SetLocalContent_w sets it,

-> 2117   set_local_content_direction(content->direction());
   2118   ChangeState();
   2119   return true;
   2120 }
(lldb) p content->direction()
(cricket::MediaContentDirection) $0 = MD_RECVONLY

When it works (start in go),

2117      set_local_content_direction(content->direction());
   2118   ChangeState();
   2119   return true;
   2120 }
(lldb) p content->direction()
(cricket::MediaContentDirection) $0 = MD_SENDRECV

All this to say, when you add the answer, local_content_direction_ needs to be MD_SENDRECV, I think.

[arlolra]

This seems to be because, answer->streams().empty() when we receive the offer.

    case MD_SENDRECV:
      answer->set_direction(answer->streams().empty() ? MD_RECVONLY
                                                      : MD_SENDRECV);

[arlolra]

Setting add_legacy_stream to true in CreateMediaContentAnswer (in lldb) gets me to "------- chat enabled! -------" but then go crashes from a bad access at,

   106  int CGO_Channel_ReadyState(CGO_Channel channel) {
   107    auto dc = (webrtc::DataChannelInterface*)channel;
   108    assert(NULL != dc);
-> 109    return dc->state();

[arlolra]

Now seem to be hitting: https://bugs.chromium.org/p/webrtc/issues/detail?id=4757

[keroserene]

Awesome tracing!! 👍
What's up with legacy streams, though? Is that due to enabling RTP and disabling DTLS for now?
The state machine bug looks concerning. Will try to poke at this tomorrow~

[arlolra]

add_legacy_stream was just a quick hack so that answer->streams().empty() returned a falsy value so that the direction would be set to MD_SENDRECV. Enabling RTP seems to have the same effect (it puts sendrecv in the offer).

[arlolra]

Despite my elation at a go-go chat last night, I don't think this is solved. The constraint we want (as you noted) is,

{
    optional: [
      { DtlsSrtpKeyAgreement: true },
      //{ RtpDataChannels: true },
    ],
}

for DTLS/SCTP Data Channels, which are still stuck in the MD_SENDRECV bind as above :(

[arlolra]

Aha, well, the merged patches did do one good things (fixing trying to access the deref'd pointer). If I try the add_legacy_stream hack from #19 (comment) with the above constraint, we get a nice DTLS/SCTP DC that works go-go. So, this is close!

[keroserene]

Whoa, awesome! Right now I'm really excited that Go+Go chat works at all :) (re: #22 (comment))

I'm wondering if maybe we didn't build from the correct release branch of webrtc, given that the same webrtc provides working DTLS/SCTP in chrome.

[keroserene]

More notes:

With a Chrome+Go chat session where Go is the "answerer", after copying pasting the generated answer and candidates, Chrome consistently thinks channel has opened successfully and allows chat to happen, while Go doesn't.

<webrtc::WebRtcSession::OnDataChannelMessageReceived is never fired.

However!
cricket::DtlsTransportChannelWrapper::OnReadPacket does fire, so we know the ICE negotiation is fine, and some packets are showing up.... this is followed by:
cricket::P2PTransportChannel::OnReadPacket ->
cricket::Connection::OnReadPacket ->
cricket::UDPPort::HandleIncomingPacket -> ... which eventually gets us to:
cricket::DtlsTransportChannelWrapper::OnDtlsEvent
which calls:
cricket::TransportChannel::set_writable(bool) using true.

So DTLS is being setup. Furthermore, OnChannelReady (from an earlier comment) shouldn't be returning early, as writable is true, and UpdateState() should be getting called. Something else must be getting in the way.

[keroserene]

Yet more notes:

peerconnection::InternalCreateDataChannel is never called, but is needed in order to push a channel to sctp_data_channels_. This is because OnDataChannelOpenMessage never fires, because WebRtcSession::SignalOpenDataChannelMessage never fires, because WebRtcSession::OnDataChannelMessageReceived never fires.

...

However, WebRtcSession::CreateDataChannel does fire after SetRemoteDescription. This connects the WebRtc::OnDataChannelMessageReceived callback to the sigslot of the created data channel, .SignalDataRecieved. (Later that sigslot gets reconnected to the data channel's OnDataReceived for actual data exchange)

Meanwhile, some stuff happens in a different thread with TransportController and SctpDataMediaChannel.

Soon after, PeerConnection::OnDataChannelCreated does get called! This then loops through sctp_data_channels_, trying to fire their OnTransportChannelCreated's, except sctp_data_channels_ is still empty.

...

Now I need to look into why SctpDataMediaChannel::OnMessage never fires in any case, because that should trigger .SignalDataReceived sigslot, which should lead to the open message.

[arlolra]

Ok, I'm pretty convinced the problem is the call to SetStreams in GetOptionsForAnswer. It doesn't have a case for MEDIA_TYPE_DATA in the streams collection. So, options.streams ends up being empty and then the answer ends up being set with MD_RECVONLY (which makes sense given the symptoms of this bug). This was described in #19 (comment)

So, I looked on master and the function is now called AddSendStreams and the implementation is,

  for (const auto& sender : senders) {
    session_options->AddSendStream(sender->media_type(), sender->id(),
                                   sender->stream_id());
  }

I did a gclient sync to HEAD tonight (bd7d8f7e2b824a887aa12236cb6185d446d7da61) and rebuilt. Thankfully the APIs we're using haven't changed much since we locked a commit and the package built successfully.

I'm happy to report go-go DTLS/SCTP DC, as well as chrome-go (with chrome the offerer), working as expected.

That was fun!

[keroserene]

Super exciting! :) Thanks for figuring that out. Had a feeling it'd be something silly like gclient sync to the right version... about to try it.

Do you know if there's a specific release branch, instead of HEAD, we should use for stable DTLS/SCTP data channels? Otherwise I suppose pinning our build script to commit bd7d8f7e2b824a887aa12236cb6185d446d7da61 sounds fine.

(It seems the next thing we need is to make the libwebrtc_magic build script solid and easy to use for everyone else)

[keroserene]

Confirmed DTLS/SCTP DC working as well! 👍

Had to remove the chat demo's bundle policy, and had to update the include/**/*.hs, as a few things changed.

Both Go+Go, and Go+Chrome (both directions) work great.

With Firefox it's a bit fussy (the paste order of offers matters, and sometimes the data still only traverses in one direction.) but Firefox + Chrome are also fussy anyways. We may need to worry more about that after more progress on the PT.

For now, hurray, thanks & that was indeed fun.

[arlolra]

\o/