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

#171644111 - Resume playback #133

Open
wants to merge 10 commits into
base: develop
Choose a base branch
from

Conversation

nitinl-cogapp
Copy link
Contributor

@neilh-cogapp
Copy link
Contributor

Is there a URL where I can have a look at this?

@nitinl-cogapp
Copy link
Contributor Author

@neilh-cogapp
Copy link
Contributor

This works nicely. Good work.

Couple of things:

  1. I haven't really looked into this, but are there any potential performance issues with updating localStorage every time timeupdate fires? This event is fired every 15-250ms during playback according to the videojs docs! I am thinking more on mobile where this might potentially increase battery usage?

  2. I thought it would be a nice UX addition to add an overlay on top of the player to signpost to users "Resume or Restart?" if the app finds a local storage item. (this might be a separate story enhancement depending on how much work is involved)

@nitinl-cogapp
Copy link
Contributor Author

Re 1, I did look into this and MDN suggests that the user agent manages this:

The event frequency is dependent on the system load, but will be thrown between about 4Hz and 66Hz (assuming the event handlers don't take longer than 250ms to run). User agents are encouraged to vary the frequency of the event based on the system load and the average cost of processing the event each time, so that the UI updates are not any more frequent than the user agent can comfortably handle while decoding the video.

Re 2, good idea, seems easy enough so I'll take a look as part of this PR.

@neilh-cogapp
Copy link
Contributor

Re 1, I did look into this and MDN suggests that the user agent manages this:

The event frequency is dependent on the system load, but will be thrown between about 4Hz and 66Hz (assuming the event handlers don't take longer than 250ms to run). User agents are encouraged to vary the frequency of the event based on the system load and the average cost of processing the event each time, so that the UI updates are not any more frequent than the user agent can comfortably handle while decoding the video.

I was thinking more along the lines of is it performant to call localStorage.set() every few milliseconds given it's a file I/O operation? Perhaps it is, but would it be better to write the value only every second for example?

@nitinl-cogapp nitinl-cogapp force-pushed the resume-playback-171644111 branch from 08046c5 to 4b1945f Compare February 20, 2023 17:43
@nitinl-cogapp
Copy link
Contributor Author

I found mixed things on how resource intensive it actually is, but agree that it's overkill to do so frequently.

I've added an interval with 4b1945f which updates localstorage once per second.

I've updated the preview too: https://dev.video.hammer.cogapp.com

@neilh-cogapp neilh-cogapp added the enhancement New feature or request label Apr 3, 2023
@nitinl-cogapp nitinl-cogapp force-pushed the resume-playback-171644111 branch from a8ce7bd to a6f0fc7 Compare January 24, 2024 17:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants