videojs / http-streaming

HLS, DASH, and future HTTP streaming protocols library for video.js
https://videojs-http-streaming.netlify.app/
Other
2.53k stars 425 forks source link

player.tech() is not available when "xhr-hooks-ready" is called #1499

Open ianjaku opened 8 months ago

ianjaku commented 8 months ago

Description

player.tech() is not yet available when "xhr-hooks-ready" is called. The code described as an example in the readme thus does not work out of the box.

Example: https://jsbin.com/kulewerepe/1/edit?html,console,output

Sources

Any source

Steps to reproduce

  1. Follow the getting started on https://videojs.com/getting-started
  2. Attach a callback to the "xhr-hooks-ready" hook
  3. Try to access player.tech on that hook

Results

Expected

player.tech().vhx.xhr to be accessible

Error output

player.tech() is undefined

Additional Information

This has been a recurring issue I've faced in a few video.js implementations. I do hope that I'm not misunderstanding the intended usage of player.on('xhr-hooks-ready', ...) and when it should be called. From my understanding of the video.js & http-streaming docs player.tech() should be available when "xhr-hooks-ready" finishes.

videojs-http-streaming version

Tested and confirmed on video.js v8.10.0 (videojs-http-streaming v3.10.0) and on video.js v8.11.8 (videojs-http-streaming v3.12.0) videojs-http-streaming x.y.z

videojs version

what version of videojs does this occur with? video.js x.y.z

Browsers

Verified on Firefox 123.0.1 & Chrome 116.0.5845.96 *

Platforms

Ubuntu 22.04 *

Other Plugins

Yes, but none of them are needed to reproduce the bug

Other JavaScript

Yes, but none of them are needed to reproduce the bug

welcome[bot] commented 8 months ago

👋 Thanks for opening your first issue here! 👋

If you're reporting a 🐞 bug, please make sure you include steps to reproduce it. We get a lot of issues on this repo, so please be patient and we will get back to you as soon as we can. To help make it easier for us to investigate your issue, please follow the contributing guidelines.

adrums86 commented 6 months ago

Hi @ianjaku I think the docs need an update to cover some cases like this where the player may not be ready when the xhr-hooks-ready event is fired. I suspect the reason is because the VhsSourceHandler is ready for global hooks before the tech is ready, so the hooks ready event fires first. A quick fix is adding a player.ready callback inside the xhr-hooks-ready listener. For example:

const player = videojs("video", {});
    player.on("xhr-hooks-ready", () => {
      player.ready(() => {
        console.log("Player tech:", player.tech({ IWillNotUseThisInPlugins: true }));
      });
    });
ianjaku commented 6 months ago

Hey @adrums86,

Thank you for your response.

Sadly player.ready seems to fire too late as it fires after the request for the master playlist's content has already been fired.

Take for example the following code: https://jsbin.com/hakuxebaka/1/edit?html I'm trying to add an authorization token to the end of the manifest file, and all other calls made from the player. ("?some=test" in the example).

So it seems "xhr-hooks-ready" fires before tech() is created, and player.ready() fires after the master playlist has already been fetched.

Ideally there would be a way to attach an onRequest hook that runs before any requests have been fired without having to do so globally as this would clash with other videos on the page.

adrums86 commented 6 months ago

@ianjaku no problem, yeah this is a tricky one. I was having the same issue when working on the hooks implementation last year and thus the event was created to try and split that timing issue. Apparently it doesn't work in all cases though. ☹️

Unfortunately most of the core videojs team is quite busy right now, so it might be a bit before we can give this some further attention. I'll add a note to take another look at this when I'm able, if you're feeling up to it we always welcome PRs, your contribution would be greatly appreciated!

lllllsmallgirl commented 6 months ago

@adrums86 waiting for fixing this bug!!! or is there any other solutions to call tech() before the master playlist fetched.

adrums86 commented 6 months ago

The core development team has been extremely busy recently with other priorities, when we are able to work on fixing this bug we will update the issue. That said, as an open source organization, we highly encourage community contribution. The 'xhr-hooks-ready' event is fired here. Feel free to do some debugging and see if you can find where the timing is off here. Even if you're not able to fix it, any information would be helpful. Thanks!

ionTea commented 4 months ago

I'm also running into this issue, it's blocking me from updating from v7 to v8 unfortunately

adrums86 commented 4 months ago

There has been some improvement to the xhr library that likely renders the xhr hooks in VHS obsolete. I would encourage everyone to try using the new interceptorStorage interface in the latest version of the xhr library, this is included in video.js v8.15.0+ https://github.com/videojs/video.js/releases/tag/v8.15.0. Documentation on the usage of this interface can be found in the readme for the xhr library. https://github.com/videojs/xhr?tab=readme-ov-file#xhrrequestinterceptorsstorage-and-xhrresponseinterceptorsstorage