Closed Afterster closed 9 years ago
I have given this the initial eyeball review and on the whole it looks fine.
I see the use of on()
in the code. I am happy to keep it, but that does change the whole of jPlayer core from jQuery 1.4.x compliant to jQuery 1.8.x (I think is the version.) I'd follow up this merge with a spring clean of the event handlers so that they all use on() for the interface and other appropriate events.
The amount of code added appears acceptable. See below.
I know I may appear a little slow right now, but I want to bring in all the changes that I've made from the dev branch before making this merge. And I also have the website to get there too. So bear with me. Your contribution is appreciated. It should just fine since I moved all the existing repo files through git
but only time will tell :smile: Oh, and I'd need to test this too of course before the merge.
This reminds me of a previous suggestion that made the internal players more generic. So we have a "jPlayer player plugin" for each solution. Then we can have various benefits and just swap the pointers around and have a common structure to play and pause and so on. This may also play nice with possible build options to optimise the code size.
jQuery 1.7 for the on() but that is fine. I have been thinking of doing it before, since the playlist code uses on() already.
Thanks for the review and your roadmap summary. I will keep an eye on your progress ;)
I used on() because of playlist code in fact, I didn't noticed about the minimum version requirement change. If you want to bump minimum jQuery version to 1.7 that's fine then. About "jPlayer player plugin", this is exactly what I was thinking about when implementing Aurora.js solution. I didn't did it to not move away from the original pull request purpose but it should be something to put on the roadmap. Current code is already almost a plugin, it can be done without much effort. I will take care of this once merged if you want, when I have some time.
Looking at this next... I think I'll add most of your 1st post to the readme while merging this PR. I will also prepare a demo ready for it too. This will mean copying in those aurora files to my /lib/ folder in the repo so that the examples will work. Plus I'm wondering how to describe the dependancy in the JSON files describing the package... jQuery is required, but these are an optional extra with conditional dependancy.
I'm have a merge conflict problem, but that is not the strange part. It is trying to merge your changes with the copy of the source file we put into the dist folders. There should not be a relationship between that file in dist, since it is a new file. During my refactor I took care to always move the existing files through git so the relationship was maintained. I have not run into this issue with any of the other PRs over the past few days. Hmm... I am merging into the dev branch, but that should be the same as master, with that fix for the IE fullscreen already and then differs only by that composer config file.
The conflict was in the comments and was easy to fix. I changed my comment about the SWF filename and you change the comment about the solution option. They were on different lines, but it threw a conflict.
Would you be able to fix the pull request so that it affects the correct file?
Your changes should point at the source here:
/src/javascript/jplayer/jquery.jplayer.js
I do not know how that went wrong. It looks like your PR points at the old file in the repo that I git mv
to the new structure. So it should have worked.
Any ideas?
Ok :) I rebased my branch on your dev branch and recommit on /src/javascript/jplayer/jquery.jplayer.js Do not merge this PR through github (github still targets master branch) but do it manually on dev. See https://github.com/Afterster/jPlayer/compare/happyworm:dev...origin/aurora.js for the real change list.
About the composer dependency, I never maintains it (it's also something in my todo for some projects ^^), @thormeier?
I will give it a try again now. I do not use the merge pull request button on githib.com preferring to manually do it and review and diff before I merge it in. It helps catch stuff like yesterday.
I think for the dependancy, we just leave aurora off the list and add it to the documentation. I am referring to all 3 JSON file, package.json, bower.json and composer.jons.
Cheers.
@Afterster this merge is looking fine. Thank you for sorting it out.
I am in the process of making a demo and adding the Aurora files to the lib folder, when I noticed your support list and that AAC seemed to be missing for the m4a format and then there are 2 formats, Opus and OGG, for the oga format. The mp3 and flac formats line up.
And while trying to make a sensible demo for this, I wonder if we should show a single FLAC media file being used, and how jPlayer tested the HTML and Flash before deciding that the Aurora solution could play it. If we use mp3 then the html or flash would be used...
Hmm... I think I'll create a new batch of demos for this, they will be like demo-01 and the main one will be like I described above for Flac and jPlayer picking Aurora to play it. Then have some others forcing Aurora and mp3, and ogg and so on so we can review each of the Aurora players specifically.
OK I see now that the command AV.Player.fromURL(url) appear to deal with all that.
I also note that some codecs are built in, but it is the WAV codec and some others I'm not familiar with: By itself, Aurora will play LPCM, uLaw and aLaw files in a number of containers. Be sure to add additional codec support by including some of our other decoders.
So we could add wav to the list of formats in the defaults for Aurora. I am also considering trimming that list down, to just "wav mp3" or maybe only "wav" since that is what aurora core supports.
These Aurora codecs do not seem to like each other very much. Including aac.js was breaking the ogg.js and then after removing aac.js, the vorbis.js was not playing our OGG files very well at all. After that, I started only including the codec I was interested in. The mp3.js worked well until you change the currentTime and then it dies. The acc.js seemed to work the best.
I've not got round to testing the flac yet. I'll make some flac files to test it with. Not tested with opus either... But I am hoping the flac works well, and then I will only demo that. I suspect that codec works pretty well since browsers do not support it.
The seek()
method you use is not in their main API docs and I suspect that we are 1000 out in the playHead() code. That would suggest the aac.js seek() code is working in seconds, not milliseconds. I will see if it helps.
Indeed, I also noticed some codec conflict, sometimes this conflict are better handled according to the browser used. You're right about wav support, I completely forget it whereas it is explicitly quoted... About seek method, decoder are excepting a seek frame but we are calling the aurora player seek function which except milliseconds on my understanding, see https://github.com/audiocogs/aurora.js/blob/master/src/player.coffee#L98. But yes, this is not documented.
I'm finding that Decoder.seek() is causing problems with the object it returning being undefined.
At this point, I'd be happy if we can get a demo working. The only one that works is the AAC - with the progress bar too - many of them play - but then break if you click the progress bar - but the ACC one is flawed due to sending seconds from jPlayer and their AAC must have that error... But that nuts because it is the only one that actually works with seek LOL.
Calling it a night.
I begin to understand why it is not documented :smile: . I believe you're using firefox for your seek test? Chrome is handled it better on my test.
Nope, I was using Chrome.
All i did was press play, listen to a bit of music play and then click on the progress bar. Instant death.
ATM, I am starting to question the need to add this player. Why did you add it?
This may be added as an experimental unsupported feature that we do not recommend anyone uses...
So why bother putting it in if it is such a knife edge whether it works or not.
The main issue is on seeking, otherwise streaming and other interactions are working fine on my side. I added this to avoid server transcoding when you cannot use flash player or attempt to stream an unsupported format on html5/flash (flac for instance). I agree current Aurora.js release is unstable on some features. If you don't plan to add it by default, maybe we could create this plugin architecture first and then create an experimental Aurora.js plugin?
I was just getting a bit stressed, since i no longer have any time to do this. I will merge with the project and then we can fix it on route. I will make a demo, and explain that Aurora.js is an experimental feature, with limitations.
I think I'll leave the Seek in seconds like you had it, since the only demo that works is AAC so I can have something to show people.
Ok good. Then like I said I will take care of the plugin refactoring in the coming weeks if you are interested about, it could be a good way to separate stable feature from experimental ones.
That sounds good. I should have some time in the last week of December to look at merging a solution plugin system. Maybe if you make a pull request once you have something, we can begin reviewing and revising its requirements at the weekends, with the aim of merging sometime in late december.
For example:
The bit I do not like is how the solution data is written directly to the jPlayer object. The html
and flash
solutions did not clash, but a plugin called setMedia
would be bad... But I suppose you could argue it would be rather stupid to name your plugin that, but what about all the other obscure internal methods and properties. Point being, the solution data should be moved to a new object for the purpose of holding all the solution plugins.
this.player = {};
// ...
plugin: function(plugin) {
if(typeof plugin !== 'object') {
return;
}
if(plugin.type === "player") {
this.player[plugin.name] = plugin;
// do the other stuff to register a player plugin.
}
Oh and try to follow the jshint rules in the .jshint files. We conform with all of the jQuery ones, except the double quotes rule. I try and use doubles now for new additions, but as you may notice, I forgot just now in that example for the 'object' string.
grunt test
to jshint all the js files.
Yes I agree, the plugin should be an independant instance. I'm not familiar with jshint rules but I will take a look at it. As you will probably close this issue once Aurora.js merged, I think we should continue the plugin discussion on another thread.
I've started #266 for the player plugin discussion.
This thread will close when dev branch is merged with master.
This add Aurora.js solution to jPlayer, like are html / flash solutions. I personally did this for the Ampache project to avoid server transcoding when it is only used because of browser compatibility issues.
To test it, get the branch and set the following jPlayer options:
Then include Aurora.js scripts / decoders:
I added a new auroraFormats option because Aurora.js can be extended with new decoders and we cannot determine which decoder manage which format from the js api. Aurora.js provides a limited set of events, I will attempt to pull request on Aurora.js to improve their api but better to talk here before ; if this has some interest.