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] Flush low quality segments once focused #5430

Merged
merged 5 commits into from
Sep 23, 2024
Merged

Conversation

mozzius
Copy link
Member

@mozzius mozzius commented Sep 19, 2024

Fixes #5315

By default, once a fragment (~5 seconds of video) is loaded by HLS.js, it sticks around indefinitely. This means that the initial low-res fragments (which at minimum is the first 9 seconds of the video) stay low-res even after the video loops.

Solution:

  1. Keep track of low-res fragments. We do this via the FRAG_BUFFERED event.
  2. Once focused, we wait for the next fragment change, then:
  3. If the auto-detected quality level at this time is greater than low-res, flush the low-res fragments
  4. HLS.js will automatically reload the fragments on next loop, likely now in hi-res

This shouldn't result in unbounded loading of data - it only triggers the eviction if it has sufficient network conditions to load hi-res fragments, and once something is hi-res it should never be subsequently evicted.

This also fixes a bug which would result in the hi-res version loading regardless of whether the video was focused, so it might even reduce bandwidth use!

Test plan

This is tricky to test. I would suggest adding logs to the flushing event trigger (VideoEmbedInnerWeb.tsx:109) and logging the lowQualityFragments state. Also keep an eye on the network tag, and just review the video to ensure that the first ~9 seconds are hi-res after looping

A good video to test with: https://bsky.app/profile/haileyok.com/post/3l4ioa5mudh25

Copy link

github-actions bot commented Sep 19, 2024

Old size New size Diff
10.43 MB 10.43 MB -133 B (-0.00%)

}
hlsRef.current.on(Hls.Events.FRAG_CHANGED, fragChanged)

const current = hlsRef.current
Copy link
Collaborator

@gaearon gaearon Sep 20, 2024

Choose a reason for hiding this comment

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

it would be best to capture this once at the top of the effect and then always use this reference instead of dereferencing during events. fewer things that can go wrong.

Copy link
Member Author

Choose a reason for hiding this comment

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

Should I also use the captured version within the listener?

_event: Events.FRAG_CHANGED,
{frag}: FragChangedData,
) {
if (!hlsRef.current) return
Copy link
Collaborator

@gaearon gaearon Sep 20, 2024

Choose a reason for hiding this comment

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

i mean here (and in the method itself), can you use the captured variable instead? likewise everywhere. so that everything in this effect always refers to the same instance.

Copy link
Member Author

Choose a reason for hiding this comment

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

can do, I've never been sure what the best way to go about it is

Copy link
Collaborator

@gaearon gaearon left a comment

Choose a reason for hiding this comment

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

react parts seem fine to me, have not tested video parts

@haileyok
Copy link
Contributor

haileyok commented Sep 23, 2024

This video is freezing whenever it replays. Audio continues but video does not https://social-app-pr-5430.onrender.com/profile/phantasmajones.net/post/3l4tkpes56w22

Not doing that for me on main/prod.

@arcalinea arcalinea temporarily deployed to samuel/video-buffering - social-app PR #5430 September 23, 2024 18:48 — with Render Destroyed
@mozzius mozzius merged commit 91853ed into main Sep 23, 2024
6 checks passed
@mozzius mozzius deleted the samuel/video-buffering branch September 23, 2024 19:30
estrattonbailey added a commit that referenced this pull request Sep 24, 2024
* origin/main:
  Remove image resizer (#5464)
  Remove `react-native-fs` (#5463)
  Revamp image editor (#5462)
  Revamp edit image alt text dialog (#5461)
  MobX removal take 2 (#5381)
  Edit self hosting copy (#5469)
  Automatically optimise SVG assets (#5467)
  [Share Extension] Update to support video (#5385)
  Revert change in FAB animation (#5465)
  Improvements to NSE (#4992)
  Don't use flex on inputs (#5458)
  Fix web splash (#5456)
  invert the fab animation, play a haptic (#4309)
  add sideborders to <ProfileHeaderLoading> (#4995)
  [Video] Flush low quality segments once focused (#5430)
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.

First 9 seconds of video are always at a low streaming resolution.
4 participants