Add Streaming Video support with example

[hthetiot]

• Implement networked-scene video options from component element
• Add enableVideo argument to connect method in NetworkConnection
• Implement EasyRtcAdapter video support and setLocalMediaStream and enableMicrophone like Janus Adapter.
• Implement basic-video example

Note: Camera from OBS Virtual Camera in screenshot.

[hthetiot]

Related: #260 #211 #18 #190

[vincentfretin]

Thanks @hthetiot for this contribution. I looked at it quickly, the changes are really great and reflect the discussion we had here. I'll take a second look later to a do a proper review.

[vincentfretin]

This a great first implementation. Thank you for contributing it. I left some comments.
I'm wanting to discuss further the video option here and how we can use setLocalMediaStream.
From my understanding, the mic and camera tracks are set by the open-easyrtc library and the stream acceptor will call setMediaStream with a stream object containing the tracks. And we will have an audio or video track depending on enableMicrophone and enableCamera.
So I'm not sure if we can use setLocalMediaStream here like we do in janus adapter where we get the mic stream ourself and call setLocalMediaStream and this will change the stream sent by the publisher RTCPeerConnection, see https://github.com/Synantoo/naf-janus-adapter/blob/a3fbd137bebf3e4d34b4ba11a9ce91a6985cd477/examples/index.html#L126-L130 and https://github.com/Synantoo/naf-janus-adapter/blob/261e1ca6040b74c4b90ba62762e0847ab8b76761/src/index.js#L914
This let you create your own ui to list available microphones, get the mic stream with getUserMedia and an exact deviceId match. In janus adapter setLocalMediaStream is using replaceTrack on the stream of the publisher RTCPeerConnection.
In your implementation, setLocalMediaStream doesn't seem to change anything on the RTCPeerConnection, so it seems that if you select another mic and call setLocalMediaStream, it will still send the audio track selected by open-easyrtc. Am I wrong?

[vincentfretin]

If we want to stream both camera and a screenshare, one need to call register3rdPartyLocalMediaStream and addStreamToCall like you said in #211 (comment)
Is there something that need to be done for the other participants on the receiving part? Do we have several videos tracks then in this.mediaStreams[clientId]["video"] automatically? Do we know if the order is kept? Are we sure first video is the camera and second video is a screenshare? What if we call enableCamera(false) and keep the screenshare?

[vincentfretin]

You can also add a link from index.html to your basic-video example.

[hthetiot]

You can also add a link from index.html to your basic-video example.

done

[hthetiot]

if we want to stream both camera and a screenshare, one need to call register3rdPartyLocalMediaStream and addStreamToCall like you said in #211 (comment)
Is there something that need to be done for the other participants on the receiving part? Do we have several videos tracks then in this.mediaStreams[clientId]["video"] automatically? Do we know if the order is kept? Are we sure first video is the camera and second video is a screenshare? What if we call enableCamera(false) and keep the screenshare?

Multi stream is more complicated, we need change on Adapters interface for that. Let keep that for another PR dedicated to multi-stream.

[hthetiot]

So I'm not sure if we can use setLocalMediaStream here like we do in janus adapter where we get the mic stream ourself and call setLocalMediaStream and this will change the stream sent by the publisher RTCPeerConnection, see https://github.com/Synantoo/naf-janus-adapter/blob/a3fbd137bebf3e4d34b4ba11a9ce91a6985cd477/examples/index.html#L126-L130 and https://github.com/Synantoo/naf-janus-adapter/blob/261e1ca6040b74c4b90ba62762e0847ab8b76761/src/index.js#L914
This let you create your own ui to list available microphones, get the mic stream with getUserMedia and an exact deviceId match. In janus adapter setLocalMediaStream is using replaceTrack on the stream of the publisher RTCPeerConnection.
In your implementation, setLocalMediaStream doesn't seem to change anything on the RTCPeerConnection, so it seems that if you select another mic and call setLocalMediaStream, it will still send the audio track selected by open-easyrtc. Am I wrong?

I would prefer to do that in separate PR, before or may be while doing multistream.

[hthetiot]

What if we call enableCamera(false) and keep the screenshare?

No but enableCamera will, again that for multi-stream next PR, will also do multi-stream sample when i do that anyway.

[vincentfretin]

I'm fine with doing more complex stuff in another PR. In this case I think you should remove the setLocalMediaStream api from this PR is it doesn't do what I expect.

[hthetiot]

I'm fine with doing more complex stuff in another PR. In this case I think you should remove the setLocalMediaStream api from this PR is it doesn't do what I expect.

Done, I will do the equivalent of setLocalMediaStream from janus using easyrtc in a separate PR later.

[vincentfretin]

The latest changes are good.
I tried running the example right now, I didn't until now.
I configured a certificate in server/easyrtc-server.js as documented in #257 and ran npm run dev
The basic-audio example works fine.
With your basic-video, I'm prompted for the camera permission, good, but I get the error in the console for Firefox
"MEDIA_ERR Failed to get access to local media. Error code was AbortError."
In Chrome I have no error, but in both browsers I don't see the element created for the other participant.
If now I set networked-scene="video:false", I see the white plane of the other participant. If now I call NAF.connection.adapter.enableCamera(true) is does nothing, no error.
I'll test again tomorrow after a reboot, maybe there is something weird with my pc.
But in any case, can we make sure we still fully connect if there is an error getting the camera?

You don't have to push the dist folder in the PR. I'll take care of pushing an up to date dist folder after the merge. You can rebase your branch to remove those "build dist" commits if you want. The npm run dev command take care of building dist/networked-aframe.js in memory via webpack-dev-middleware. And npm start will build them on the filesystem (just fixed an issue with that on master after I merged the "remove browserify" PR, it seems I didn't retested properly the latest commit I did on that, sorry)

[vincentfretin]

Nope, after a reboot I have the same issue I described. Is this working properly on your side?
Even if we don't do multi video streams in this PR, wouldn't it be good to have a mode where we share the screen/window/tab instead of the camera as part of this PR? We may think the api now before merging this. Maybe NAF.connection.adapter.enableVideo(true, type="camera") instead of NAF.connection.adapter.enableCamera(true)? And we could call NAF.connection.adapter.enableVideo(true, type="screen") to trigger screen sharing instead of the camera?

[hthetiot]

"MEDIA_ERR Failed to get access to local media. Error code was AbortError."

Occurs when another application use you camera on firefox.

Works for me with Chrome and Firefox.

NAF.connection.adapter.enableCamera(true)

Will require to call initMedia again, enableCamera and enableMicrophone can be use to disable media stream not create media stream.

You don't have to push the dist folder in the PR.

Ok noted.

You can rebase your branch to remove those "build dist" commits if you want.

I rather not please, merge and rebuild, next time i will not make dist.

The npm run dev command take care of building dist/networked-aframe.js in memory via webpack-dev-middleware.

I already use npm run dev, i commited dist because i saw it was done on other PR (e.g #259)

[hthetiot]

But in any case, can we make sure we still fully connect if there is an error getting the camera?

Works for me on MacOS FF, Chrome Safari and Windows on Chrome, Edge.
I never push unless it's tested, if you are on Linux you may have your own issues.

Did you git fetch to get latest version ?

[hthetiot]

Even if we don't do multi video streams in this PR, wouldn't it be good to have a mode where we share the screen/window/tab instead of the camera as part of this PR?

NO this is another feature. Trust me it's better to go step by steps.

[hthetiot]

NAF.connection.adapter.enableVideo(true, type="camera") instead of NAF.connection.adapter.enableCamera(true)? And we could call NAF.connection.adapter.enableVideo(true, type="screen") to trigger screen sharing instead of the camera?

That not what Janus adapter does.
Trust me the more you add the more you going to have problems to tests, and the current networked-aframe API need to be thought before doing multi-stream or screenshare. EasyRTC does not provide screen share it only expose register custom stream.

Please dont ask again, i dont like when people want feature A and then ask B and C when its not in the same scope and A is already fulfilling #260 #211 #18 #190 scope. Feature PR need to be progressive the more you add the more you have risk of breaking and bug, beside setLocalMediaStream this match janus adapter.

[hthetiot]

you need to provide what "networked-aframe" api will look like, you already want to remove audio: true, video: true

You dont realize that multi stream will require negotiation and special code when the caller have more than one stream due WebRTC bugs, we will need to send the first stream, then once establish send the nexts ones and negotiate.

Video is step one, then if you provide proper instruction for interface, like you did on #211 i may do more. But again mixing multi stream and video stream in one PR is a mistake, it better to iterate than making big PR that change everything for no good and have review that never ends.

<3

[hthetiot]

I used enableCamera to match janus adapter enableMicrophone logic. I'm open to change that minor.

[hthetiot]

Edge ok, but will result into error "MEDIA_ERR Failed to get access to local media. Error code was NotReadableError." if another browser use the camera, similar to linux AbortError. Check slack PM for private test URL i sent you.

[hthetiot]

Ok on Safari iOS 14.4 and Android Chrome and all combination Windows (Edge/Chrome <-> Safari <-> Firefox <-> Android <-> iOS.

[vincentfretin]

Accessing the camera twice on the same PC was effectively my issue, thanks for pointing that out. :)
So I tested a first participant joining with video:true (Firefox)
Then modified the file to set video:false (Chrome)

In Chrome I got
"Error play video stream DOMException: play() failed because the user didn't interact with the document first."
which is normal because it needs a user gesture.
If I write in the console
document.querySelectorAll('[networked-video-source]').forEach(e=>e.components['networked-video-source'].video.play())
I have the video, yeah.
Maybe using networked-scene="connectOnLoad:false" and adding a button to enter the room may fix the user gesture requirement. I'll try that this weekend.

If in Firefox, I do NAF.connection.adapter.enableCamera(false), it stops the stream, the plane goes black in Chrome, good.
If in Chrome, I run NAF.connection.adapter.enableCamera(true) nothing happen, the plane stays white on this other side, but you said that this normal because it doesn't create the stream, but I wrongly thought it would, but you're right it's like enableMicrophone it doesn't create the stream either. Hum I thought this.easyrtc.enableVideo(enabled); would create it, maybe it does? But I think the issue lies on the other participant that don't use again getMediaStream to set again the video stream on the video element. Ok we can look at it in another PR where we can add a button enable/disable cam for example and try to fix this use case. The simple example works as intended.

I tested video:true on Chrome and then just consuming video on Firefox, it worked right away.

I understand the multi streams videos is a complex subject. Here I was talking about screensharing in replacement of the camera, so not multistream, just one stream. But it seems you can't easily do that with open-easyrtc api, compared to setLocalMediaStream that the janus adapter api propose, noted.

I'm good to merge that then.

[hthetiot]

I understand the multi streams videos is a complex subject. Here I was talking about screensharing in replacement of the camera, so not multistream, just one stream. But it seems you can't easily do that with open-easyrtc api, compared to setLocalMediaStream that the janus adapter api propose, noted.

Yes, replacing stream is actualey a subset of multi-stream (Transceiver/Sender related). Still i can do setLocalMediaStream in one step then once the API for multistream is defined i can do Multi-stream.

It can be done with open-easyrtc today, my issue is more what the API will look like on networked-aframe to add a stream. may be it can simply be addLocalMediaStream(stream, streamName) and removeLocalMediaStream(stream, streamName) not sure what you d'like.

[hthetiot]

Hum I thought this.easyrtc.enableVideo(enabled); would create it, maybe it does?

No it should only disable if sending one depending the browsers involved it may freeze, get black or blank.