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

fix(replay): Fix video flickering on Safari #82769

Merged
merged 7 commits into from
Jan 3, 2025
+21 −13
Merged

fix(replay): Fix video flickering on Safari #82769

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
f173b56
keep prev video
c298lee Dec 31, 2024
ab38768
seems to fix flickering
c298lee Dec 31, 2024
82151ab
increase z-index of user touch
c298lee Jan 2, 2025
11f1eb6
chrome & safari works
c298lee Jan 2, 2025
77e6cde
Merge branch 'master' into cl/safari-flicker
c298lee Jan 2, 2025
f1a16b9
fix CI
c298lee Jan 2, 2025
a8722dd
rename
c298lee Jan 3, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions static/app/components/replays/player/styles.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import {css, type Theme} from '@emotion/react';
// Base styles, to make the Replayer instance work
export const baseReplayerCss = css`
.replayer-wrapper {
z-index: 1000000;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Replays should be max 60 minutes and each clip is ~5 seconds so the z-index of the clips should never reach this

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you add a comment about why/how the z-index is needed?

user-select: none;
}
Expand Down
33 changes: 20 additions & 13 deletions static/app/components/replays/videoReplayer.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,6 @@ export class VideoReplayer {
private _attachments: VideoEvent[];
private _callbacks: Record<string, (args?: any) => unknown>;
private _currentIndex: number | undefined;
private _currentVideo: HTMLVideoElement | undefined;
private _startTimestamp: number;
private _timer = new Timer();
private _trackList: [ts: number, index: number][];
Expand Down Expand Up @@ -196,6 +195,8 @@ export class VideoReplayer {
const el = document.createElement('video');
const sourceEl = document.createElement('source');
el.style.display = 'none';
el.style.zIndex = index.toString();
el.style.position = 'absolute';
sourceEl.setAttribute('type', 'video/mp4');
sourceEl.setAttribute('src', `${this._videoApiPrefix}${segmentData.id}/`);
el.setAttribute('muted', '');
Expand Down Expand Up @@ -397,29 +398,35 @@ export class VideoReplayer {

/**
* Shows the video -- it is assumed that it is preloaded. Also
* hides the previous video, there should not be a reason we show
* a video and not hide the previous video, otherwise there will
* hides all other videos, otherwise there will
* be multiple video elements stacked on top of each other.
*/
protected showVideo(nextVideo: HTMLVideoElement | undefined): void {
if (!nextVideo) {
return;
}

// This is the soon-to-be previous video that needs to be hidden
if (this._currentVideo) {
this._currentVideo.style.display = 'none';
// resets the soon-to-be previous video to the beginning if it's ended so it starts from the beginning on restart
if (this._currentVideo.ended) {
this.setVideoTime(this._currentVideo, 0);
for (const [index, videoElem] of this._videos) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need to loop through all videos? Shouldn't there only be 2 videos that are relevant: the next video to show and the current/soon-to-be-previous video?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We loop through them all since you can click on a timestamp much earlier or later in the video. It's harder to keep track of which videos need to be hidden in that case, especially if someone decided to click around different timestamps of the video. For instance, if someone's on video 5 and clicking on a timestamp that would show video 10, then we would need to hide video 4 and 5 and show video 9 and 10.
I decided to just loop through them all since adding that logic might be confusing, and when testing it with looping through everything, it didn't seem to add any visible lag or weirdness.

// On safari, some clips have a ~1 second gap in the beginning so we also need to show the previous video to hide this gap
if (index === (this._currentIndex || 0) - 1) {
if (videoElem.duration) {
// we need to set the previous video to the end so that it's shown in case the next video has a gap at the beginning
// setting it to the end of the video causes the 'ended' bug in Chrome so we set it to 1 ms before the video ends
this.setVideoTime(videoElem, videoElem.duration * 1000 - 1);
}
videoElem.style.display = 'block';
}
// hides all videos because videos have a different z-index depending on their index
else {
videoElem.style.display = 'none';
// resets the other videos to the beginning if it's ended so it starts from the beginning on restart
if (videoElem.ended) {
this.setVideoTime(videoElem, 0);
}
}
}

nextVideo.style.display = 'block';

// Update current video so that we can hide it when showing the
// next video
this._currentVideo = nextVideo;
}

protected async playVideo(video: HTMLVideoElement | undefined): Promise<void> {
Expand Down
Loading