New WebRTC #151

joaoantoniocardoso · 2022-12-22T14:28:03Z

rafaellehmkuhl · 2022-12-23T16:30:26Z

src/composables/webRTC.ts

+
+          // Regularly asks for available streams, which will trigger the consumer "on_available_streams" callback
+          let handler_id: number | undefined = undefined
+          handler_id = window.setInterval(() => {


Will this interval be ended if the component is removed?

great one, interval's life is independent, right? So it will never be collected if we don't clear it... Any suggestion on how to do it properly?

actually, probably this setInterval will prevent its parent object (this) from being collected, right? 🤦

Similarly, maybe all those callbacks (e.g. on_track_added in Session) might need to be set to undefined too for the Session to be collected..? ..Or are they something like weak references and they don't count for the garbage collection thing? @patrickelectric

If I remember correctly, the parent will be collected, but the interval will keep running (the former I'm sure).

After some research and reasoning, my understanding is the following:

this will only be collectible if this arrow function is collectible.

the interval arrow function will only be collectible if id is nullified.

id is not being nullified, even when this ends

To this point, id is never nullified, thus, neither the interval arrow function nor this will be ever collected.

To enable the interval arrow function to be collectible, we might need to:

set id = undefined after every clearInterval(id)

create another condition to make sure that:

if this ends, the interval will be cleared and the id will be nullified.

The only ways I see to make it possible is to:

make the id a private property of this and nullify it inside its end() method.

create a private boolean property, like ended, set to false in its end(), and use it inside the arrow function to nullify the id and clear the interval, which seems better

rafaellehmkuhl · 2022-12-23T16:30:46Z

src/libs/webrtc/session.ts

+   */
+  private onTrackAdded(event: RTCTrackEvent): void {
+    let id: number | undefined = undefined
+    id = window.setInterval(() => {


Will this interval be ended if the component is removed?

patrickelectric

That's not a full review, it's just a tribute.

patrickelectric · 2023-01-04T17:50:25Z

src/composables/webRTC.ts

-  const addAvailablePeer = (peerId: string, displayName: string): void => {
-    removeAvailablePeer(peerId)
-    availablePeers.value.push({ id: peerId, displayName })
+  const consumer_id: Ref<string | undefined> = ref()


We avoid snake_case when possible.

patrickelectric · 2023-01-04T17:50:28Z

src/composables/webRTC.ts

+  const rtc_configuration: RTCConfiguration = { iceServers: [] }
+  const status: Ref<string> = ref('waiting...')
+
+  const updateStatus = (new_status: string): void => {


We avoid snake_case when possible.

patrickelectric · 2023-01-04T17:51:29Z

src/composables/webRTC.ts

+
+  const updateStatus = (new_status: string): void => {
+    status.value = new_status
+    console.log(`Status updated: ${new_status}`)


You probably want to add a prefix for the message like webRTC: or something.
That would be more a console.debug than console.log

patrickelectric · 2023-01-04T19:21:45Z

src/composables/webRTC.ts

+              clearInterval(handler_id)
+            }
+
+            signaller.requestStreams()


Why this need to be inside the timer inside of a websocket connection ?
Can't we put it totally outside ? That should fix the clearInterval issue.

patrickelectric · 2023-01-04T19:22:56Z

src/composables/webRTC.ts

+      },
+      { once: true }
+    )
+  }


Making this a singleton component that will be accessible by video widgets would fix the unmount logic

patrickelectric · 2023-01-04T19:48:39Z

src/composables/webRTC.ts

-      console.warn(`Socket for session '${sessionId.value}' already undefined.`)
-      return
+  const startSession = (producer_id: string): void => {
+    {


Should we check consumer_id before the playing message ?

patrickelectric · 2023-01-04T19:49:15Z

src/composables/webRTC.ts

-    if (sessionWebSocket.value === undefined) {
-      resetSocket()
+    const stream = available_streams.value.find((s) => s.id === producer_id)
+    console.debug(`stream: ${stream}`)


This debug message should be after the if check, the if check message will already provide the necessary information if the value is undefined.

src/libs/webrtc/session.ts

joaoantoniocardoso · 2023-01-31T02:31:53Z

@rafaellehmkuhl @patrickelectric there are two (similar) cases where it's not behaving as expected:

When the cockpit is opened before the mavlink-camera-manager, the signalling connects as soon as the mavlink-camera-manager goes up, but the stream is never negotiated.
When the cockpit is opened and the mavlink-camera-manager restarts, the signalling connects as soon as the mavlink-camera-manager goes up, but the stream is never re-negotiated.

patrickelectric · 2023-01-31T12:37:28Z

src/components/widgets/VideoPlayer.vue

+  }
+
+  if (selectedStream.value === undefined) {
+    console.debug!('[WebRTC] trying to set stream...')


Does it make sense to not return if the selectedStream is undefined ?

Yes, regardless of selectedStream being undefined, when we get a defined savedStream different from selectedStream, we want selectedStream to receive the value of the found savedStream.

This should be enough to guarantee that the saved stream is synched with the available streams and that the selected stream is only be updated to a valid stream. We could, however, want the selectedStream to go back to 'undefined' when we don't find the saved stream in the list of available streams. This change would make the combobox be shown as empty when the saved stream is not available.

patrickelectric · 2023-01-31T12:45:22Z

src/composables/webRTC.ts

+      if (oldStream !== undefined) {
+        this.stopSession(msg)
+      }
+      if (newStream !== undefined && newStream !== oldStream) {


Suggested change

if (newStream !== undefined && newStream !== oldStream) {

if (newStream !== undefined) {

The first if of the logic takes care of it

patrickelectric · 2023-01-31T12:46:34Z

src/composables/webRTC.ts

+   * @param {RTCTrackEvent} event
+   * @returns {boolean}
+   */
+  private onTrackAdded(event: RTCTrackEvent): boolean {


Does it make sense to return a value at all ? If its always true

patrickelectric · 2023-01-31T12:51:19Z

src/composables/webRTC.ts

+    this.signaller.requestSessionId(
+      this.consumerId,
+      producerId,
+      (receivedSessionId: string): void => {


isn't better to break it on own its function over a huge lambda ?

src/composables/webRTC.ts

patrickelectric · 2023-01-31T13:08:28Z

src/libs/webrtc/session.ts

+   * Defines the behavior for the RTCPeerConnection's 'onconnectionstatechange' event
+   */
+  private onConnectionStateChange(): void {
+    switch (this.peerConnection.connectionState) {


You should save the state as a variable inside this function to be used for msgs when necessary, avoiding the long property path

patrickelectric · 2023-01-31T13:08:46Z

src/libs/webrtc/session.ts

+        const msg = `RTCPeerConnection state changed to ${this.peerConnection.connectionState}`
+        console.debug('[WebRTC] [Session] ' + msg)
+
+        if (this.peerConnection.connectionState == 'failed') {


This should never happen since state will be disconnected

src/libs/webrtc/session.ts

patrickelectric · 2023-01-31T13:11:34Z

src/libs/webrtc/session.ts

+    // Cleanup event listeners
+    this.peerConnection.removeEventListener('track', this.onTrackAddedCallback.bind(this))
+    this.peerConnection.removeEventListener('icecandidate', this.onIceCandidate.bind(this))
+    this.peerConnection.removeEventListener('iceconnectionstatechange', this.onIceConnectionStateChange.bind(this))
+    this.peerConnection.removeEventListener('connectionstatechange', this.onConnectionStateChange.bind(this))
+    this.peerConnection.removeEventListener('signalingstatechange', this.onSignalingStateChange.bind(this))
+    this.peerConnection.removeEventListener('icegatheringstatechange', this.onIceGatheringStateChange.bind(this))


You should encapsulate this logic under a function and use it in end and reconnected

rafaellehmkuhl · 2023-01-31T14:45:35Z

Joao, I'm not able to list streams. Is this because I don't have streams available? Should I create an UDP stream so a webRTC one is created?

PS: I have created the following stream and yet I don't have webRTC streams available.

joaoantoniocardoso · 2023-01-31T15:00:40Z

Joao, I'm not able to list streams. Is this because I don't have streams available? Should I create an UDP stream so a webRTC one is created?

PS: I have created the following stream and yet I don't have webRTC streams available.

Yes, you need to create a stream, but it needs to be an H264 encoded one. I advise RTSP over UDP 'cause RTSP only uses resources when has a client connected to it, as opposed to UDP, which is always sending packages.

patrickelectric · 2023-01-31T15:04:31Z

Use the reset video settings on BlueOS

…

On Tue, Jan 31, 2023, 12:00 João Antônio Cardoso ***@***.***> wrote: Joao, I'm not able to list streams. Is this because I don't have streams available? Should I create an UDP stream so a webRTC one is created? [image: image] <https://user-images.githubusercontent.com/6551040/215792152-55d361bd-0922-4b10-b6d1-efe0ea68109c.png> PS: I have created the following stream and yet I don't have webRTC streams available. [image: image] <https://user-images.githubusercontent.com/6551040/215792637-129cd9ac-8b39-488e-b494-5ce3a8ff0b63.png> Yes, you need to create a stream, but it needs to be an H264 encoded one — Reply to this email directly, view it on GitHub <#151 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAJIYCISXLO7AJPKYKQILS3WVESKHANCNFSM6AAAAAATGZCDIQ> . You are receiving this because you were mentioned.Message ID: ***@***.***>

rafaellehmkuhl · 2023-01-31T15:05:22Z

Joao, I'm not able to list streams. Is this because I don't have streams available? Should I create an UDP stream so a webRTC one is created?

PS: I have created the following stream and yet I don't have webRTC streams available.

Yes, you need to create a stream, but it needs to be an H264 encoded one. I advise RTSP over UDP 'cause RTSP only uses resources when has a client connected to it, as opposed to UDP, which is always sending packages.

Ok. Don't know if a problem with mavlink-camera-manager or this PR, but the stream (UDP) appears, but once connected, give a blank screen. Also, on BlueOS it stays forever on "fetching thumbnail...".

patrickelectric · 2023-01-31T15:07:46Z

Is the thumbnail normal ? I'm having the same problem on BlueOS when using my mac

rafaellehmkuhl · 2023-01-31T15:11:04Z

Joao, I'm not able to list streams. Is this because I don't have streams available? Should I create an UDP stream so a webRTC one is created?

PS: I have created the following stream and yet I don't have webRTC streams available.

Yes, you need to create a stream, but it needs to be an H264 encoded one. I advise RTSP over UDP 'cause RTSP only uses resources when has a client connected to it, as opposed to UDP, which is always sending packages.

Ok. Don't know if a problem with mavlink-camera-manager or this PR, but the stream (UDP) appears, but once connected, give a blank screen. Also, on BlueOS it stays forever on "fetching thumbnail...".

RTSP worked flawlessly.

rafaellehmkuhl · 2023-01-31T15:11:59Z

Is the thumbnail normal ? I'm having the same problem on BlueOS when using my mac

No. H264 + UDP gives me no thumbnail and blank stream.

joaoantoniocardoso · 2023-02-07T21:41:52Z

I didn't know, but:

Is it better to remove the 'autoplay' from the video object?

rafaellehmkuhl · 2023-02-07T22:20:35Z

I didn't know, but:

Is it better to remove the 'autoplay' from the video object?

Also new to me.

Apparently if the source has no audio, we can autoplay.

So just add a muted attribute to the video widget and it will work.

rafaellehmkuhl · 2023-02-07T22:22:24Z

I didn't know, but:
Is it better to remove the 'autoplay' from the video object?

Also new to me.

Apparently if the source has no audio, we can autoplay.

So just add a muted attribute to the video widget and it will work.

Strange, there is one already. You shouldn't be receiving this warn...

rafaellehmkuhl · 2023-02-07T22:24:36Z

I didn't know, but:
Is it better to remove the 'autoplay' from the video object?

Also new to me.
Apparently if the source has no audio, we can autoplay.
So just add a muted attribute to the video widget and it will work.

Strange, there is one already. You shouldn't be receiving this warn...

Apparently this changes from browser to browser.

joaoantoniocardoso · 2023-02-08T02:56:13Z

I didn't know, but:
Is it better to remove the 'autoplay' from the video object?

Also new to me.
Apparently if the source has no audio, we can autoplay.
So just add a muted attribute to the video widget and it will work.

Strange, there is one already. You shouldn't be receiving this warn...

Apparently this changes from browser to browser.

Yeah, I didn't get why Firefox is complaining, maybe there's a hidden related to WebRTC. We can check if it can autoplay, and create a popup asking the user to click a button that will call the play. Probably in another PR hehe

joaoantoniocardoso · 2023-02-10T00:32:02Z

@rafaellehmkuhl @patrickelectric there are two (similar) cases where it's not behaving as expected:

1. When the cockpit is opened before the mavlink-camera-manager, the signalling connects as soon as the mavlink-camera-manager goes up, but the stream is never negotiated.

2. When the cockpit is opened and the mavlink-camera-manager restarts, the signalling connects as soon as the mavlink-camera-manager goes up, but the stream is never re-negotiated.

After some investigation, the (1) is on the signalling server side, and (2) was finally solved here :)

patrickelectric · 2023-02-10T01:02:10Z

src/libs/webrtc/signalling_protocol.d.ts

+export type Message =
+  | {
+      /**
+       *


This empty comments are a sad thing to see

Yeah, one day, maybe..: Aleph-Alpha/ts-rs#52

patrickelectric

We can add the necessary permission request later

patrickelectric · 2023-02-10T16:51:13Z

src/assets/defaults.ts

+  bundlePolicy: 'max-bundle',
+  iceServers: [
+    {
+      urls: `turn:blueos.local:3478`,


We should use the same address as the network configuration of cockpit

Or at least allow configuring it somehow

We should not have addresses in default.ts, the mavlink one should also be contemplated by the solution. Do you think this should be handled in this PR?

OH, it already has a configuration. I'll do that for these :D

So just to be sure, you will take the same address from the mainVehicle store and parse it, right?

So just to be sure, you will take the same address from the mainVehicle store and parse it, right?

Nope, I'm modifying the configuration page to allow a global address (defaulted to blueos.local) to build both mainVehicle and webRTC addresses, and also making both customizable:

Perfect. Was going to suggest that haha

Williangalvani · 2023-02-14T17:30:41Z

fixes #209

joaoantoniocardoso · 2023-02-15T15:52:45Z

Well, it's merged, but we still need to make those custom ips/addresses permanent (saved in the store) and reactive. @rafaellehmkuhl is helping me with it

patrickelectric · 2023-02-15T15:57:43Z

Well, it's merged, but we still need to make those custom ips/addresses permanent (saved in the store) and reactive. @rafaellehmkuhl is helping me with it

Yes, I saw that, but merged to get a release since this fixes should not hold the test release.

joaoantoniocardoso requested review from patrickelectric and rafaellehmkuhl December 22, 2022 14:28

rafaellehmkuhl requested changes Dec 23, 2022

View reviewed changes

patrickelectric requested changes Jan 5, 2023

View reviewed changes

joaoantoniocardoso requested review from patrickelectric and rafaellehmkuhl January 31, 2023 02:27

joaoantoniocardoso marked this pull request as ready for review January 31, 2023 02:27

patrickelectric requested changes Jan 31, 2023

View reviewed changes

joaoantoniocardoso requested a review from patrickelectric February 10, 2023 00:32

patrickelectric reviewed Feb 10, 2023

View reviewed changes

joaoantoniocardoso requested a review from patrickelectric February 10, 2023 16:47

patrickelectric requested changes Feb 10, 2023

View reviewed changes

joaoantoniocardoso added 3 commits February 15, 2023 00:04

yarn: add webrtc-adapter dependency

d7d9e89

src: Add new WebRTC

228d557

src: Create global vehicle address, while allowing custom addresses

0c02d8f

patrickelectric approved these changes Feb 15, 2023

View reviewed changes

patrickelectric merged commit ffb51a3 into bluerobotics:master Feb 15, 2023

Williangalvani mentioned this pull request Feb 15, 2023

Video stream should be automatically selected when it becomes available #209

Closed

	if (newStream !== undefined && newStream !== oldStream) {
	if (newStream !== undefined) {

New WebRTC #151

New WebRTC #151

Conversation

joaoantoniocardoso commented Dec 22, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joaoantoniocardoso Dec 23, 2022 • edited Loading

Choose a reason for hiding this comment

joaoantoniocardoso Dec 23, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joaoantoniocardoso Dec 23, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

patrickelectric left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joaoantoniocardoso commented Jan 31, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rafaellehmkuhl commented Jan 31, 2023 • edited Loading

joaoantoniocardoso commented Jan 31, 2023 • edited Loading

patrickelectric commented Jan 31, 2023 via email

rafaellehmkuhl commented Jan 31, 2023

patrickelectric commented Jan 31, 2023

rafaellehmkuhl commented Jan 31, 2023

rafaellehmkuhl commented Jan 31, 2023

joaoantoniocardoso commented Feb 7, 2023 • edited Loading

rafaellehmkuhl commented Feb 7, 2023 • edited Loading

rafaellehmkuhl commented Feb 7, 2023

rafaellehmkuhl commented Feb 7, 2023

joaoantoniocardoso commented Feb 8, 2023 • edited Loading

joaoantoniocardoso commented Feb 10, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

patrickelectric left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joaoantoniocardoso Feb 10, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joaoantoniocardoso Feb 13, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Williangalvani commented Feb 14, 2023

joaoantoniocardoso commented Feb 15, 2023

patrickelectric commented Feb 15, 2023

joaoantoniocardoso commented Dec 22, 2022 •

edited

Loading

joaoantoniocardoso Dec 23, 2022 •

edited

Loading

joaoantoniocardoso Dec 23, 2022 •

edited

Loading

joaoantoniocardoso Dec 23, 2022 •

edited

Loading

joaoantoniocardoso commented Jan 31, 2023 •

edited

Loading

rafaellehmkuhl commented Jan 31, 2023 •

edited

Loading

joaoantoniocardoso commented Jan 31, 2023 •

edited

Loading

joaoantoniocardoso commented Feb 7, 2023 •

edited

Loading

rafaellehmkuhl commented Feb 7, 2023 •

edited

Loading

joaoantoniocardoso commented Feb 8, 2023 •

edited

Loading

joaoantoniocardoso commented Feb 10, 2023 •

edited

Loading

joaoantoniocardoso Feb 10, 2023 •

edited

Loading

joaoantoniocardoso Feb 13, 2023 •

edited

Loading