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.

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 ReplayOverlay rewind timer #629

Merged
merged 1 commit into from
Oct 16, 2024
Merged

Fix ReplayOverlay rewind timer #629

merged 1 commit into from
Oct 16, 2024

Conversation

balins
Copy link
Contributor

@balins balins commented Oct 16, 2024

This PR fixes a bug introduced when adding video replays in #579 that was making the timer freeze and sometimes show negative values during rewind.

The issue was caused by updating only the video player's current time and not propagating the change to the element rendering the overlay. We've seemed to rely on handleTimeUpdate callback registered on "timeupdate" event, but this event is not emitted when updating the current time of the video programmatically.
Sometimes, the timer indicated negative values during rewind, and this was also caused by setting the current time on video without updating the overlay's time accordingly, which in turn led to time = 0 (default on mount) and startTime = [video duration - 5], causing the timer to show negative values when video duration > 5 (we calculate the time displayed on timer as time - startTime).

The proposed changes include passing a callback that allows the rewind function not only for updating the video's current time, but also the current time of the overlay component.
Another change is that we won't show • REPLAY 0:00 until video metadata is loaded to avoid flickering from 0:00 to video duration when starting the rewind.

main.mov
new.mov

How Has This Been Tested:

Tested that with introduced changes, the timer behaves as expected both in rewind and normal playback.

@balins balins requested a review from kmagiera October 16, 2024 08:05
@balins balins self-assigned this Oct 16, 2024
Copy link

vercel bot commented Oct 16, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
radon-ide ✅ Ready (Inspect) Visit Preview 💬 Add feedback Oct 16, 2024 8:12am

Copy link
Member

@kmagiera kmagiera left a comment

Choose a reason for hiding this comment

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

Code looks good. Can you just expand in the description why was the issue occurring and why the proposed changes fix it?

@balins
Copy link
Contributor Author

balins commented Oct 16, 2024

Code looks good. Can you just expand in the description why was the issue occurring and why the proposed changes fix it?

Updated the description 👍

@balins balins merged commit c60b347 into main Oct 16, 2024
3 checks passed
@balins balins deleted the jb/fix-rewind-timer branch October 16, 2024 14:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants