Skip to content
This repository has been archived by the owner on Aug 25, 2021. It is now read-only.

RFC: Client WebRTC API #7

Merged
merged 24 commits into from
Jan 7, 2019
Merged

RFC: Client WebRTC API #7

merged 24 commits into from
Jan 7, 2019

Conversation

alexlapa
Copy link
Collaborator

@alexlapa alexlapa commented Dec 17, 2018

Required by #6

Rendered

@alexlapa alexlapa added feature New feature or request k::api Related to API (application interface) k::design Related to overall design and/or architecture k::documentation Related to project documentation labels Dec 17, 2018
@alexlapa alexlapa self-assigned this Dec 17, 2018
@alexlapa
Copy link
Collaborator Author

alexlapa commented Dec 18, 2018

@tyranron ,

Примеры в json'е. Но сами методы оставил растовыми. Смотрел на OpenAPI - на расте получается намного выразительнее и понятнее.

Copy link
Member

@tyranron tyranron left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall impression is somewhat positive, but naming and little details should be bikeshedded, no doubts.

Please fill up information holes up marked by my comments. And I need a time to consume the overall thing to give a constructive feedback on the protocol and propose improvements.

docs/rfc/0002-webrc-client-api.md Outdated Show resolved Hide resolved
docs/rfc/0002-webrc-client-api.md Outdated Show resolved Hide resolved
docs/rfc/0002-webrc-client-api.md Show resolved Hide resolved
docs/rfc/0002-webrc-client-api.md Outdated Show resolved Hide resolved
in answer.

Probably, Server will always give his permission on any Client's request. This kind of request flow will allow Server
to do any request related stuff that Server needs to do, and distinguish between abnormal and normal events.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@alexlapa I think we're unable to control that. What if server rejects this, but client still disposes the peers? It's kinda weird state situation.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tyranron ,

Да, поэтому и говорю, что разрешать будем всегда. Но, мы хотим чтобы клиент подождал пока мы сделаем все что нужно.

Если клиент будет одновременно выключать пира и сообщать об этом - то не известно что до нас первое дойдет.

Copy link
Member

@tyranron tyranron left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Boom...

@tyranron
Copy link
Member

@alexlapa continuing out yesterday conversation...

I had time to think about clear separations between client/server methods and request/responses. Below is the description of such separation which I consider satisfying enough.

I've found that CQRS+ES terminology here plays quite well, so:

  1. All WS messages sent by server are called Events. Event means a fact that already has happened, so client cannot reject Event in any way (you cannot reject the happened past), it can only adopt itself to the received Events. So, server just notifies client about happened facts and clients reacts on them to make the proper state. This also emphasizes the indisputable authority of the server.
    The naming for Events follows the convention <entity><passive-verb>, for example: PeerCreated, TracksApplied, PeersRemoved.

  2. All WS messages sent by client are called Commands (or Cmds). Command is basically a request/desire/intention of client to change the state on server.
    The naming for Commands follows the convention <infinitive-verb><entity>, for example: SetTracks, MakeSdpOffer, MakeSdpAnswer.

  3. There is no acknowledgment on each WS message. No need to increase the network activity. No need to imply ids for WS messages. All the protocol logic is boiled into FSM (finite state machine) of the protocol, implemented on the server and on the client side. Generally:

    • Server sends Events and doesn't require acks of them, but due to protocol FSM it awaits certain Commands from client depending on the current state/phase. If client does not provide expected Commands, he is kicked off, and so on...
    • Client sends Commands to the server and awaits the Events from the server and changes its state by reaction on those Events.
  4. As the server rules all events it will/must take care about concurrent issues like unordered Events receiving and throw away expired Event if any.

@tyranron
Copy link
Member

Let's rewrite the "1 <=> 1 p2p with unpublish and republish" example in those terms:

  1. Server sends PeerCreated event to user1.
  2. user1 sends MakeSdpOffer command to server.
  3. Server sends PeerCreated event to user2, which contains user1's SDP Offer.
  4. user2 sends MakeSdpAnswer command to server.
  5. Server sends SdpAnswerMade event to user1 with user2's SDP Answer.
  6. user1 and user2 send SetIceCandidates commands to server.
  7. Server sends appropriate IceCandidatesDiscovered events to user1 and user2.
  8. At this point connection is supposed to be established.
  9. user1 wants to unpublish his tracks, so he sends RemoveTracks commands to the server.
  10. Server sends TracksRemoved event both to user1 and user2.
  11. user1 initiates SDP re-negotiation

@alexlapa
Copy link
Collaborator Author

alexlapa commented Dec 27, 2018

  1. 👍
  2. 👍
  3. Client sends Commands to the server and awaits the Events.

Думал о таком. Но, в итоге, возникает лишнее дублирование сообщений. Допустим, клиент хочет замьютить свое аудио. Для этого он бросит:

{
  "method": "SetTracks",
  "payload": {
    "peer_id": 1,
    "tracks": [{
      "id": 1,
      "media_type": {
        "Audio": {"muted":true}
      },
      "direction": {
        "Send": {
          "receivers": [2]
        }
      }
    }]
  }
}

Если у нас есть система запрос-ответ, то серверу достаточно ответить на этот запрос пустым сообщением, главное чтобы айдишка стояла та-же. Если айдишки убираем, то, фактически, сервер должен будет продублировать сообщение, что выглядит немного кривовато.

Попробуем без айдишек, но, не исключаю, что мы захотим их вернуть.

  1. 👍

@alexlapa
Copy link
Collaborator Author

@tyranron ,

Внес правки.

@tyranron tyranron force-pushed the 7-client-webrtc-api-rfc branch 7 times, most recently from 14ce77e to 9b77e60 Compare January 7, 2019 13:40
@tyranron tyranron force-pushed the 7-client-webrtc-api-rfc branch 2 times, most recently from 265124f to 6964007 Compare January 7, 2019 16:39
@tyranron
Copy link
Member

tyranron commented Jan 7, 2019

@alexlapa FCM:

Create client_webrtc_api RFC 0002 (#7)

@tyranron tyranron changed the title RFC: WebRTC signaling client API RFC: Client WebRTC API Jan 7, 2019
@alexlapa alexlapa merged commit 7a79e2a into master Jan 7, 2019
@alexlapa alexlapa deleted the 7-client-webrtc-api-rfc branch January 7, 2019 17:29
@alexlapa alexlapa added this to the 0.1.0 milestone Jan 7, 2019
@tyranron
Copy link
Member

tyranron commented Jan 8, 2019

@alexlapa be careful with FCM in the future. You've duplicated the title here. The first line of the FCM should come as title in commit message window on GitHub.

tyranron pushed a commit that referenced this pull request Jan 8, 2019
@tyranron
Copy link
Member

tyranron commented Jan 8, 2019

Fixed merge commit in 8c988496.

@alexlapa git pull --force origin master

@alexlapa alexlapa mentioned this pull request Jan 14, 2019
4 tasks
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
feature New feature or request k::api Related to API (application interface) k::design Related to overall design and/or architecture k::documentation Related to project documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants