Video store rules all #654
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…same stream source
|
Tested, the only thing I found was this:
|
Good catch! |
rafaellehmkuhl
force-pushed
the
video-store-rules-all
branch
from
9fceac3
to
366fefe
Compare
Fixed. |
rafaellehmkuhl
force-pushed
the
video-store-rules-all
branch
from
366fefe
to
fa31936
Compare
|
I forgot to test the recording widget stuff. I'll make the review tomorrow =) |
It initially has the configuration of the allowed ICE IPs.
We don't have Secure Context right now, so it does not have value.
rafaellehmkuhl
force-pushed
the
video-store-rules-all
branch
from
fa31936
to
68db7a1
Compare
rafaellehmkuhl
force-pushed
the
video-store-rules-all
branch
from
68db7a1
to
699f90b
Compare
patrickelectric
approved these changes
Dec 22, 2023
|
Thanks for the quick approval @patrickelectric! I will only wait to see if there's something from @joaoantoniocardoso before we merge it. @ES-Alexander I'm going to merge it without the docs changes as it's a critical bug fix and the changes on docs are small. |
joaoantoniocardoso
approved these changes
Dec 22, 2023
There was a problem hiding this comment.
Tested. All seems fine!
It also helps #633, fixing the error "2".
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
With this patch, the video store begins being responsible for internally routing available video streams.
Screen.Recording.2023-12-15.at.17.59.36.mov
Instead of having each video stream consumer (e.g.: VideoPlayer, MiniVideoRecorder) taking full control of the stream it is consuming, we delegate that to the video store, with the widget solely requesting (to the store) the stream it needs. The video store them checks if the requested stream is already being consumed, and if so, routes the same stream, instead of opening a new one with the WebRTC server, which was causing unnecessary usage of both bandwidth and computer resources, to the point of causing system failures or video stuttering.
In resume, with this patch one can add as many widgets as it wants for a same stream and only one connecting will be made to consume it.
I explicitly followed a non-reactive interval-based approach, as I'm not fully confident on the reactive behavior of the WebRTC composable yet. I think we still have an opportunity in the future to improve this code, making it completely reactive, but as it is the performance footprint is very small, and we benefit from having this patch sooner, fixing a critical problem being experienced by our beta testers.
Fix #360
Fix #602
Helps #633.
PS: