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

VJS 5: Tech 2.0 proposal #2008

Closed
eXon opened this issue Apr 3, 2015 · 9 comments
Closed

VJS 5: Tech 2.0 proposal #2008

eXon opened this issue Apr 3, 2015 · 9 comments

Comments

@eXon
Copy link
Contributor

eXon commented Apr 3, 2015

Ok I wrote this. Rewrote this. Now I re-re-re-re-rewrote this because having used the tech so much their is a lot of issues I want to address. Here is the first version that is worth discussing.

The data flow between the player and the tech could get some improvements.
The ideal situation is to achieve a perfect two-ways data binding between the player and the tech.

  • When an event happens on the VJS player, the tech player should be notified.
  • When an event happens on the tech player, the VJS player should be notified.

However, the tech should not care about keeping the state of anything.
He should not know what is the current volume or the current time.
He should only send the initial state at the player and then notify when a state change.
The VJS player is the only place where the current states should be known.

(keep in mind this is just a way to express my idea in a simple way, not how things work)
(I don't know if we will keep the current EmitEmitter or use the one in node.js I don't care that much, this is only about the techs)

class YoutubeTech extends VjsTech {
  constructor(element, options) {
    super(element, options);

    element.src = 'https://www.youtube.com/embed/' + videoId;

    this.onVjsPlay = this.onVjsPlay.bind(this);
    this.onVjsPause = this.onVjsPause.bind(this);

    this.on('play', this.onVjsPlay);
    this.on('pause', this.onVjsPause);
    // ...

    // I'm not so sure with the emitToOthers name but it is pretty self-explanatory, I don't think their is something built-in
    this.emitToOthers('volume', initialValue);
    this.emitToOthers('poster', 'big-fat-cat.jpg');

    this.on('dispose', function() {
      this.removeListener('play', this.onVjsPlay);
      this.removeListener('pause', this.onVjsPause);
    });
  }

  onVjsPlay() {
    // Call play on the YouTube API
  }

  onVjsPause() {
    // Call pause on the YouTube API
  }

  onYoutubePlay() {
    this.emitToOthers('play');
  }

  onYoutubePause() {
    this.emitToOthers('pause');
  }
}

YoutubeTech.createElement = function() {
    var iframe = document.createElement('iframe');
    iframe.src = 'about:blank';
    return iframe;
};

Then the player would only need:

// When a new event happens in the player, it call tech.emitToOthers(event)
// When a new event happens in the tech, it calls player.emit(event)
this.broadcast(tech);

Your first question is probably going to be: where is my player reference?
The answer is: you don't need it. All you need is to make sure to handle all the possible events your tech can support.

I WANT MY PLAYER REFERENCE MOMMIE!
Then, if you need to touch anything else, it should be in a plugin, not a tech.
We will need to add a bunch of new events to support this but I feel this is the right separation of concern and would help people write plugins.

How would we handle the current time (or any other state with no user event)?
Those will be updated when you ask for it if it doesn't make sense to poll them all the time. For YouTube, their is an event that keep telling us the progress of the video. But for another tech that don't do that, we could send him an event to ask for an update. I'm pretty sure we have a feature setting for that right now.

How would we handle the DOM?
You can define how to create the DOM element for your tech.
This should allows us to reuse the same element created before no matter what tech we use.
The core will take care of adding it at the correct place and detaching it when disposing.
If you can't re-use the element, the tech can destroy the element to force a new one to be created (no current tech need that AFAIK)

Overall there is three main DOM element we want to create:

  • video tag based (html5)
  • embed tag based (flash)
  • iframe tag based (anything else like YouTube, Vimeo, Wistia, Viddler, ...)

But we keep an open door for something new.

I don't think the tech should do anything else with the DOM except creating the correct element and listening to his events.

Do you guys have any idea on how to improve it (or if I should flush it down the toilet)?


If we can agree on something I'll would like to take the PR ball to make sure it gets implemented quickly before the planned release of 5.0. But before doing anything I'm sure a lot of other people have ideas and I would like to hear them.

@heff
Copy link
Member

heff commented Apr 4, 2015

Thanks for the writeup @eXon! It's great having you dig in, and you provide a perspective that few of the other contributors have, since you work with a tech element (youtube iframe) that you have literally no control over and may be limited in a lot of ways compared to the vide element or the flash player.

So in #1485, what I was actually going for was more like this diagram.
untitled-1_r2_c2

Method calls in that diagram also represent property getters/setters. Let me know if all that is clear.

When an event happens on the tech player, the VJS player should be notified.

Definitely agree with that.

When an event happens on the VJS player, the tech player should be notified.

I don't think I agree with that. With #1485 I was actually trying to get away from the Tech and the Player needing to know about each other, because it's become a confusing relationship. See #863 and #1194.

If I channel my inner @dmlap, the fact is that there is state that has to exist at the tech element level (networkState, readyState, currentTime, buffered or whatever their equivalent names are for the specific video object) and storing that state on the player opens up possibilities for getting out of sync with the actual video object.

So, in summary, the video object (video element, flash object, youtube iframe) is where the state would live whenever possible, the tech would be simply a translation layer for the state, events and methods (and for creating the object), and the player would call methods on the tech and listen for events on it, removing the need for the Tech to even know about the player.

With what you know of the YouTube iframe, does that sound at all like a reasonable direction?

@eXon
Copy link
Contributor Author

eXon commented Apr 4, 2015

I like the idea. Their is one limitation that we might want to be careful. It removes the possibilities of having tech events that are only handled by plugins. Here is an example:

Let say someone write an awesome playlist plugin. It works fine with the flash and Html5 tech. However, he wants to take advantage of the YouTube playlist when possible. With VJS 4.12, the tech could be sending the list of videos with a custom event on the player. It would be easy for the plugin to listen for the custom event on the player and get them.

If the tech stop emitting events on the player, how would we bubble up custom events and custom data from the tech all the way to the plugins?

@heff
Copy link
Member

heff commented Apr 5, 2015

However, he wants to take advantage of the YouTube playlist when possible.

That's a good scenario to look at, I hadn't considered it. We have a few other features that I feel like are similar, in that we could rely on the feature that's built into the underlying tech element or create our own custom version.

The way we handle these is a little scattered, but I've been working on a better solution in #1474, and #1147 is related.

But so I would consider the YouTube playlists the native version, and then there's a few plugins for video.js that create custom playlists (including a new one from Brightcove).

The other thing to consider is what if the person writing the awesome playlist plugin wants it to work for Vimeo playlists, Dailymotion, SoundCloud, and other techs that have native playlists, and even custom playlists defined by the player user. We could help standardize how plugins get the native playlist information at the player level so each tech isn't creating their own custom way of exposing that information.

So basically I'm saying that if it makes sense to, we could make playlists a first-class concept in video.js and standardize how you access them whether they're custom or native. I think that's a good idea at least for features that we think are going to be commonly supported in underlying tech players like YouTube and Vimeo, though others might want to weigh in on that idea.

Do you like that direction at all? Aside from playlists, are there other examples we could look at of things we'd want the tech to bubble up?

@eXon
Copy link
Contributor Author

eXon commented Apr 5, 2015

If we have everything we need to handle the playlist, it would help a lot.

And yes there is other events (outside of the playlist) that the player don't support right now that would be useful.

  1. Video quality
    When using the YouTube tech, their is different video quality you can switch from. The menu is built by the tech directly in the DOM but it would be nice if that information (active quality + list of possible) could be accessible for the plugins.
  2. Close caption
    When using external techs like YouTube, we can get the CC informations from the YouTube API. We should be able to get the available languages, set it and know when they display it (in case they are not using the VJS controls). I don't know if it is possible but the YT tech don't do that at the moment.
  3. Playback speed
    I don't know if it is possible for Html5/flash, but for YT you can change the speed of the video.
  4. Cue video
    Again this is related to the playlist but we can cue as much videos (or playlists) as we want to be play one after another.

This could easily be all supported and wouldn't need custom events. If the tech has the feature, we should let him do the job instead.

Now speaking of features, I personally would like to have a method within the tech that return if a feature is available. Then, the player would look at it and if it can fake the unavailable feature from the outside, it would return true. I don't know which feature could be faked like that.

@eXon
Copy link
Contributor Author

eXon commented Apr 20, 2015

@heff I'm trying to figure out how I can get rid of the player inside the tech but I fail to see the value. It will need a lot of refactoring and I don't think it will improve the quality of the code or the player.

If we don't have access to the player, it will need to listen for tech events and send them back to the plugins. Right now, the plugins are listening on the player so it is straightforward. We would also need access to the player DOM to either change it or listen to the video tag.

I don't care getting rid of the player inside the tech, but it would not make sense without getting rid of the DOM manipulation inside the tech as well. What do you guys think?

@eXon
Copy link
Contributor Author

eXon commented Apr 20, 2015

When I'm talking about DOM manipulation, I'm talking about creating / reusing the <video>, <embed> or <iframe> tags. If the tech touch them, it needs access to the player and his elements.

@heff
Copy link
Member

heff commented Apr 21, 2015

I see what you're saying and it will be a lot of work, but I still think it's worth the revision. The overall philosophy of techs is that they should all work the same. If a tech has to modify the dom of the player or add controls that weren't there before, then I don't think the player is doing a good job providing everything the tech needs. The tech would then not be working the same as the other techs and could break the expectations of the player and other plugins. We want the player and plugins to work the same no matter which tech is used (assuming the specific feature is supported by the tech).

It looks like you've had to do a lot of work to get around how closed the player has been with the mangled variables, including manually modifying the dom. Sorry for that. :( Now that we're not mangling variables I think we can simplify some of this work using the components and conventions that are built into video.js.

it will need to listen for tech events and send them back to the plugins

Yeah, that's the relationship I think would be better. Right now every tech builds its own method for triggering events directly on the player. This opens the door for additional differences between techs, and also makes it more complicated to write tests for tech events, since it requires a fully functioning player. In the new model techs would simply trigger their own events and not be concerned with the player.

We would also need access to the player DOM to either change it or listen to the video tag

The html5 and flash techs don't modify the dom except to insert their own element into the player div, and that piece I think is the wrong approach. Everywhere else, the parent player is responsible for adding the child component's element to itself, so I think the player should be responsible for adding the tech element to itself also, and take that responsibility off of the tech.

Overall I think the main benefit of these changes is to take work away from the techs so they're easier to understand, build and maintain.

Looking at the youtube tech source, here's some of the notes I have:

this.player_.options()[...]

https://github.com/eXon/videojs-youtube/blob/master/src/youtube.js#L46
We're going to try to move away from accessing options like this on the player. It creates two places where values like autoplay live when there should be just one (player.autoplay()). Instead they'll be passed into the tech as options (new Youtube({ autoplay: true })), or the player will change the value later by calling a method on the instance, youtube.setAutoplay(true).

player_.options()['ytcontrols']

Anything youtube-specific should really be scoped under the youtube key. options.youtube.controls. This is what the built-in techs already do, and then those specific options get passed to the tech in the options argument.

this.qualityButton = document.createElement('div');

https://github.com/eXon/videojs-youtube/blob/master/src/youtube.js#L63
This relates to what I was saying in a previous comment, and I think we should move this into it's own project and standardize how the available quality levels are exposed by the tech, so that this button could be used with all techs that have quality levels.

this.player_.bigPlayButton.hide();

https://github.com/eXon/videojs-youtube/blob/master/src/youtube.js#L265
Is this something that should be fixed in the main player? Should we always hide the big play button when the spinner is showing?

this.playerEl_.insertBefore(this.el_, this.playerEl_.firstChild);

https://github.com/eXon/videojs-youtube/blob/master/src/youtube.js#L101
This is exactly how the HTML5 and Flash work too...but as I mentioned, I now think it's backwards.

So yeah...not a small amount of work. I can help if you think this is a good direction.

@eXon
Copy link
Contributor Author

eXon commented Apr 22, 2015

Alright, happy to see we're on the same page for how the tech should look like. I'll start a pull request and start small so we can see how it could potentially looks like.

I will start with only changing the events being trigger on the tech instead of the player and then we could work on getting the DOM/player reference out it. Looks good?

@eXon eXon mentioned this issue Apr 22, 2015
8 tasks
@eXon
Copy link
Contributor Author

eXon commented Apr 22, 2015

@heff For the

this.player_.bigPlayButton.hide();

It was only a problem for YouTube because we had to way until the API is ready before triggering the play event. It might not even be needed anymore or have a better work around now. I'll probably do a complete rewrite for VJS 5.

@eXon eXon closed this as completed May 6, 2015
@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 27, 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

2 participants