Automatically manage remote video track layer#77
Conversation
|
Excellent job @davidzhao |
| // override id to parsed ID | ||
| this.sid = sid; | ||
| this.receiver = receiver; | ||
| this.intersectionObserver = new IntersectionObserver(this.handleVisibilityChanged); |
There was a problem hiding this comment.
Instead of creating an own IntersectionObserver for each RemoteVideoTrack, it would probably be more performant to use a shared IntersectionObserver (maybe on Room level or as a singleton). I'm not sure how big the impact would be for a couple of video elements and if it's worth it, but when implementing lazy-loading where potentially hundreds of image elements get observed, the difference becomes noticable. (https://www.bennadel.com/blog/3954-intersectionobserver-api-performance-many-vs-shared-in-angular-11-0-5.htm)
The same might be true for ResizeObserver.
There was a problem hiding this comment.
@lukasIO this is a great insight! thanks for sharing it.
I'll modify this to use a shared instance instead.
There was a problem hiding this comment.
actually reading this a bit more. this is quite complicated to share an instance.. because currently it's nicely scoped to a single RemoteVideoTrack. from the post, the author wasn't sure the source of the speedup with sharing a single instance, and suspected it could be angular.
it could be that the performance bottleneck is actually the Angular template reconciliation and the number of change-detection digests.
Because of the added complexity without clear payoff, I'm going to leave it the way it is for now. We can always optimize in a future PR.
There was a problem hiding this comment.
Though I did remove the observers when this is not in use. Just so that we are not adding unnecessary processing.
| track?: RemoteTrack; | ||
|
|
||
| /** @internal */ | ||
| autoManageVideo?: boolean; |
There was a problem hiding this comment.
is it intended that users can change the flag also after the TrackPublication and it's corresponding MediaTrack have been created?
I'm only asking because setting the autoManageVideo option (additionally) on RemoteVideoTrack would allow to actually opt out of instantiating the observers all together (or not calling observe in attach)
Which in turn would make all the if (this.autoManageVideo) return checks in RemoteVideoPublication redundant.
There was a problem hiding this comment.
I think this would be a room-level setting, where you couldn't change it on a track by track basis. Instead of this flag though, I'll pass down the shared observer instances and use those to determine eligibility
boks1971
left a comment
boks1971
left a comment
There was a problem hiding this comment.
Excellent stuff @davidzhao . This will be magical!
Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
5 participants
Opt-in feature for now. When enabled, the client will automatically request an appropriately sized layer that matches the attached video elements. It'll also pause the stream when the object becomes invisible, without requiring explicit management from client apps.