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

Template for Tech #835

Open
ghost opened this issue Nov 15, 2013 · 10 comments
Open

Template for Tech #835

ghost opened this issue Nov 15, 2013 · 10 comments
Labels
confirmed documentation enhancement pinned Things that stalebot shouldn't close automatically
Milestone

Comments

@ghost
Copy link

ghost commented Nov 15, 2013

Hello,

I'm having problems writing new tech and it would be great if there were a functional template with all necessary functions to implement, which triggers to be sent, which parent methods to call, etc.

An "echo" tech ( techOrder: ["echo"]) could be imagineable for sources of type "videojs/echo" and it could log when each function is being called. It would help tremendously when writing an initial tech.

The reason I ask is because I'm not able to deduce that simply by looking at the existing tech. In addition, because the existing tech is in this repository, when it's compiled and minified the internal variable names change and when they are needed in subclasses, there are unhelpful errors like a is null in the javascript console.

Here is an example of tech (youtube,vimeo,dailymotion) that works once, but not when a new source is loaded.

Cheers

@heff
Copy link
Member

heff commented Nov 15, 2013

The tech docs could definitely be improved, and there are some tech methods that could be exposed. Have you had a chance to check out the other techs in the plugins list?
https://github.com/eXon/videojs-youtube

Feel free to ask any questions here, or for specific functions to be exposed.

(the link in your description is failing with a 502)

@heff
Copy link
Member

heff commented Nov 15, 2013

also the echo tech is a cool idea

@ghost
Copy link
Author

ghost commented Nov 16, 2013

Hi,

I updated the issue with a link to the functional plunk. I includes tests with eXon/videojs-youtube , eXon-videojs-vimeo and benjipott/video.js-dailymotion . It is ugly, but shows that player.src(url) doesn't work with them.

Cheers

@heff
Copy link
Member

heff commented Nov 21, 2013

Do we know if that's an issue with the video.js api or with the specific tech plugins? @eXon?

@eXon
Copy link
Contributor

eXon commented Nov 21, 2013

This issue has been for videojs-youtube. I doubt it is a problem with HTML5 and Flash.

@ghost
Copy link
Author

ghost commented Nov 22, 2013

I going to test that by excluding HTML5 from the videojs build and minification, then adding it as tech in a javascript tag. That's the situation we are currently in as external tech users and devs.

It's possible there will be issues with isReady_ e.g
in player.js#L545

// Pass values to the playback tech
vjs.Player.prototype.techCall = function(method, arg){
  // If it's not ready yet, call method when it is
  if (this.tech && !this.tech.isReady_) {
    this.tech.ready(function(){
      this[method](arg);
    });

isReady_ is not defined in HTML5 but in the parent class Component.js in it's triggerReady(), which (after being minified) should have a different variable name.

It might get it done on the weekend, so if somebody wants to pick up the ball beforehand, go ahead. I'm looking forward to the results :)

@ghost
Copy link
Author

ghost commented Nov 22, 2013

The test

As seen in this plunk, the split HTML5 tech doesn't work when split and used together with the minified videojs.

Errors

I just started adjusting the code a little. A diff of the HTML5 will show what changed.

  1. vjs doesn't exist anymore --> replace with videojs.
  2. videojs.TEST_VID is private and not exported during compilation --> videojs.TEST_VID = document.createElement("video");
  3. videojs.on is private as well --> this.on

That leaves it in it's current state where, when initialized without a source, triggerReady() is called and videojs then does a techCall and the html5 element hasn't been created yet --> this.el_ is undefined.

Possible solutions

  1. Keep as it is (unified) and create a seperate repository with the echo tech
  2. Create a repository for each tech (html5, flash and echo)
  3. Create a class to inherit from with functions that, if unimplemented, throw an exception e.g
videojs.AbstractTech = videojs.Component.extend({
  // ...
});

videojs.AbstractTech.prototype.buffered = function(){
  throw "'buffered' not implemented. Implement this to return a TimeRange representing buffering progress";
};
  1. A mix of 2 and 3: An "abstract" base class and seperate projects. Echo to create logs for an example flow of method calls and HTML5 and Flash.

Maybe even a two different base classes could exist for synchronous tech and asynchronous tech. It could make integrating external APIs (vimeo, dailymotion, soundcloud and youtube are all async) a breeze and really appealing to integrators.

@heff
Copy link
Member

heff commented Dec 11, 2013

These are good thoughts. I'd like to make a lot of improvements to techs and the tech building process for version 5.

RE: AbstractTech, the MediaTechController might be what you're looking for. Or at least it's meant to do something similar.
https://github.com/videojs/video.js/blob/master/src/js/media/media.js

@ghost
Copy link
Author

ghost commented Dec 19, 2013

I check the file out and found the "not implemented" messages.
When I acquire the knowledge to split the techs from videojs I'll update this. I'm struggling with the symbol exports at the moment as it is an iterative job to find which ones are missing. Plus I'm very new to node.js and javascript testing.
Please bear with me :)

@heff heff added this to the Techs 2.0 milestone Apr 25, 2014
@heff
Copy link
Member

heff commented May 20, 2014

I've added this to the "Tech 2.0" milestone.
https://github.com/videojs/video.js/issues?milestone=5&state=open

@gkatsev gkatsev added documentation and removed docs labels Feb 3, 2016
@videojs videojs deleted a comment Oct 18, 2017
@gkatsev gkatsev added the pinned Things that stalebot shouldn't close automatically label Jul 2, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
confirmed documentation enhancement pinned Things that stalebot shouldn't close automatically
Projects
None yet
Development

No branches or pull requests

3 participants