search

pfertyk / webrtc-working-example

Tested on local and public network.
The Unlicense
154 stars 62 forks source link

Replace deprecated pc.onaddstream callback with pc.ontrack #22

Closed AndrewMeadows closed 2 years ago

AndrewMeadows commented 3 years ago

This webrtc-working-example project helped me learn how to use webrtc. While playing with it I discovered two issues that seemed worth fixing, hence this PR:

(1) RTCPeerConnection.onaddstream callback has been deprecated; we are to use RTCPeerConnection.ontrack instead (2) The socket.io.js was old and would not work with a fresh python-socketio package, so I updated it to v4.2.0

pfertyk commented 3 years ago

Hi!

Thank you very much for your contribution! I'm glad to hear that this project helps people :)

I didn't know that onaddstream is deprecated, definitely a good idea to upgrade it! However, there might be an issue with socket.io.js. I've specifically chosen an old version, because there were some problems with any newer one. They happened in the web or in the mobile version (I remember React Native logging a lot of warnings, but I can't recall the details). The solution was to downgrade socket.io.js.

This project's purpose is to help people get started with WebRTC as smoothly as possible, so it cannot have any warnings/errors logged immediately after it starts. Therefore, before I merge this PR, I'll have to test it with the new version of React Native to check if the issues are fixed now. This might take me a moment, as I'm busy with other projects, but I'll get to it eventually :)

Thank you again for making this project better!

pfertyk commented 3 years ago

Hi @AndrewMeadows ! I've tested your branch today and there was an issue with the new socket.io library. It didn't work in the browser and on mobile. The version used in master worked though. Could you please check it on your machine as well?

Chances are that after some minor changes in the rest of the code we can actually make it work. I'll also try to investigate this, as soon as I find more time ;)

AndrewMeadows commented 3 years ago

@pfertyk I rebased the branch and removed the changed socket.io.js file. Alas, the version in master does not work for me because when I run the signal server it complains about incompatible (too old) socket.io version. That is, it does not agree with the python socket.io package I obtain by default via pip. I'm running Ubuntu-20.04 with python-3.8.10.

Thanks for the working example. I modified your the signal server python script to help build a mock-SFU server/client system I'm using to test some insertable-stream experiments.

pfertyk commented 2 years ago

Ah, I see, I was running the dockerized version with Ubuntu 18. I'll check how it behaves after an update ;)

pfertyk commented 2 years ago

Hi @AndrewMeadows , sorry for a long break, but I got good news :) I've tested socket.io 4.2 with the mobile application, and it worked! I had to, however, recreate the application using the new version of React Native, WebRTC and other modules, so there is going to be a lot of changes ;) I also upgraded Docker images and Python dependencies, so that everything works on signaling/web side as well. Once I clean everything up, I will be able to merge this PR. Thank you very much for contributing!

If it's not a problem, could you please bring back socket.io 4.2 to your branch? I can also do it on my own, but it was your idea, so I want you to take the credit :)

pfertyk commented 2 years ago

I've merged this PR and updated the remaining files :) The changes are now available in v1.1.0. Thank you very much for your contribution @AndrewMeadows , without you this repo would rely on socket.io 2.2 for a very long time :D