Tech breaks other video.js players on same page

[collab-project]

Tech breaks other video.js players on same page

[thijstriemstra]

it adds a non-intuitive extra step

I agree it's non-intuitive but this plugin does not run without any javascript setup (atm) and I'd like to keep the actual metadata in the audio tag to a minimum, and specify it using the player config.

AFAIK it's not possible to specify a src of an audio element and use that to render the waveform anyway. so defining the track metadata there is confusing to me because you still define a 'src' on the player config, so why not define the texttracks there as well?

Not being able to specify a src of an audio element is something from video.js 4.0 times (when this plugin was created), when it was not possible, or really hard, to override or interact with that src attribute, so I went the all-in javascript route with this plugin. It might be easier today though (with techs etc).

I encourage compatibility with the src of audio elements of course, the simpler the better, and I think this tech might be the way to go, but it cannot interfere with other video.js players as it's doing now.

As long as it works on Safari, with other players, etc, I'm happy ;)

[mfairchild365]

Well, I suppose there are a few things:

1. The issue was never being able to access text tracks from the plugin itself. I wrote the tech because the videoJS interface expects timing events to bubble up from the tech, not a plugin. If we emit time change events from the plugin, the videoJS interface for text tracks will not work at all. This may or may not still be true. The PR you have in place breaks text tracks in every browser.
2. I think its best to support safari by default, rather than having to opt-in to support safari.
3. I'd suggest giving our custom tech a unique name (which would fix this problem), but then captions would break. VideoJS core will remove the audio element if the tech isn't name Html5.
4. I think the tech initialized every time a new player is created. We might be able to look at the options object in the constructor to determine if the tech should use wavesurfer or fall back to the default html5 tech. In other words, we could look at the element and avoid all overrides if the element is a

[thijstriemstra]

Thanks for your feedback. Everything all feels like a big hack unfortunately. Every time I think I found a solution, something else shitty pops up.

• cannot use the same name (Html5) because it overrides the original tech and breaks non-videojs-wavesurfer players
• cannot use a different name because video.js will remove the media element if tech name isn't Html5

what chicken-egg-shit is this ;) I'll revert the changes in PR #61 regarding the tech, merge it and start a new branch for this tech stuff.

[thijstriemstra]

@mfairchild365 I was always thinking about specifying the text tracks in the player config, instead of defining them in the tag. There's the https://github.com/videojs/video.js/blob/master/docs/guides/options.md#nativetexttracks option that can be set by videojs-wavesurfer plugin, and if we can get the timeupdate stuff working we're good to go?

Something like:

<audio id="myAudio" class="video-js vjs-default-skin"></audio>

<script>
var player = videojs('myAudio', {
    controls: true,
    autoplay: true,
    fluid: false,
    width: 600,
    height: 300,
    nativeTextTracks: false,
    plugins: {
        wavesurfer: {
            src: 'media/hal.wav',
            texttracks: [
                 {
                     kind: 'captions',
                     language: 'en-US',
                     label: 'English',
                     src: 'media/hal.vtt'
                 }
            ],
            msDisplayMax: 10,
            debug: true,
            waveColor: '#086280',
            progressColor: 'black',
            cursorColor: 'black',
            hideScrollbar: true
        }
    }
});
</script>

[thijstriemstra]

This also looks related, introduced in video.js 7.0: an option to override native audio and video in html5 tech

[thijstriemstra]

Adding text tracks programatically seems to work, making the texttracks config option possible:

diff --git a/src/js/videojs.wavesurfer.js b/src/js/videojs.wavesurfer.js
index 2bc0cda..4700453 100644
--- a/src/js/videojs.wavesurfer.js
+++ b/src/js/videojs.wavesurfer.js
@@ -73,6 +73,16 @@ class Wavesurfer extends Plugin {
     initialize() {
         // setup tech
         this.player.tech_.setActivePlayer(this.player);
+        // define text track settings
+        var captionOption = {
+            kind: 'captions',
+            srclang: 'en',
+            label: 'English',
+            src: 'media/hal.vtt',
+            mode: 'showing'
+        };
+        // add and show text track
+        this.player.addRemoteTextTrack(captionOption, true);
 
         // hide big play button
         this.player.bigPlayButton.hide();

But they're not actually displayed during playback. The UI elements are visible but I guess the lack of timeupdate on the tech prevents displaying the texttrack..

[thijstriemstra]

@mfairchild365 there's also an issue with the z-index, or something, of the track selection menu during playback:

[mfairchild365]

@thijstriemstra I don't think that method is ideal (it adds a non-intuitive extra step), but as long as it is documented in this project, I think it is fine.

As you found, you would still need a custom tech to emit the time changes for the videojs UI to update. HOWEVER, by requiring this method of adding text tracks, I don't think the tech would need to override the default html5 tech. Giving the custom tech unique name in combination with this technique would fix the problems that have been identified (I think).

[thijstriemstra]

Looks like the current tech implementation also raises the CPU to "about 100% (on one core), about 50-60% without videojs-wavesurfer", see #62

[thijstriemstra]

A short term solution would be renaming our current custom tech (registering it under a different name than Html5) and make the tech have precedence in the texttrack example so it overrides the default HTML5 tech and still provides tech tracks?

[mfairchild365]

Yes, that would work, although I don't think it is ideal. The CPU issue would still be relevant, just scoped to players that need text track implementation. I rather like the rate limiting solution that was suggested in the other issue as it strikes what I see as an acceptable middle ground. Perhaps a combination of the two would be best: I wonder if you could detect the presence of text tracks and automatically load the tech so that it isn't yet another thing the implementer needs to know or deal with to get text tracks working.

The timeupdate event is critical because the videojs player watches the tech for that event in order to drive text tracks.

[thijstriemstra]

I created #65 and managed to get the text track example work without any other modifications (so both using a <track> element and the dynamic video.js tracks option). The tech was removed because it breaks the player (and introduced high CPU, see #63). We can introduce the tech again if:

• it properly works for all browsers or
• it is used as a workaround for Safari only (so you have to manually videojs.registerTech the custom tech in safari)

I think it also doesn't make sense to try to make text tracks work in Safari, when an even bigger and more important issue (actual normal playback on Safari, see #47) has to be solved before adding other features.

I will open a new ticket for Safari text track support (which will be blocked by #47 but perhaps that also fixes it) and optimize #65 a little more. It's unfortunate to have to rip out code now but a half-baked solution that resulted in large issues with other plugins and browsers is a bigger problem IMO and it's hard to move forward this way.