-
Notifications
You must be signed in to change notification settings - Fork 475
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.
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
Update unbindVideoElement #2217
Merged
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
devalevenkatesh
changed the title
Unbind video element
Cleanup May 11, 2022
srcObject
in unbindVideoElement
devalevenkatesh
changed the title
Cleanup
Update unbindVideoElement
May 11, 2022
srcObject
in unbindVideoElement
devalevenkatesh
force-pushed
the
unbindVideoElement
branch
from
May 11, 2022 17:46
209a4e8
to
0512687
Compare
devalevenkatesh
force-pushed
the
unbindVideoElement
branch
from
May 11, 2022 17:46
0512687
to
b1d3450
Compare
devalevenkatesh
force-pushed
the
unbindVideoElement
branch
from
May 11, 2022 17:47
b1d3450
to
1fe4a5c
Compare
- Update the unbindVideoElement API to take `cleanUpVideoElement` as an optional parameter with default set to true. - When cleanUpVideoElement is true, bounded video element's srcObject is set to null. This helps in memory leaks. The stream wont remain bound to the video element once unbound. - When cleanUpVideoElement is false, the stream remains bound to the video element once unbound. - Since, this is a change in behavior the control flag is added. - Update documentation to match v3 for some methods. - Add corresponding unit tests.
devalevenkatesh
force-pushed
the
unbindVideoElement
branch
2 times, most recently
from
May 11, 2022 21:31
5b8f1a8
to
336c3f7
Compare
- Update docs. - Tested in video_test app. - Updated unit tests.
simmkyu
reviewed
May 11, 2022
devalevenkatesh
force-pushed
the
unbindVideoElement
branch
from
May 11, 2022 22:35
336c3f7
to
823bff5
Compare
ltrung
reviewed
May 11, 2022
ltrung
approved these changes
May 12, 2022
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.
Issue #:
Libraries or apps depending on JS SDK can leak memory when the video element bound to a video tile is not cleaned correctly. Based on the above issues, the
unbindVideoElement
API is used for cleanup. For example, in Amazon Chime SDK react component library, as part of the cleanup function in React'suseEffect
(or similarly in various apps) we callunbindVideoElement
for cleanup. But this does not really dissociate the media stream from the now un-bounded video element. Builders have to explicitly address this on their side. Builders expect that theunbindVideoElement
should do the opposite ofbindVideoElement
as per #1076 .In Safari specifically, memory leak is observed if the un-bounded video element's stream is not cleaned up.
Check below graph, the memory keeps increasing steadily in a pagination app where the video elements are binded and un-binded often on page changes.
I have checked multiple approaches and found that with JS SDK v3, we can now disconnect the
srcObject
from the bounded video element as part of un-bind. Below graph shows the memory usage whensrcObject
is set tonull
:Note: Check the "page" memory category.
It is also recommended in WebRTC to set this and we could not do so in v2 as we were also cleaning the media stream tracks to avoid crashing in Safari 12.
https://webrtchacks.com/srcobject-intervention/
With V3, we no more clear tracks and hence can fix this. We are still looking for the impact and will gain some more info by sharing this PR with builders and see if there are any issues.
Since, this is a behavior change, added a
cleanUpVideoElement
optional parameter inunbindVideoElement
API defaulting totrue
. If builders face any issue and thissrcObject
cleanup is un-intended, builders can callunbindVideoElement(tileId, false)
to fallback to previous behavior.Description of changes:
srcObject
as part ofunbindVideoElement
API. Note that theDefaultVideoTile
currently does that internally indisconnectVideoStreamFromVideoElement
function hence used it.POSTLogger
which was not reaching the 100% threshold.Testing:
Can these tested using a demo application? Please provide reproducible step-by-step instructions.
Tested with custom pagination sample using React component library. Sample link
Tested the JS SDK video test app to specifically test the
unbindVideoElement
API.video_test
app underintegration/js/app
folder.startLocalVideoTile
.bindVideoElement
settingtileId
to 1 and video element id asvideo-0
.stopLocalVideoTile
.null
.unbindVideoElement
and see that the tile state showsboundVideoElement
asnull
.unbindVideoElement
withcleanUpVideoElement
true and false and verify that whenfalse
the un-bounded HTML video element still has the stream attached throughsrcObject
. When true, the video element does not have the stream attached anymore.Checklist:
Have you successfully run
npm run build:release
locally?Yes
Do you add, modify, or delete public API definitions? If yes, has that been reviewed and approved?
Yes, modified
unbindVideoElement
API. The PR is for review.Do you change the wire protocol, e.g. the request method? If yes, has that been reviewed and approved?
No
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.