Skip to content
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.

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

5.0: Consider adding "sourcechange" event #2382

Closed
nickygerritsen opened this issue Jul 20, 2015 · 17 comments
Closed

5.0: Consider adding "sourcechange" event #2382

nickygerritsen opened this issue Jul 20, 2015 · 17 comments
Milestone

Comments

@nickygerritsen
Copy link
Contributor

Our video.js based project allows to switch sources and to detect when a source has been switched we overwrote the player.src method to emit a sourcechange event.

However, we want this event to only trigger as soon as it is guaranteed that player.src() will return the new source.

Because setting the source using player.src(...) is done in a player.ready() block, this is cumbersome for us.

Currently we have code like the following:

// video.js sets source using ready function. Because that function is performed async, we also need to
// do our source change event triggering in a ready function, as we will otherwise trigger the change before
// video.js updates the source
player.ready(function()
{
    // Now we need to do a setTimeout, as otherwise it is not guaranteed that this code happens after the source change
    setTimeout(function()
    {
        player.trigger('sourcechange');
    }, 1);
});

It would be really useful if video.js itself could emit something like this sourcechange event as soon as player.src() would return the new source, i.e. as soon as the techSet('src') is performed.

Is this something that could be added?

@gkatsev
Copy link
Member

gkatsev commented Jul 20, 2015

I think you'll find that the loadstart event already will function in just about the same way you want sourcechange to function.
Also, you should be able to set the handler even before the player is ready. As long as the player is available, you can set events on it.

Let us know if that makes sense and matches your use case. Thanks.

@nickygerritsen
Copy link
Contributor Author

I will look into the loadstart event.

Setting the handler before the player is ready works, but in that case my sourcechange URL is emitted before the source is actually changed.

@gkatsev
Copy link
Member

gkatsev commented Jul 20, 2015

Ah, I see. Well, hopefully, the loadstart event will be useful :)

@nickygerritsen
Copy link
Contributor Author

Loadstart does not work if preload = none on the video element i guess? We currently have at least some failing tests in that case if I switch to the loadstart event.

@gkatsev
Copy link
Member

gkatsev commented Jul 20, 2015

Yes, you're right. In the preload=none case, it will only fire when the video gets played.
Can you elaborate on why you need to know exactly when a source has changed?

@nickygerritsen
Copy link
Contributor Author

So we are creating a quality switching plugin. When a user switches a quality while still playing or when the video is loaded, we want to seek to the time of the old source.

We use this source change event to start listening for duration changed events so we know when we can seek.

I'm also using the event to update my quality switching UI to determine if I should show the UI and what quality is currently active

@heff
Copy link
Member

heff commented Jul 20, 2015

So loadstart fires no matter what preload is, or at least it's supposed to. That should be the event you can you use for this use case. The issue here I think is related to #2326. In 5.0 we changed ready() to always be async, which is now making it so the source isn't added synchronously. See that issue for more details.

@nickygerritsen
Copy link
Contributor Author

I guess that #2326 is indeed related. I will check tomorrow if loadstart indeed is always called and if so this issue can probably be closed :)

@nickygerritsen
Copy link
Contributor Author

OK loadstart seems to do the trick for the HTML5 tech, because it is triggered both when the source changes and when the video-element actually starts loading the video.

However the Flash tech only emits the event when the data is actually loaded, so it seems. Is it possible to also emit it there after the source changes?
Currently we have an inconsistency between HTML5 and Flash I guess and our tests fail when using Flash.

@heff
Copy link
Member

heff commented Jul 21, 2015

I don't love that the loadstart event fires twice for html5 in your case, but I guess it's better than nothing. We must be calling load() again somewhere. I created some tests just to verify how the video element is working in browsers. The states aren't consistent but the event firing is at least. And if you call load() on an element where loadstart was already fired, it will fire it again.
http://jsbin.com/cuzude/4/edit?html,js,console

Flash is another issue, and we need to make it line up with how HTML5 works. We have a previous discussion around handing loadstart in the player, and we may want to try to finish that for 5.0.

@nickygerritsen
Copy link
Contributor Author

I indeed found the same, that both setting the source and calling load emits loadstart.

I hope we can at least find some solution i can use instead of "inventing" my own source change event.

heff added a commit to heff/video.js that referenced this issue Jul 22, 2015
For 5.0 we switched ready() to always execute listeners
asynchronously, but for most internal use cases we would
prefer the ready function to execute immediately if the
component is already ready.

Fixes videojs#2326
Related: videojs#2382, videojs#1667, videojs#2188
@heff heff added this to the v5.0.0 milestone Sep 14, 2015
@heff heff modified the milestones: v5.1.0, v5.0.0 Sep 15, 2015
@misteroneill
Copy link
Member

In the Brightcove Player, we need a way to detect that we are changing away from a video (e.g. in a playlist). The "loadstart" event does not work for this use-case because by the time it fires, the previous state of the player is gone. We have tentatively solved this by overriding Player#src and manually firing a "beforesourcechange" event if it is setting a playable source.

@gkatsev
Copy link
Member

gkatsev commented Jan 3, 2018

Anyone who liked this thread should chime in on #4660.

@gkatsev
Copy link
Member

gkatsev commented Mar 7, 2018

This is addressed by #4660!

@gkatsev gkatsev closed this as completed Mar 7, 2018
@BonBonSlick
Copy link

BonBonSlick commented Oct 16, 2019

I do not understand, it says "merged to master", but no such event in docs "sourceChange"

"video.js": "^7.6.0",
    player.src({
                    type: 'video/mp4',
                    src:  newVideoUrl,
                });

                player.on('sourcechange', source => {
                    console.log('sourcechange', source); // never called
                });
                player.on('sourceChange', source => {
                    console.log('sourceChange', source); // never called
                });
                this.player.on('sourceset', event => 
                   console.log(event.src, this.player.currentSrc()) // never called
               );

    player.src({
                    type: 'video/mp4',
                    src:  newVideoUrl,
                });

@gkatsev
Copy link
Member

gkatsev commented Oct 16, 2019

No need to ping multiple issues. It's generally best to open a new issue rather than replying on an old closed issue.
The feature is documented here: https://docs.videojs.com/player#event:sourceset. We felt the change was a bit risky and needed more time testing it, so we put it behind a flag. You need to pass enableSourceset to the player option to be able to listen to sourceset.

@BonBonSlick
Copy link

@gkatsev okay, thank you.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 25, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

5 participants