invidious icon indicating copy to clipboard operation
invidious copied to clipboard

Upgrade to video.js 8

Open unixfox opened this issue 1 year ago • 9 comments

Description

The final plugin that was stopping us from upgrading to a newer video.js version was https://github.com/jfujita/videojs-http-source-selector. There is now a replacement plugin available officially by video.js: https://github.com/videojs/videojs-contrib-quality-menu.

This PR aims to use this new plugin and upgrade to video.js 8.

Drawbacks

  • Support video.js player for Internet Explorer is dropped: https://videojs.com/getting-started/#internet-explorer-support IE will still work on Invidious, just that it will use the default video player of IE with no way to switch between qualities without going through the preferences.

Fixes

Confirmed

  • Fixes https://github.com/iv-org/invidious/issues/2848
  • Fixes https://github.com/iv-org/invidious/issues/1263
  • Fixes https://github.com/iv-org/invidious/issues/2236
  • Fixes https://github.com/iv-org/invidious/issues/3220
  • Fixes https://github.com/iv-org/invidious/issues/3966
  • Fixes https://github.com/iv-org/invidious/issues/882
  • Fixes https://github.com/iv-org/invidious/issues/4386
  • Fixes https://github.com/iv-org/invidious/issues/4286

Unsure

  • Fixes https://github.com/iv-org/invidious/issues/900
  • Fixes https://github.com/iv-org/invidious/issues/1880
  • Fixes https://github.com/iv-org/invidious/issues/4307
  • Fixes https://github.com/iv-org/invidious/issues/3389
  • Fixes https://github.com/iv-org/invidious/issues/2078
  • Fixes https://github.com/iv-org/invidious/issues/3602
  • Fixes https://github.com/iv-org/invidious/issues/3550
  • Fixes https://github.com/iv-org/invidious/issues/3139
  • Fixes https://github.com/iv-org/invidious/issues/3816
  • Fixes https://github.com/iv-org/invidious/issues/4016
  • Fixes https://github.com/iv-org/invidious/issues/4035

PR depends on

  • [ ] https://github.com/iv-org/videojs-quality-selector/pull/2
  • [x] https://github.com/videojs/video.js/issues/8712
  • [x] https://github.com/videojs/video.js/issues/8641

TODO

  • [x] Icon built-in the plugin, PR opened at https://github.com/videojs/videojs-contrib-quality-menu/pull/1
  • [ ] Handle the case where there is multiple audio tracks for the same audio quality. If you try to present multiple Representation with the same ID, video.js will complain. Related to https://github.com/iv-org/invidious/issues/2007
  • [ ] Fix the plugin videojs-share
  • [x] Fix the default resolution automatic selection in DASH.
  • [ ] Fix or try to understand the reason why video.js is overriding padding-top for .player-dimensions.vjs-fluid
  • [ ] Do not load video.js on IE in order to avoid errors?
  • [ ] Frame skipping bug. Can replicate DASH 1080p on /watch?v=LXb3EKWsInQ
  • [x] Bump videojs-contrib-quality-menu to 1.0.2 but waiting for the version to be released on NPM: https://www.npmjs.com/package/videojs-contrib-quality-menu
  • [ ] Mobile experience issue: https://github.com/iv-org/invidious/pull/4439#issuecomment-2105371218

unixfox avatar Feb 19 '24 23:02 unixfox

Fix or try to understand the reason why video.js is overriding padding-top for .player-dimensions.vjs-fluid

I can answer that one for you, it is because the CSS selector in video.js got slightly more specific due to an unrelated change https://github.com/videojs/video.js/pull/7724 which was released in 7.19.1. Thus the Invidious CSS no longer takes precedence.

Not being a CSS expert myself I don't know what the most future-proof way to increase the precedence of the selector on the Invidious side would be. Adding the extra :not(.vjs-audio-only-mode) conditional would fix it but this is surely the wrong way. In my fork I added !important and that also fixed it, but we all know !important is a bad idea.

raxod502 avatar Mar 09 '24 22:03 raxod502

Issue #4199 is not fixed. I just tested this PR on macOS 14.4 with Safari 17.4 and still get the error message.

stonerl avatar Mar 15 '24 16:03 stonerl

@stonerl

Issue #4199 is not fixed. I just tested this PR on macOS 14.4 with Safari 17.4 and still get the error message.

Maybe an upstream bug: https://github.com/videojs/video.js/issues/8641. Thank you for the info.

unixfox avatar Apr 28 '24 15:04 unixfox

I use code like this in my fork, it's a combination of your work and my own:

Diff listing
diff --git a/assets/js/player.js b/assets/js/player.js
index 71c5e7da..c83d9d5e 100644
--- a/assets/js/player.js
+++ b/assets/js/player.js
@@ -18,7 +18,7 @@ var options = {
             'Spacer',
             'captionsButton',
             'audioTrackButton',
-            'qualitySelector',
+            'qualityMenuButton',
             'playbackRateMenuButton',
             'fullscreenToggle'
         ]
@@ -157,6 +157,16 @@ player.on('timeupdate', function () {
     elem_iv_other.href = addCurrentTimeToURL(base_url_iv_other, domain);
 });
 
+player.one('playing', function () {
+
+    if (!video_data.params.listen && video_data.params.quality === 'dash') {
+        var quality_menu_button = document.getElementsByClassName('vjs-quality-menu-button');
+        for (var i = 0; i < quality_menu_button.length; i++) {
+            quality_menu_button[i].className += ' vjs-icon-cog';
+        }
+    }
+});
+
 
 var shareOptions = {
     socials: ['fbFeed', 'tw', 'reddit', 'email'],
@@ -193,42 +203,47 @@ function isMobile() {
 }
 
 if (isMobile()) {
-    player.mobileUi({ touchControls: { seekSeconds: 5 * player.playbackRate() } });
-
-    var buttons = ['playToggle', 'volumePanel', 'captionsButton'];
-
-    if (!video_data.params.listen && video_data.params.quality === 'dash') buttons.push('audioTrackButton');
-    if (video_data.params.listen || video_data.params.quality !== 'dash') buttons.push('qualitySelector');
-
-    // Create new control bar object for operation buttons
-    const ControlBar = videojs.getComponent('controlBar');
-    let operations_bar = new ControlBar(player, {
-      children: [],
-      playbackRates: [0.25, 0.5, 0.75, 1.0, 1.25, 1.5, 1.75, 2.0]
+    player.mobileUi({
+        touchControls: {
+            seekSeconds: 5 * player.playbackRate(),
+        },
+        fullscreen: {
+            enterOnRotate: false,
+            exitOnRotate: false,
+            lockOnRotate: false,
+            lockToLandscapeOnEnter: false,
+        },
     });
-    buttons.slice(1).forEach(function (child) {operations_bar.addChild(child);});
-
-    // Remove operation buttons from primary control bar
-    var primary_control_bar = player.getChild('controlBar');
-    buttons.forEach(function (child) {primary_control_bar.removeChild(child);});
-
-    var operations_bar_element = operations_bar.el();
-    operations_bar_element.classList.add('mobile-operations-bar');
-    player.addChild(operations_bar);
 
-    // Playback menu doesn't work when it's initialized outside of the primary control bar
-    var playback_element = document.getElementsByClassName('vjs-playback-rate')[0];
-    operations_bar_element.append(playback_element);
-
-    // The share and http source selector element can't be fetched till the players ready.
     player.one('playing', function () {
-        var share_element = document.getElementsByClassName('vjs-share-control')[0];
-        operations_bar_element.append(share_element);
-
-        if (!video_data.params.listen && video_data.params.quality === 'dash') {
-            var http_source_selector = document.getElementsByClassName('vjs-http-source-selector vjs-menu-button')[0];
-            operations_bar_element.append(http_source_selector);
-        }
+        var buttons = [
+            'playToggle',
+            'volumePanel',
+            'captionsButton',
+            'audioTrackButton',
+            'qualityMenuButton',
+            'playbackRateMenuButton',
+        ];
+
+        // Create new control bar object for operation buttons
+        const ControlBar = videojs.getComponent('controlBar');
+        let operations_bar = new ControlBar(player, {
+            children: [],
+            name: "mobileOperationsBar",
+            playbackRates: [0.25, 0.5, 0.75, 1.0, 1.25, 1.5, 1.75, 2.0]
+        });
+        operations_bar.addClass("mobile-operations-bar");
+        player.addChild(operations_bar);
+
+        // Move operation buttons from to secondary control bar
+        var primary_control_bar = player.getChild('controlBar');
+        buttons.forEach(function (child) {
+            const elt = primary_control_bar.getChild(child);
+            primary_control_bar.removeChild(elt);
+            if (child !== "playToggle") {
+                operations_bar.addChild(elt);
+            }
+        });
     });
 }
 
@@ -386,7 +436,7 @@ if (video_data.params.autoplay) {
 }
 
 if (!video_data.params.listen && video_data.params.quality === 'dash') {
-    player.httpSourceSelector();
+    var qualityMenuOptions = {}
 
     if (video_data.params.quality_dash !== 'auto') {
         player.ready(function () {
@@ -409,12 +459,13 @@ if (!video_data.params.listen && video_data.params.quality === 'dash') {
                                 break;
                         }
                 }
-                qualityLevels.forEach(function (level, index) {
-                    level.enabled = (index === targetQualityLevel);
-                });
+                qualityMenuOptions.defaultResolution = (qualityLevels[targetQualityLevel].height + "p");
             });
         });
     }
+
+    console.log(qualityMenuOptions)
+    player.qualityMenu(qualityMenuOptions);
 }
 
 player.vttThumbnails({

It gives me a more navigable user interface where only the fullscreen button is in the bottom bar, and space is saved by moving all the other controls to the mobile operations bar. I found that I was getting some errors with the video.js 8 upgrade before making these changes. It's not a complete solution, however, and I think the quality selector is still a bit broken even with this implementation.

May be helpful.

raxod502 avatar May 10 '24 04:05 raxod502

I was getting some errors with the video.js 8 upgrade before making these changes

What errors? Can you tell us, it's quite important for achieving an ideal upgrade to video.js 8.

I think the quality selector is still a bit broken even with this implementation.

Apart from the Frame skipping bug., I have not found any other bug related to the quality selector. Do you have other ones?

unixfox avatar May 10 '24 09:05 unixfox

What errors? Can you tell us, it's quite important for achieving an ideal upgrade to video.js 8.

Yes, of course, sorry for being vague the first time. I have created a branch on my fork that matches your PR here, rolling back all of the video.js related changes that I had made. For reference, the exact diff from your PR to the branch I used for the following test case is https://github.com/unixfox/invidious/compare/videojs8...raxod502:invidious:rr-without-videojs-hacks.

One problem that I notice almost immediately is that although the control bar renders fine on desktop, I see an unexpected undefined on mobile, and the gear icon does nothing when tapped (Firefox 126.0b6, Android 14, PWA):

image

Using Firefox remote debugger, the following stacktrace appears immediately on page load:

image

The other buttons on the mobile UI work okay.

raxod502 avatar May 10 '24 23:05 raxod502

One problem that I notice almost immediately is that although the control bar renders fine on desktop, I see an unexpected undefined on mobile, and the gear icon does nothing when tapped (Firefox 126.0b6, Android 14, PWA):

Thank you for the feedback! Did you try to replicate this issue on a "vanilla" video.js project? Like with just video.js and the plugin videojs-contrib-quality-menu.

Because if you have the same issue on mobile then we have a bug in the plugin that need a ticket creation in https://github.com/videojs/videojs-contrib-quality-menu/issues!

unixfox avatar May 13 '24 17:05 unixfox

Did you try to replicate this issue on a "vanilla" video.js project?

No, the issue is around the quality menu failing to initialize properly when it's moved from its default location to the mobile operations bar by Invidious in this code:

https://github.com/iv-org/invidious/blob/44df12fdfef99873af8ceb2b592b117fed9af938/assets/js/player.js#L214-L220

There is even a comment saying that a similar problem occurs with other components:

https://github.com/iv-org/invidious/blob/44df12fdfef99873af8ceb2b592b117fed9af938/assets/js/player.js#L230

So this is not a problem that could be reproduced outside of Invidious (without repeating the same logic to manually move components around). I did set up this test case though to import the relevant plugins:

index.html
<!doctype html>
<html>
  <head>
    <meta charset="utf-8" />
    <title>Test</title>
    <link
      rel="stylesheet"
      href="https://cdnjs.cloudflare.com/ajax/libs/video.js/8.14.0/video-js.min.css"
      integrity="sha512-K8LjnAlu87KsLcGLq5Y6cpwpLkGUgYpEBCtxiEjLTGZzXih1CSSfDHeo2RQTVi4mVq2KCnXAHMoL3D1XwOz0Dg=="
      crossorigin="anonymous"
      referrerpolicy="no-referrer"
    />
  </head>
  <body>
    <video
      id="myvideo"
      class="video-js"
      controls
      preload="auto"
      width="640"
      height="264"
    >
      <source
        src="https://stream.mux.com/UZMwOY6MgmhFNXLbSFXAuPKlRPss5XNA.m3u8"
      />
    </video>
    <script
      src="https://cdnjs.cloudflare.com/ajax/libs/video.js/8.14.0/video.min.js"
      integrity="sha512-TuwqcgFUxZeSmVdVYuGxu8jfbeJHBW/Q25/p15XPRwqx/YjOWHhCGghk03ZFfO0MDvuIRCiuawSaZaIsEVTqsw=="
      crossorigin="anonymous"
      referrerpolicy="no-referrer"
    ></script>
    <script
      src="https://unpkg.com/[email protected]/dist/videojs-contrib-quality-menu.js"
      integrity="sha512-/7iX6nvO3kvMsf+rS/qml3Iv8q9Huch0UYFEaQtT9r7bSHNqqMtJmQA+stRd5CrBQYVsOsI7dDgFjDzMhX3lYw=="
      crossorigin="anonymous"
      referrerpolicy="no-referrer"
    ></script>
    <script
      src="https://unpkg.com/[email protected]/dist/videojs-mobile-ui.min.js"
      integrity="sha512-i9YYtHHqBLdHJo3eQa880kia/3I8R/XPzOvCgpVobFrE3pi7+JkYSegl/5g2PHs2w6+dgAQkREJQJuqJxuDdVw=="
      crossorigin="anonymous"
      referrerpolicy="no-referrer"
    ></script>
    <script>
      function isMobile() {
        try {
          document.createEvent("TouchEvent");
          return true;
        } catch (e) {
          return false;
        }
      }

      let player = videojs("myvideo");
      player.qualityMenu();
      if (isMobile()) {
        player.mobileUi();
      }
    </script>
  </body>
</html>

raxod502 avatar May 17 '24 22:05 raxod502