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.

Sign up for GitHub

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

Jump to bottom

perf: only update ui on change, wrap things in requestAnimationFrame #6155

Merged

gkatsev merged 6 commits into master from perf/only-update-on-change

Aug 29, 2019

Contributor

brandonocasey commented Aug 2, 2019 •

edited

Loading

Description

Only update ui components when something has actually changed and wrap ui changes in requestAnimationFrame. This nets us most of the benefit from #6150

brandonocasey mentioned this pull request

perf: Throttle ProgressControl ui updates, add document.hidden check to time-displays #6150

Closed

brandonocasey force-pushed the perf/only-update-on-change branch 2 times, most recently from 676a919 to cfd8aeb Compare

August 2, 2019 19:33


          perf: only update on change

57cf88b

brandonocasey force-pushed the perf/only-update-on-change branch from cfd8aeb to 57cf88b Compare

August 2, 2019 19:34

brandonocasey commented

View reviewed changes

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

    
                  // The default skin has a gap on either side of the `SeekBar`. This means

                  // that it's possible to trigger this behavior outside the boundaries of

                  // the `SeekBar`. This ensures we stay within it at all times.

                  seekBarPoint = clamp(0, 1, seekBarPoint);

Contributor Author

brandonocasey Aug 2, 2019

Use the new clamp function to keep this between 1 and 0.

Member

OwenEdwards Apr 28, 2020

@brandonocasey I was just looking at this PR, and I think you have the parameters to clamp in the wrong order here, based on your comment?? This merged change seems to still be in the latest code - I'm wondering if it's a bug?

brandonocasey commented

View reviewed changes

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

    
                  const playProgressBar = seekBar.getChild('playProgressBar');

                  const mouseTimeDisplay = seekBar.getChild('mouseTimeDisplay');

                  if (!playProgressBar && !mouseTimeDisplay) {

Contributor Author

brandonocasey Aug 2, 2019

Don't do anything if we don't have either child.

brandonocasey commented

View reviewed changes

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

    
                    mouseTimeDisplay.update(seekBarRect, seekBarPoint);

                  }

                  if (playProgressBar) {

Contributor Author

brandonocasey Aug 2, 2019

Update the playProgressBar here as well because we only show it on mousemove and because getting the seekBarRect is a very performance impacting thing.

brandonocasey force-pushed the perf/only-update-on-change branch from 0e940eb to aa431bb Compare

August 2, 2019 19:52

brandonocasey commented

View reviewed changes

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

    
                  this.on(this.player_, 'timeupdate', this.update);

                  this.on(this.player_, 'ended', this.handleEnded);

                  this.on(this.player_, 'durationchange', this.update);

                  this.on(this.player_, ['ended', 'durationchange', 'timeupdate'], this.update);

Contributor Author

brandonocasey Aug 2, 2019

ended is now handled in update itself.

brandonocasey commented

View reviewed changes

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

    
                  }

                }

                enableInterval_() {

                  this.clearInterval(this.updateInterval);

                  if (this.updateInterval) {

Contributor Author

brandonocasey Aug 2, 2019

clearInterval takes time, only do it if we have to.

brandonocasey commented

View reviewed changes

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

    
                }

                disableInterval_(e) {

                  if (this.player_.liveTracker && this.player_.liveTracker.isLive() && e.type !== 'ended') {

                    return;

                  }

                  if (!this.updateInterval) {

Contributor Author

brandonocasey Aug 2, 2019

clearInterval takes time, only do it if we have to.

brandonocasey commented

View reviewed changes

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

    
                 *

                 * @private

                 */

                update_(currentTime, percent) {

Contributor Author

brandonocasey Aug 2, 2019

All of this logic was moved to the update function.

brandonocasey commented

View reviewed changes

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

    
                  );

                  // Update the `PlayProgressBar`.

                  if (this.bar) {

Contributor Author

brandonocasey Aug 2, 2019

We update this in progress-control now, next to mouseTimeDisplay

brandonocasey commented

View reviewed changes

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

    
            @@ -169,15 +133,41 @@ class SeekBar extends Slider {
          
                 *          The current percent at a number from 0-1

                 */

                update(event) {

                  // if the offsetParent is null, then this element is hidden, in which case

                  // we don't need to update it.

                  if (this.el().offsetParent === null) {

Contributor Author

brandonocasey Aug 2, 2019

This never did anything since seek bar is always visible as far as the dom is concerned due to screen readers.

brandonocasey commented

View reviewed changes

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

    
            @@ -231,7 +208,7 @@ class SeekBar extends Slider {
          
                    percent = currentTime / this.player_.duration();

                  }

                  return percent >= 1 ? 1 : (percent || 0);

                  return percent;

Contributor Author

brandonocasey Aug 2, 2019

This is clamped in slider now.


          add clamp helper

dd2e478

brandonocasey force-pushed the perf/only-update-on-change branch from aa431bb to dd2e478 Compare

August 2, 2019 19:59

brandonocasey changed the title ~~perf: only update on change~~ perf: only update ui on change, wrap things in requestAnimationFrame

brandonocasey commented

View reviewed changes

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

    
                  this.formattedTime_ = formattedTime;

                  this.requestAnimationFrame(this.updateTextNode_);

                    if (oldNode) {

Contributor Author

brandonocasey Aug 2, 2019

replaceChild is 10% faster then appendChild.

brandonocasey commented

View reviewed changes

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

    
                    if (rounded) {

                      percent = percent.toFixed(2);

                  this.requestAnimationFrame(() => {

Contributor Author

brandonocasey Aug 2, 2019

wrap in a requestAnimationFrame and only update when changed.

brandonocasey force-pushed the perf/only-update-on-change branch from 665687f to f3b8f03 Compare

August 2, 2019 20:18


          update load-progress-bar too

f07a341

brandonocasey force-pushed the perf/only-update-on-change branch from f3b8f03 to f07a341 Compare

August 2, 2019 20:27


          add ended to test faker to fix tests

baf4c23

brandonocasey force-pushed the perf/only-update-on-change branch from 9cd79bd to baf4c23 Compare

August 2, 2019 21:34

gkatsev reviewed

View reviewed changes

Member

gkatsev left a comment

First review for load progress bar. Didn't get a chance to look at the others yet.

src/js/control-bar/progress-control/load-progress-bar.js Outdated

-                    className: 'vjs-load-progress',
-                    innerHTML: `<span class="vjs-control-text"><span>${this.localize('Loaded')}</span>: <span class="vjs-control-text-loaded-percentage">0%</span></span>`
+                  const el = super.createEl('div', {className: 'vjs-load-progress'});
+                  const wrapper = super.createEl('span', {className: 'vjs-control-text'});

Member

gkatsev Aug 5, 2019

I think we only want to call super.createEl for the main div, the children should be Dom.createEl.

src/js/control-bar/progress-control/load-progress-bar.js Outdated

-                    const start = buffered.start(i);
-                    const end = buffered.end(i);
-                    let part = children[i];
+                      part.start_ = start;

Member

gkatsev Aug 5, 2019

we should avoid setting properties on DOM elements. Maybe data-attribute will be fine?

Contributor Author

brandonocasey Aug 5, 2019

that will probably work.

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


		// update the width of the progress bar
		this.el_.style.width = percentify(bufferedEnd, duration);

Member

gkatsev Aug 5, 2019

I wonder if there's a big difference between using the rounded vs non-rounded value here. Probably not noticeable?

Contributor Author

brandonocasey Aug 5, 2019

I think css only ever uses two decimal places, it's possible I am wrong though.

cr

d52c247

gkatsev reviewed

View reviewed changes

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

    
            @@ -100,12 +100,12 @@ class LoadProgressBar extends Component {
          
                      }

                      //  only update if changed

                      if (part.start_ === start && part.end_ === end) {

                      if (part.dataset.start === start && part.dataset.end === end) {

Member

gkatsev Aug 6, 2019

suprisingly, IE11 keeps having more support for features than I'd expect 😆.


          bug in clamp

c7daea5

gkatsev added the minor label

gkatsev approved these changes

View reviewed changes

gkatsev merged commit 99b610b into master

gkatsev deleted the perf/only-update-on-change branch

August 29, 2019 21:02

ustbhuangyi mentioned this pull request

fix: update the progress-bar correctly when pausing the video and cli… #6234

Closed

7 tasks

This was referenced Mar 19, 2020

[Snyk] Upgrade video.js from 7.4.1 to 7.7.5 videojs/videojs-playlist#173

Closed

[Snyk] Upgrade video.js from 7.6.6 to 7.7.5 videojs/videojs-errors#192

Closed

This was referenced Apr 28, 2020

High CPU usage after the player stays in background for a while #5937

Closed

[PLAYER] Investigate video.js background tab high CPU usage bitwave-tv/bitwave#135

Closed

fix regression: prevent requestAnimationFrame calls from repeatedly being callled when tab is in background #6621

Closed

brandonocasey mentioned this pull request

fix: Use clamp correctly in progress control #6625

Merged

brandonocasey mentioned this pull request

Feat: Add named requestAnimationFrame to prevent performance issues #6627

Merged

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

minor