WebRTC frontend code tech debt and notes

[PVince81]

Some notes I took while trying to understand more in depth the various code paths in the src/utils/webrtc/* paths:

• [ ] redundant "joined users" collections:

  • joinedUsers in signaling.js
  • usersInCallMapping
  • callParticipantCollection
  • see also https://github.com/nextcloud/spreed/issues/2561

• [ ] signaling.js

  • seems signaling.js is mostly used by webrtc, so maybe they should simply work on the same models instead of considering them separate layers ?
  • [ ] signaling.js: this.roomCollection completely unused ??
  • [ ] sendVideoIfAvailable is unrelated to signaling but this class is used as a container by webrtc for this seemingly global attribute

• [ ] webrtc/*

  • [ ] file too big, split into smaller logical units ?

    • maybe move some to the participant models / collections ?
    • participant vs connection vs peer vs peer stream vs tracks vs data channels... (single responsibility principle if applicable ?)

  • [ ] checkPeerMedia and related

    • use async/await for better readability ?
    • setInterval for X+ participants might be expensive ?? use a single interval to check all streams at once ?
    • setInterval might become troublesome if the inside async functions take more than 1s to run like getStats()
    • should we move this logic inside a separate handler ? (detecting mute / black videos)

  • [ ] "webrtc.webrtc.on()" we better not use the inner object but forward the events, reading this inside a file called webrtc.js itself is already confusing enough...

  • [ ] encapsulation

    • [ ] don't access localStorage from inside the webrtc classes, rely on external events/providers/vuex.store, or set this initially when starting the call...
    • [ ] don't trigger toast notifications directly from inside the webrtc code: trigger events to the outside
    • [ ] LocalParticipantModel must not import store!
    • [ ] don't bind webrtc stuff inside LocalParticipantModel but do the event connections outside

  • [ ] use mixin for those repeated on/off functions, use WildEmitter everywhere ?

  • [ ] don't use events for triggering actions

    • like the "sendToAll" event, it should be a method. There might be others.
    • webrtc.emit('createdPeer') ?

  • [ ] unit tests for MediaDevicesManager ?

• [ ] LocalMediaControls.vue

  • [ ] don't access inner WebRTC object through the model, add methods on the model to query what we need (ex: see videoButtonTooltip)

• general

  • many ways to "do the same things" and many workarounds for things that don't always happen automatically
    • mcu vs no mcu logic
  • complex state management / accounting:
    • across multiple files: ICE connection states vs peer added states vs streams added states vs video/audio mute/unmute. Not sure if this can be consolidated in a more readable way.
    • need to create/send existing video/audio to newly added peers
    • need to accept connection from existing peers when joining a call with people already there, need initial state from everyone, etc
  • confusing way of sending messages around: signaling messages vs data channel messages vs various events across multiple layers of the webrtc objects
  • seems SimpleWebRTC is incomplete as we still need to do things on top in some scenarios: for example starting a screen share requires us to manually create a screen sharing peer with different logic whether mcu or !mcu

[nickvergessen]

✨

[PVince81]

sparkles

more like :face_with_head_bandage:

:wink:

[PVince81]

• [ ] ParticipantAnalyzer is only used for the local call model ??
  • [ ] setReceiverParticipant never used, remove ?
  • [ ] setSenderReceiverParticipant never used, remove ?

[PVince81]

• [ ] CallParticipantCollection is too generic: we could extend it to report more useful information like number of video and audio streams enabled and trigger events when those states change. Currently the latter logic is required in various places like the call analyzer, so it could be moved here.

[PVince81]

• [ ] are the MediaStream, etc shims still needed for recently supported browser versions ?

• [ ] getscreenmedia:

  • [ ] remove the shims and workarounds from the times where we still needed a chrome extension ? (everything < chrome 49)
  • [ ] cefGetScreenMedia ??
  • [ ] remove workarounds for Firefox < 52
  • [ ] some polling workaround in Firefox > 52 ?? not clear in what Firefox version this got fixed... https://bugzilla.mozilla.org/show_bug.cgi?id=1208373. Probably still needed because of https://bugzilla.mozilla.org/show_bug.cgi?id=1317872 but I'd recommend increasing the polling time to 2-3 seconds.

[PVince81]

• [ ] simplewebrtc/localmedia.js:

  • [ ] missing description doc at the top
  • [ ] depends on ../../index.js !!!
  • [ ] too much juggling with tracks when video or audio id changed, this needs to be made more straightforward/readable
    • cloned streams, monitor stream, etc
    • this file seems to contain the key to be able to disable the camera device completly but I failed to do so back then due to all the required juggling, see https://github.com/nextcloud/spreed/issues/4008#issuecomment-706185774

• [ ] simplewebrtc/peer.js:

  • wraps around RTCPeerConnection
  • handles ICE logic, SDP handshakes and codec sorting for negotiation
  • handles sending peer messages through signaling or data channels
  • [ ] missing description at top
  • [ ] another isAllTracksEnded like in localmedia => move to common utility functions
  • [ ] send() is emitting message event to parent for sending through signaling. This feels mixed up, maybe we should inject the signaling class instance here and call method on it instead of firing events
  • [ ] some browser specific handling (moz prefix, etc), obsolete ?

[PVince81]

• [ ] confusion peerId vs sessionId as we compare them in some places
• [ ] adding custom messages (like raisedHand back then) requires adding in multiple places. Would be good to have a more generic place for those. We'll need this for future work like ask to unmute

[PVince81]

• [ ] look into using https://github.com/webrtcHacks/adapter for shimming: that lib was recommended in https://developer.mozilla.org/en-US/docs/Web/API/WebRTC_API (it also seems to normalize statistics)