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

[Video] Performance tweaks for Android #4997

Merged
merged 10 commits into from
Aug 28, 2024

Conversation

haileyok
Copy link
Contributor

@haileyok haileyok commented Aug 26, 2024

built on #5003

Why

  // On Android, there are some performance concerns if we call `updateActiveViewAsync` too often while scrolling.
  // This isn't really a problem with how `updateActiveViewAsync` works, but rather that it will cause too many
  // ExoPlayer instances to be created and destroyed, which is pretty expensive and can cause some jank in the feed.
  // When comparing to Twitter, it seems they are much more conservative with when videos will begin playback than
  // they are on iOS, likely for the same reason.
  //
  // Because we're going to be calling this far less on Android, we will also be extra careful to make sure some video
  // starts playing when we reach the end of a scroll by calling `updateActiveViewAsync` at the end of a scroll (whether
  // the user ending the drag or the momentum ending). These extra calls will _not_ be excessively expensive, since it
  // is guaranteed to only happen once (React Native will only call either `onEndDrag` or `onMomentumEnd`, not both) and
  // any scroll performance jank will not be noticeable, since the scroll has already completed.

Copy link

render bot commented Aug 26, 2024

Copy link

github-actions bot commented Aug 26, 2024

Old size New size Diff
7.13 MB 7.13 MB 0 B (0.00%)

@haileyok haileyok marked this pull request as ready for review August 26, 2024 22:59
src/view/com/util/List.tsx Outdated Show resolved Hide resolved
@haileyok haileyok changed the base branch from main to hailey/rework-vid-structure August 27, 2024 17:37
Copy link
Member

@mozzius mozzius left a comment

Choose a reason for hiding this comment

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

Looks good, although we do need to get the currentTime indicator back at some point

Comment on lines +94 to +101
// const interval = setInterval(() => {
// // duration gets reset to 0 on loop
// if (player.duration) setDuration(Math.floor(player.duration))
// setCurrentTime(Math.floor(player.currentTime))
//
// // how often should we update the time?
// // 1000 gets out of sync with the video time
// }, 250)
Copy link
Member

Choose a reason for hiding this comment

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

Any idea what we need to do to uncomment this? :/

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yea, I have a plan to fix this today.

@haileyok haileyok merged commit 32d8007 into hailey/rework-vid-structure Aug 28, 2024
6 checks passed
@haileyok haileyok deleted the hailey/video-perf-work branch August 28, 2024 15:45
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