-
Notifications
You must be signed in to change notification settings - Fork 160
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
Safari leaks memory when videos are rapidly removed from the screen and re-added #771
Comments
I found some additional details. It seems that unbinding the video element ( That is why me adding |
Hi @mmarekbb , Yup you are right that the I am still checking on the Questions:
|
Created a demo using react component library which at an interval mounts and then un-mounts 25 Test Scenario
Observations
Few next questions
|
HI @devalevenkatesh, that is the same behavior I was observing. Setting srcObject helps a bit but the tracks are causing another memory leak. Answers:
|
Thanks for getting back @mmarekbb . For the source tracks cleanup, I have the following code which attaches the tracks back correctly after un-mount and re-mount, could you try if this helps? return () => {
const tile = audioVideo.getVideoTile(tileId);
if (tile) {
audioVideo.unbindVideoElement(tileId);
// Added the cleanup.
if (videoEl.current) {
const mediaStream = videoEl.current.srcObject as MediaStream;
const tracks = mediaStream.getTracks();
for (const track of tracks) {
track.stop();
mediaStream.removeTrack(track);
}
videoEl.current.srcObject = null;
}
}
} |
Hi @mmarekbb , Checking if you found time to test above suggestion. I also tested just cleaning up tracks and not the Graph for cleaning up source tracks but not setting srcObject to null: |
Hi @devalevenkatesh, this unfortunately has the issue I was mentioning earlier. The tracks are stopped but then no videos start loading again. I'm testing this with 25 videos (which is the video limit) if that makes any difference. I'm also attaching the console logs, I'm getting a lot of |
Thanks for checking @mmarekbb . Well, I think it is hard to reproduce how the binding un-binding works when paginating unless we update the demo to do that. Could you please let me know:
Next Steps:
|
Thanks @devalevenkatesh. I'll try to boil our pagination component down as much as possible to share with you. For the time being, here is the answer to your questions (consolidated into one block :)) Our app allows users to choose to see fewer streams. By default, we display 25, but in cases where the screen estate is too small or the user wishes to use fewer, we limit it to say 6. We use the When switching to the next page, we take another 6 attendee IDs from the roster, unmount the previous 6 and using the same mechanism we render the new 6 |
@devalevenkatesh I can reproduce with this example of pagination: import * as React from 'react';
import {
useRemoteVideoTileState,
useRosterState,
useAudioVideo,
useApplyVideoObjectFit,
VideoTile
} from 'amazon-chime-sdk-component-library-react';
// Copied from the React SDK, added cleanup and replaced className by some simple styles for the example.
const RemoteVideo: React.FC<any> = ({
name,
tileId,
...rest
}) => {
const audioVideo = useAudioVideo();
const videoEl = React.useRef<HTMLVideoElement>(null);
useApplyVideoObjectFit(videoEl);
React.useEffect(() => {
if (!audioVideo || !videoEl.current) {
return;
}
audioVideo.bindVideoElement(tileId, videoEl.current);
return () => {
const tile = audioVideo.getVideoTile(tileId);
if (tile) {
audioVideo.unbindVideoElement(tileId);
// Added the cleanup.
if (videoEl.current) {
const mediaStream = videoEl.current.srcObject as MediaStream;
const tracks = mediaStream.getTracks();
for (const track of tracks) {
track.stop();
mediaStream.removeTrack(track);
}
videoEl.current.srcObject = null;
}
}
}
}, [audioVideo, tileId]);
return (
<VideoTile
{...rest}
ref={videoEl}
nameplate={name}
style={{width: 300, height: 169}}
/>
);
};
export const RemoteVideos = () => {
const { attendeeIdToTileId } = useRemoteVideoTileState();
const { roster } = useRosterState();
const [startIndex, setStartIndex] = React.useState(0);
const PER_PAGE = 3;
const total = Object.keys(roster).length;
const attendees = Object.values(roster).slice(startIndex, startIndex + PER_PAGE);
console.log({ startIndex, attendees, total });
return (
<div style={{ width: 1000, height: 800 }}>
{startIndex > 0 && (
<button onClick={() => setStartIndex(Math.max(0, startIndex - PER_PAGE))}>Previous page</button>
)}
{startIndex < total && (
<button onClick={() => setStartIndex(Math.min(total - 1, startIndex + PER_PAGE))}>Next page</button>
)}
<div style={{display: 'grid'}}>
{attendees.map(attendee => (
<RemoteVideo
key={attendee.chimeAttendeeId}
name={attendee.name}
attendeeId={attendee.chimeAttendeeId}
tileId={attendeeIdToTileId[attendee.chimeAttendeeId]}
/>
))}
</div>
</div>
);
} |
Thanks @mmarekbb for providing the code snippet, I could repro the issue just had to clean up the local attendeeId showing from the remote attendee videos. I found that the main reason for the video going black is due to the cleanup in If we remove the tracks using Thus, due to this we cannot touch the I have pushed the pagination code sample to this repo (slightly modified from your version so thanks for providing yours): I also tried the pagination using I have pushed the code sample to this repo: The graph on the memory leak with this sample looks like below:
Also, cleaning up using
I will consult within team if I can do more but for now I am concluding that:
|
@mmarekbb The memory leak fix for setting the We have made provision in the API to optionally fallback to older behavior if new is not un-intended. Once JS SDK release is done, component library will take the dependency and release in its next version. Thank you for your patience and all the inputs on this. |
@devalevenkatesh Thanks for the update. For the time being we're unsetting the srcObject in our codebase. Should this ticket be closed? There still is a memory leak in Safari, although not as big as before. |
Yes, I think the major leak is the one we are addressing with the |
I am closing this issue since builders can install the JS SDK 3.1.0 or later on the latest component library 3.2.0. Newer release on component library is not needed. Thanks! |
What happened and what did you expect to happen?
Safari has a problem with the HTMLVideoElement object that keeps leaking memory if there are many video elements that are being removed from the screen and re-attached back.
There is a similar bug reported in the WebKit bugzilla: https://bugs.webkit.org/show_bug.cgi?id=216820
At some point, Safari will show a message that the application is using significant memory. If it continues even further, Safari will crash.
Have you reviewed our existing documentation?
Reproduction steps
I'm able to confirm that this happens because RemoteVideo.tsx. If I comment the useEffect out so no video stream is attached, I cannot see any memory leaks.
Amazon Chime SDK React Components Library version
2.15.0
What browsers are you seeing the problem on?
MacOS Safari, iOS Safari
Browser version
14.1
Device Information
MacOS 11.6.4
Meeting and Attendee ID Information.
No response
Browser console logs
collaborate-ultra-log-1649082858254.log
Add any other context about the problem here.
Updating the RemoteVideo.tsx component's useEffect to this seems to help but it does not eliminate the memory leaks completely. Perhaps there's something else going on?
The text was updated successfully, but these errors were encountered: