Multiple video streams for the easyrtc adapter #269

[hthetiot]

• add addLocalMediaStream, removeLocalMediaStream and update getMediaStream
• add streamName option to video and audio components
• No breaking change for previous API usage
• Add sample with screen stream on top of default video stream (camera).

[hthetiot]

One thing that can be interesting is to add a stream option in the networked-scene shema to use stream if provided instead of create one. alternatively calling addLocalMediaStream at any point before or after connection will works out of the box in this PR.

[vincentfretin]

Thanks. I'll do a proper review and some testing in the coming days.
There is a typo in the filename basic-mutli-stream.html => multi
Could you please add the new API addLocalMediaStream/removeLocalMediaStream and streamName field for the networked-video-source to the documentation in the README?

One thing that can be interesting is to add a stream option in the networked-scene shema to use stream if provided instead of create one

I didn't understand. The schema allows only primitive types I think, no? You can't set a stream on an entity field.
Why do you say "instead of creating one"? Which one are you talking about?

[hthetiot]

#294 (comment)

I agree, i did not want to change the API you instructed because I thought it had to match what other adapter does. I can change not problem.

[hthetiot]

There is a typo in the filename basic-mutli-stream.html => multi

Thx will fix.

Could you please add the new API addLocalMediaStream/removeLocalMediaStream and streamName field for the networked-video-source to the documentation in the README?

Ok will do.

[hthetiot]

I didn't understand. The schema allows only primitive types I think, no? You can't set a stream on an entity field.

I see, I did not know that.

Why do you say "instead of creating one"? Which one are you talking about?

Yes I thought user may want pass custom stream from schema i did not realised the primitive types limitation.
In any case user can use addLocalMediaStream at any point in time in the life cycle.

[vincentfretin]

#294 (comment)

I agree, i did not want to change the API you instructed because I thought it had to match what other adapter does. I can change not problem.

Ah no, no other adapter have this API. I didn't see it was me I specified this in the issue. I just didn't think much when I wrote the issue :)

[hthetiot]

Just need to add the doc and should be good, will do later today may be.

[vincentfretin]

Sorry for delay, I took the time only now to reconfigure https on the nodejs process, reconfigure my PC and test with two devices on the same network.

On the basic-video.html example I have a regression with these changes, on master this works well between Chrome Ubuntu and iPad 14.8 Safari
With the changes, the second participant only see a white screen for the other participant. No error in chrome console, on Safari I don't know I don't have easy access to the logs.

For the basic-multi-streams.html example, I think I succeeded only once on the iPad to see the two streams of the other participant. But after that I never saw again the second participant, on the PC on Chrome console I don't even see "Creating remote entity" so there is probably a js error on Safari, maybe randomly. I didn't dig further.

Do you reproduce the regression on basic-video.html?

I don't have much devices with me to test, I have currently only my PC on Ubuntu and iPad. My Android device refuse to connect on my hostname.local domain I don't know why.

I'll retest next weekend once the basic-video.html is working again properly for me probably after a fix in setMediaStream if you have time to look into it, see my comment above.
I'll also update to iPadOS 15 in the meantime, and try to use vorlon server to access the Safari logs.

[hthetiot]

Sorry for delay, I took the time only now to reconfigure https on the nodejs process, reconfigure my PC and test with two devices on the same network.

No problem, we all have job on the side or open source 🐱 , i will check when I have a moment myself. Thank you for the review and testing.

For https you should check ngrok for free https tunnel to localhost (npm i -g ngrok and ngrok http 8080).

[vincentfretin]

I updated my iPad to iPadOS 15.0.1, same issue with the regression.

With Android Chrome, I tested via the ip https://192.168.1.14:8080 instead of https://mymachine.local:8080 and accepted the certificate. Same issue with white screen on Android Chrome. It works on master.
I couldn't install ngrok, the server that host the archive currently times out.

[vincentfretin]

About your comment #269 (comment) about stream ended, I would prefer we implement something as part of this PR if possible. At least change the plane visible to false or to grey color when the stream ends instead of just freezing to the latest image. I'm afraid we will get some bug reports if we merge this PR and include that in the next release without a proper fix to handle stream ended.

[vincentfretin]

@hthetiot I copied your branch here https://github.com/networked-aframe/networked-aframe/commits/multi-streams and fixed the regression in 8025374
that was mainly because you changed this.mediaStreams[clientId] to be a Map (was an object before), but you didn't adapted getMediaStream so this.mediaStreams[clientId][streamName] there was undefined, it should have been this.mediaStreams[clientId].get(streamName). I reverted to using an object.
You can retrieve my commits in your branch if you want to continue to work on it.

[vincentfretin]

Remaining tasks before merging:

• update README
• hide plane if stream ended

If you don't have time, I'll do that next weekend.

[vincentfretin]

I merge this and create another PR with the changes in my branch.

[vincentfretin]

The example is now on https://naf-examples.glitch.me/basic-multi-streams.html