Added support for AMD in build process

[palmerj3]

Build process now supports AMD.

Example usage:

(function() {
  'use strict';

  require(['node_modules/shaka-player/amd'], function(shaka) {
    shaka.polyfill.Fullscreen.install();
    shaka.polyfill.MediaKeys.install();
    shaka.polyfill.VideoPlaybackQuality.install();

    var player = new shaka.player.Player(document.getElementById('foo'));
    var playerSource = new shaka.player.HttpVideoSource('http://video.webmfiles.org/elephants-dream.webm');
    player.load(playerSource).then(function() {
      player.play();
    });
  });
}());

[sanbornhilland]

@palmerj3 @joeyparrish

Continuing the discussion from #35 . There I said that I had no problem running @palmerj3's solution but was getting errors when trying to load shaka as an AMD module inside our project. I realized that the working solution loads and Http video source whereas we are loading a Dash video source. If you modify the solution to be this:

require(['./amd.js'], function(shaka) {
// shaka = shaka.shaka;

    shaka.polyfill.Fullscreen.install();
    shaka.polyfill.MediaKeys.install();
    shaka.polyfill.VideoPlaybackQuality.install();

    var player = new shaka.player.Player(document.getElementById('video'));
    var playerSource = new shaka.player.DashVideoSource('http://shaka-player-demo.appspot.com/assets/car-20120827-manifest.mpd');
    player.load(playerSource).then(function() {
      player.play();
    });
  });
}());

then it fails. It gives me that 'cannot read addEventListener of undefined' error that I was getting before.

So it appears that it's not an issue with our source but that exporting shaka as an AMD module and trying to load a Dash source is not working at the moment. Is a module export something that you'd like to support officially? Is it worth registering this as a bug?

[palmerj3]

@sanbornhnewyyz so in my project I am actually utilizing both the HTTP source and the DASH source but switching them based on the content. Both are working fine for me in a CommonJS environment. I'll do a bit more testing to see if there are any obvious issues in a AMD environment but from my local testing I don't see any of the errors you mention when playing back Dash content. However that content I'm playing is not DRM'd so perhaps that's a code path I haven't tested yet in AMD or CommonJS. Is that a requirement for your project?

At this point I'm wondering if perhaps shaka is being garbage collected in your environment... Are you able to create an outside-the-closure reference to test this out? I'll do some more testing later and see if I see any issues myself.

[joeyparrish]

It appears that the TypeError is caused by a bug in the master branch. EWMABandwidthEstimator should be exported, but is being stripped as dead code since it is now only referenced at the application level.

[joeyparrish]

@palmerj3 are you using the 1.2.x branch or master? The TypeError bug described by @sanbornhnewyyz is only present in master. (Introduced in 9c0d2db.)

[joeyparrish]

@sanbornhnewyyz: TypeError fixed in 4571061.

[joeyparrish]

Rather than building amd.js/index.js/index2.js, etc., I've put together a new wrapper template that works for commonJS in node, requireJS in node, requireJS in a browser, and direct use in a browser. I've tested in those 4 scenarios and everything seems to be working. Should have the change up on github soon.

[sanbornhilland]

Great. Thanks for moving so quickly on this. Importing as an AMD module works fine now. Though shaka is still getting attached to the window as far as I can see. Is there any way around that?

[sanbornhilland]

Sorry, just a quick follow up to my last comment.

Using @palmerj3 's amd.js build, shaka gets attached to the window. Using @palmerj3's commonJS build and then wrapping it in a define() requireJS define function also works but does not attach shaka to the window.

[joeyparrish]

I have a solution in review internally which does not attach to the window when using commonJS or requireJS.

[joeyparrish]

@palmerj3 @sanbornhnewyyz Please check out the latest in master. A new wrapper template was committed in 5f3a153. If there are no objections, I will cherry-pick this into the 1.2.x branch as well.

[palmerj3]

Just tried out the latest master with these MPD samples:
http://dash-mse-test.appspot.com/media.html

And it doesn't seem to be working. Works fine with HttpVideoSource however the DashVideoSource is giving me a few errors:

1. Refused to get unsafe header "Date"
2. TypeError: Cannot read property 'addEventListener' of undefined {stack: (...), message: "Cannot read property 'addEventListener' of undefined"} ---> line 12 of compiled file

Any ideas?

[sanbornhilland]

Just tried the master branch build and got the same TypeError as @palmerj3. Shaka gets imported fine but then on Dash playback it seems like the same error that got fixed in 4571061.

[joeyparrish]

I'm not getting this TypeError. Can you please get the value of shaka.player.Player.version when one of these errors occurs?

[sanbornhilland]

@joeyparrish: v1.2.0-156-ga1c0e44

[joeyparrish]

@sanbornhnewyyz, what's the most recent commit you have from me?

I'm unable to reproduce this in the test app in compiled mode, nor in a separate app using requireJS. Can either of you provide me a sample which reproduces this problem?

Here's the source to my working requireJS sample: https://gist.github.com/joeyparrish/64e3f0a1231f3ea382b9

I tried all of the YouTube test content @palmerj3 linked to. The manifest for oops_cenc is a 404, but all of the others play in the gist linked above.

[sanbornhilland]

@palmerj3

Okay, looking @joeyparrish's gist, I think the cause of the problem is that I am not passing an estimator into the source constructor. I suspect that this is why @palmerj3 is getting the same errors, because the basic test app I was using was based on the one he posted above. If I pass null in @joeyparrish's gist instead of an estimator, then I get the same errors.

So can you just clarify that? Now we have to explicitly create and pass an estimator?

[joeyparrish]

Yes, that is new for v1.3.0, introduced in 9c0d2db. The purpose is two-fold. For one, passing an estimator lets you inject your own implementation of IBandwidthEstimator without modifying the library. For another, it makes it possible to persist bandwidth information across playbacks, which means that the second video you stream can start in HD if you have enough bandwidth for it.

[palmerj3]

Ok will try that. In the future would be good to increment the major version for breaking API changes. Until now I've passed null in as the second param and it worked fine - perhaps that wasn't supposed to be the case?

[joeyparrish]

Well, v1.3.0 isn't complete yet. I did not intend to break compatibility quite so hard. :-) I will make the parameter optional, and if null, we will create an instance of the default implementation.

[joeyparrish]

After 44b975d, defaults to a new instance of EWMABandwidthEstimator() if not supplied by the caller.

[sanbornhilland]

This is great, thanks!

[joeyparrish]

If there are no objections, I am going to consider this resolved. Please feel free to open a new issue if there are any remaining issues with module support. And thanks again for helping get commonJS and requireJS supported.