feat: Seek bar smooth seeking #8287

amtins · 2023-05-21T10:16:02Z

Description

This PR adds a player option called enableSmoothSeeking, which is false by default, to provide a smoother seeking experience on mobile and desktop devices.

player-smooth-seek.webm

This PR includes the fix #8285 and should fix #8283.

How to use it?

// Enables the smooth seeking
const player = videojs('player', {enableSmoothSeeking: true});

// Disable the smooth seeking
player.options({enableSmoothSeeking: false});

Specific Changes proposed

player.js add an option called enableSmoothSeeking
time-display.js add a listener to the seeking event if enableSmoothSeeking is true allowing to update the CurrentTimeDisplay and RemainingTimeDisplay in real time
seek-bar.js update the seek bar on mousemove event if enableSmoothSeeking is true
add test cases
refer to fix(player): cache_.currentTime is not updated when the current time is set #8285 for the player.currentTime modification

Benchmark

The number of calls to the update method when the player is playing versus when seeking with smooth seeking enabled. The test was performed 10 times in a row and consisted of a counter that incremented by one on each call. It was inserted in the anonymous function provided to requestNamedAnimationFrame of the update method.

The duration of each round was 5 seconds. The enableInterval_ it started when the play button was clicked and stopped at the end of the setTimeout. The handleMouseMove it started on the mousedown, then was followed by frantic movement on the seek bar and stopped at the end of the setTimeout.

	enableInterval_	handleMouseMove
Round 1	142	114
Round 2	144	133
Round 3	145	126
Round 4	152	114
Round 5	144	122
Round 6	150	119
Round 7	146	112
Round 8	155	133
Round 9	144	131
Round 10	133	121

Requirements Checklist

Feature implemented / Bug fixed
If necessary, more likely in a feature request than a bug fix
- Change has been verified in an actual browser (Chrome, Firefox, IE)
- Unit Tests updated or fixed
- Docs/guides updated
- Example created (starter template on JSBin)
Reviewed by Two Core Contributors

codecov · 2023-05-21T10:20:04Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (7972c23) 82.71% compared to head (5b8b883) 82.77%.
Report is 11 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #8287      +/-   ##
==========================================
+ Coverage   82.71%   82.77%   +0.05%     
==========================================
  Files         113      113              
  Lines        7592     7600       +8     
  Branches     1826     1829       +3     
==========================================
+ Hits         6280     6291      +11     
+ Misses       1312     1309       -3

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

amtins · 2023-05-21T11:13:54Z

Observation

It would be interesting to rename this.currentTime_ to this.previousCurrentTime_ and this.duration_ to this.previousDuration_ to improve readability and better define their purpose

video.js/src/js/control-bar/progress-control/seek-bar.js

Lines 164 to 178 in f1558c6

    
           if (this.currentTime_ !== currentTime || this.duration_ !== duration) { 
        
             // human readable value of progress bar (time complete) 
        
             this.el_.setAttribute( 
        
               'aria-valuetext', 
        
               this.localize( 
        
                 'progress bar timing: currentTime={1} duration={2}', 
        
                 [formatTime(currentTime, duration), 
        
                   formatTime(duration, duration)], 
        
                 '{1} of {2}' 
        
               ) 
        
             ); 
        
             this.currentTime_ = currentTime; 
        
             this.duration_ = duration; 
        
           }

Thanks to the fix fix(player): cache_.currentTime is not updated when the current time is set #8285 it is now possible to rely only on this.player_.getCache().currentTime and thus remove the following logic

video.js/src/js/control-bar/progress-control/seek-bar.js

Lines 213 to 217 in f1558c6

    
           getCurrentTime_() { 
        
             return (this.player_.scrubbing()) ? 
        
               this.player_.getCache().currentTime : 
        
               this.player_.currentTime(); 
        
           }

video.js/src/js/control-bar/progress-control/play-progress-bar.js

Lines 64 to 66 in f1558c6

    
           const time = (this.player_.scrubbing()) ? 
        
             this.player_.getCache().currentTime : 
        
             this.player_.currentTime();

video.js/src/js/control-bar/time-controls/current-time-display.js

Line 39 in f1558c6

    
           time = (this.player_.scrubbing()) ? this.player_.getCache().currentTime : this.player_.currentTime();

mister-ben

Looks good and is more in line with other players' behaviour. Thanks!. One observation is that you no longer have an indication of where you were in the video when you started seeking, if that matters.

gkatsev

At first, I was worried about this change because the original reason we didn't update this is that internally we are still seeking directly and basically relying on it for the preview and we were worried about the potential jump it a seek failed and the mismatch between the preview and the seek bar. However, at least on desktop, it seems to be working well enough and I actually think this behavior feels a bit better. Keeping it as an option (and perhaps enabling it by default later on) makes sense to me, though, still.

Overall, this looks good to me, but I had one suggestion about the option.

gkatsev · 2023-05-24T13:42:33Z

src/js/control-bar/progress-control/seek-bar.js

+    const options_ = merge({
+      playerOptions: {
+        enableSmoothSeeking: false
+      }
+    }, options);
+
+    super(player, options_);


while it makes sense for this to be a player option, I think that rather than setting a default player option here, we should have it be translated into a seek-bar option called enableSmoothSeeking.

Maybe something like this (I didn't verify this works)

Suggested change

const options_ = merge({

playerOptions: {

enableSmoothSeeking: false

}

}, options);

super(player, options_);

const opts = merge({

enableSmoothSeeking: options.playerOptions.enableSmoothSeeking ?? false

}, options);

super(player, opts);

Then below, it could use this.options_.enableSmoothSeeking

I guess the alternative would be to always do this.player().options().enableSmoothSeeking (then someone could more easily update this on the fly and won't need to drill down to the seek bar via player.controlBar.progressControl.seekBar.options_.playerOptions.enableSmoothSeeking = true, which I had to do to test this out. However, I think making it a seek-bar option is fine and makes sense.

@gkatsev thanks for the suggestion.

I guess the alternative would be to always do this.player().options().enableSmoothSeeking (then someone could more easily update this on the fly and won't need to drill down to the seek bar

Your suggestion is much better because it offers a nicer developer experience.

Here's what the changes could look like:

seek-bar.js

constructor(player, options) { super(player, options); this.setEventHandlers_(); } handleMouseMove(event, mouseDown = false) { // ... if (this.player_.options_.enableSmoothSeeking) { this.update(); } }

time-display.js

constructor(player, options) { super(player, options); this.on(player, ['timeupdate', 'ended', 'seeking'], (e) => this.update(e)); this.updateTextNode_(); } update(event) { if (!this.player_.options_.enableSmoothSeeking && event.type === 'seeking') { return; } this.updateContent(event); }

These changes remove merge options, reducing the number of lines of code to maintain. As a result, a developer will be able to toggle the smooth seeking on the fly:

player.options({enableSmoothSeeking : true}); player.options({enableSmoothSeeking : false});

Finally, relying directly on player options is something already done for other components: clickable-component, live-tracker, poster-image and picture-in-picture-toggle.

What do you think about these changes?

Sounds good to me!

@misteroneill thank you for the feedback, I'll try to make the changes by the end of the week.

I pushed the changes. However, I don't see where I can add the documentation for this new option.

gkatsev · 2023-05-24T13:50:37Z

Also, appreciate all the PRs you've been opening, thanks!

amtins · 2023-05-26T11:04:40Z

Also, appreciate all the PRs you've been opening, thanks!

@gkatsev thank you all for taking the time to review these PRs, and if it benefits the community, all the better.

misteroneill · 2023-06-01T13:49:21Z

Echoing what @gkatsev said, it's a huge help and great to see such active contribution from a community member! Thanks!

…ethod

…is set Updating cache_.currentTime as soon as the currentTime is set avoids having to wait for the timeupdate event, which results in: - making cache_.currentTime more reliable - updating the progress bar on mouse up after dragging when the media is paused. See also: videojs#6232, videojs#6234, videojs#6370, videojs#6372

Adds a player option called enableSmoothSeeking, which is false by default, to provide a smoother seeking experience on mobile and desktop devices. Usage: ```javascript // Enables the smooth seeking const player = videojs('player', {enableSmoothSeeking: true}); // Disable the smooth seeking player.options({enableSmoothSeeking: false}); ``` - **player.js** add an `option` called `enableSmoothSeeking` - **time-display.js** add a listener to the `seeking` event if `enableSmoothSeeking` is `true` allowing to update the `CurrentTimeDisplay` and `RemainingTimeDisplay` in real time - **seek-bar.js** `update` the seek bar on `mousemove` event if `enableSmoothSeeking` is `true` - add test cases

mister-ben · 2023-12-14T07:49:27Z

Should this still have the needs-updates tag?

amtins · 2023-12-14T09:53:45Z

@mister-ben I've implemented @gkatsev's suggestions (thanks again), which improves the developer experience. From my point of view, it looks like everything has been addressed.

mister-ben · 2023-12-14T10:20:06Z

Great, let's get this merged before the next minor release then. We'll need to add it to the options guide in the videos.com repo too.

Documents the `enableSmoothSeeking` option introduced in videojs 8.9.0. Refers videojs/video.js#8287

amtins changed the title ~~Feat seek bar smooth seeking~~ Seek bar smooth seeking May 21, 2023

amtins marked this pull request as draft May 21, 2023 10:17

amtins marked this pull request as ready for review May 21, 2023 10:52

amtins changed the title ~~Seek bar smooth seeking~~ feat: Seek bar smooth seeking May 21, 2023

mister-ben approved these changes May 23, 2023

View reviewed changes

mister-ben added the needs: LGTM Needs one or more additional approvals label May 23, 2023

gkatsev reviewed May 24, 2023

View reviewed changes

misteroneill added needs: updates and removed needs: LGTM Needs one or more additional approvals labels Jun 1, 2023

amtins force-pushed the feat/seek-bar-smooth-seeking branch 2 times, most recently from 11de73b to 396e196 Compare June 2, 2023 20:15

amtins added 3 commits November 29, 2023 10:26

refactor(player): decrease the indentation level in the currentTime m…

720eecf

…ethod

amtins force-pushed the feat/seek-bar-smooth-seeking branch from 396e196 to 5b8b883 Compare November 29, 2023 09:44

amtins mentioned this pull request Dec 13, 2023

Laggy and Jumpy Video Seeking in Video.js Component #8518

Open

mister-ben added minor This PR can be added to a minor release. It should not be added to a patch release. and removed needs: updates labels Dec 14, 2023

amtins mentioned this pull request Dec 16, 2023

Fix/time tooltip overflow #8530

Draft

7 tasks

dzianis-dashkevich merged commit 608a585 into videojs:main Jan 2, 2024
10 checks passed

amtins added a commit to amtins/videojs.com that referenced this pull request Jan 7, 2024

docs: add enableSmoothSeeking option

75e3245

Documents the `enableSmoothSeeking` option introduced in videojs 8.9.0. Refers videojs/video.js#8287

amtins mentioned this pull request Jan 7, 2024

docs: add enableSmoothSeeking option videojs/videojs.com#182

Merged

phloxic mentioned this pull request Jan 18, 2024

fix: time tooltip truncated #8527

Merged

5 tasks

mister-ben pushed a commit to videojs/videojs.com that referenced this pull request Feb 3, 2024

docs: add enableSmoothSeeking option (#182)

0bffbd6

Documents the `enableSmoothSeeking` option introduced in videojs 8.9.0. Refers videojs/video.js#8287

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Seek bar smooth seeking #8287

feat: Seek bar smooth seeking #8287

amtins commented May 21, 2023 •

edited

Loading

codecov bot commented May 21, 2023 •

edited

Loading

amtins commented May 21, 2023

mister-ben left a comment

gkatsev left a comment

gkatsev May 24, 2023

gkatsev May 24, 2023

amtins May 26, 2023

misteroneill Jun 1, 2023

amtins Jun 1, 2023 •

edited

Loading

amtins Jun 2, 2023

gkatsev commented May 24, 2023

amtins commented May 26, 2023

misteroneill commented Jun 1, 2023

mister-ben commented Dec 14, 2023

amtins commented Dec 14, 2023

mister-ben commented Dec 14, 2023

feat: Seek bar smooth seeking #8287

feat: Seek bar smooth seeking #8287

Conversation

amtins commented May 21, 2023 • edited Loading

Description

How to use it?

Specific Changes proposed

Benchmark

Requirements Checklist

codecov bot commented May 21, 2023 • edited Loading

Codecov Report

amtins commented May 21, 2023

Observation

mister-ben left a comment

Choose a reason for hiding this comment

gkatsev left a comment

Choose a reason for hiding this comment

gkatsev May 24, 2023

Choose a reason for hiding this comment

gkatsev May 24, 2023

Choose a reason for hiding this comment

amtins May 26, 2023

Choose a reason for hiding this comment

misteroneill Jun 1, 2023

Choose a reason for hiding this comment

amtins Jun 1, 2023 • edited Loading

Choose a reason for hiding this comment

amtins Jun 2, 2023

Choose a reason for hiding this comment

gkatsev commented May 24, 2023

amtins commented May 26, 2023

misteroneill commented Jun 1, 2023

mister-ben commented Dec 14, 2023

amtins commented Dec 14, 2023

mister-ben commented Dec 14, 2023

amtins commented May 21, 2023 •

edited

Loading

codecov bot commented May 21, 2023 •

edited

Loading

amtins Jun 1, 2023 •

edited

Loading