Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add feedback on video connection #890

Merged
merged 13 commits into from Apr 19, 2024
Merged

Add feedback on video connection #890

merged 13 commits into from Apr 19, 2024

Conversation

rafaellehmkuhl
Copy link
Member

@rafaellehmkuhl rafaellehmkuhl commented Apr 16, 2024 •
edited

This patch adds status for the signalling server and the stream connection on the video widget.
It also fixes some minors.

There's more to come, but I didn't want to hold bigger changes to get the fix in.

image

Fix #664
Fix #684
Fix #813

@rafaellehmkuhl rafaellehmkuhl force-pushed the video-connection-more-feedback-start branch from 0afdcca to df7c533 Compare April 17, 2024 14:15
@joaoantoniocardoso
Copy link
Contributor

joaoantoniocardoso commented Apr 18, 2024 •
edited

TODO:

  • Once the connection is dropped (for example, if you kill the MCM or disconnect the ethernet cable), the video turns black and the user gets no feedback about what is happening.
  • When ICE fails (for example, if there's no internet connection and the allowed IPs don't include any reliable route), the video is forever in 'loading...', providing no feedback that the Connection failed (possibly noticed by connectionState). In this case, it should warn the user to check the allowed IPs and/or keep trying.

@rafaellehmkuhl
Copy link
Member Author

TODO:

  • Once the connection is dropped (for example, if you kill the MCM or disconnect the ethernet cable), the video turns black and the user gets no feedback about what is happening.
  • When ICE fails (for example, if there's no internet connection and the allowed IPs don't include any reliable route), the video is forever in 'loading...', providing no feedback that the Connection failed (possibly noticed by connectionState). In this case, it should warn the user to check the allowed IPs and/or keep trying.
  1. The first will be fixed with If stream is removed and then added back, it does not work (black screen) #775.
  2. In this case the server/stream status is not showing that information?

@joaoantoniocardoso
Copy link
Contributor 

2. In this case the server/stream status is not showing that information?

Nope, the only text being displayed is "loading..."

@rafaellehmkuhl
webRTC: Remove redundant check around available streams answer
46b233f 
There's no way for this check to be false.
@rafaellehmkuhl
webrtc: signaller: Remove unnecessary log of callback codebase.
0244c36
@rafaellehmkuhl
webrtc: Make availableStreams public and consume directly from it
bdc5de8
@rafaellehmkuhl
webrtc: Stop returning availableStreams and availableIceIps on st…
8cdc94c 
…artStream`

They are public values that are not stream specific, and can be consumed directly.
@rafaellehmkuhl
webrtc: Expose server and stream status
4841e0c
@rafaellehmkuhl
video-player: Add status of server and stream while the video is not …
a411573 
…loaded
@rafaellehmkuhl
video-player: Add "loading" state for when stream is connected but no…
86c7632 
…t playing
@rafaellehmkuhl
video-player: Differentiate between no available streams and selected…
837864c 
… stream not available
@rafaellehmkuhl
webrtc: signaller: Remove unused method
d08909e
@rafaellehmkuhl
webrtc: signaller: Concentrate all status changes on one callback
891fe13
@rafaellehmkuhl
webrtc: session: Allow Session object to add updates to the stream …
767ef2a 
…status
@rafaellehmkuhl
webrtc: session: Add status updates to session class
690b169
@rafaellehmkuhl rafaellehmkuhl force-pushed the video-connection-more-feedback-start branch from df7c533 to 690b169 Compare April 18, 2024 19:21
@rafaellehmkuhl rafaellehmkuhl force-pushed the video-connection-more-feedback-start branch from 5e12136 to 4d1e2ef Compare April 18, 2024 20:18
@rafaellehmkuhl
Copy link
Member Author 

2. In this case the server/stream status is not showing that information?

Nope, the only text being displayed is "loading..."

Fixed! Thanks for the tip!

image

@rafaellehmkuhl rafaellehmkuhl merged commit 1ff3a4d into bluerobotics:master Apr 19, 2024
8 checks passed
@rafaellehmkuhl rafaellehmkuhl deleted the video-connection-more-feedback-start branch April 19, 2024 20:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@joaoantoniocardoso joaoantoniocardoso joaoantoniocardoso approved these changes

@patrickelectric patrickelectric Awaiting requested review from patrickelectric

@ArturoManzoli ArturoManzoli Awaiting requested review from ArturoManzoli

@JoaoMario109 JoaoMario109 Awaiting requested review from JoaoMario109

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
2 participants
@rafaellehmkuhl @joaoantoniocardoso