These are chat archives for AxisCommunications/locomote-video-player

24th
Nov 2014
Gaétan Collaud
@gaetancollaud
Nov 24 2014 08:58
Hello, I corrected the pull request for the RTMP streams
I changed the two parameters in one single Object or String parameter
AxisCommunications/locomote-video-player#109
Sorry
My master branch was behind yours and synce I had some pull request I didn't resynchronize
Alexander Olsson
@noseglid
Nov 24 2014 09:22
@gaetancollaud I think you misunderstood me. I don't think the class in com/axis/http/url.as should have any conceptual knowledge of "stream". It is a URL parser, and there is no such concept in an URL such as streamName.
I think that if the "url" is an object. It should be passed, unaltered, to the client. This means that there would have to be a safe-guard in url.parse against objects. E.g. something like if (!url is not string) return url. Then rtmpClient would have to parse the two differents parts as makes sense for RTMP.
Gaétan Collaud
@gaetancollaud
Nov 24 2014 09:39
Ok, so yes, I missunderstood you
So, if Player.play receive a string, we pass it through the url parser. If it's an object, we pass it directly to the client.
Is that correct ?
About the object, what should its attributes be ?
{ connect:'', streamName:''} and that's it or should we take the same attributes as the urlparser give ?
{basename, protocol, host, port, basepath} ?
Alexander Olsson
@noseglid
Nov 24 2014 09:55
Nah, I think it should be fine to mirror the API of the NetStreamclass, so connect and streamName would be cool
Hmm, what you said might be tricky though since we determine which client to use based on the initial part of the string.
Gaétan Collaud
@gaetancollaud
Nov 24 2014 09:57
So when the RTMPClient receive an object, how does it know that it comes from the urlParser or directly from the player ?
Should I just check the properties ?
Or should I add an id on the object
{source : 'urlParser'} or {source: 'direct'} ?
Alexander Olsson
@noseglid
Nov 24 2014 09:59
Hmm... maybe we should revisit the idea of having multiple arguments again. I'm not sure which is best here. Both solutions are kind of awkward at best.
Gaétan Collaud
@gaetancollaud
Nov 24 2014 10:01
I think we should go through the url parser any way because the "connect" parameter has a port, a protocol, a host, etc.
Maybe it's wrong to pass the streamName
Maybe we should only pass the "connect" or "url" to the url parser
and add the "streamName" after the parser
Alexander Olsson
@noseglid
Nov 24 2014 10:08
That's not bad. If you can pass an object like { url: "rtmp://...", streamName: "asdf" }, and Player.as can verify if the passed argument is an object (and pass object.url to urlparser), otherwise pass just the string-url parameter.
Then Player.as could extend the urlParsed object with all other parameters in the object passed to play.
Gaétan Collaud
@gaetancollaud
Nov 24 2014 10:09
Is that ok for you ?
Pseudo code something like that. What do you think?
Gaétan Collaud
@gaetancollaud
Nov 24 2014 10:13
Why do you set in.url to null at line 4 ?
Alexander Olsson
@noseglid
Nov 24 2014 10:14
So not to add it when extending urlParsed (the original full URL is already added by url.parse.
(it's probably not valid actionscript - there might be another way to extend an object with all parameters 'except' some)
Gaétan Collaud
@gaetancollaud
Nov 24 2014 10:20
can't we just do something like that : https://gist.github.com/gaetancollaud/a29a7d6d7520d0ec8caf
Alexander Olsson
@noseglid
Nov 24 2014 10:24
The downside is that it only works with streamName. But it might be enough for now.
Wouldn't work of another client required a different custom parameter
But as I said. Probably good enough for now
Gaétan Collaud
@gaetancollaud
Nov 24 2014 10:24
I see
ok, I will make a first pull request with only streamName
Alexander Olsson
@noseglid
Nov 24 2014 10:38
Make sure you base your changes on the latest master so they can be merged.
oups, sorry I commited the player.swf and locomote.min.js
I think this is why the merge fail
why do you put the dist directory in the source ? Why don't you put it only in the release ?
Alexander Olsson
@noseglid
Nov 24 2014 12:05
I'm intending to only build and commit these on release tags - to avoid just the issue you're describing.
Gaétan Collaud
@gaetancollaud
Nov 24 2014 12:07
You just need to add the directory on the .gitignore file and that's it
Now that you have releases you just have to put a link in the readme and the people can use it directly
Alexander Olsson
@noseglid
Nov 24 2014 12:07
Yar, but when i run 'gulp dist' (which make a new release), it should add and commit them with the release tag.
Gaétan Collaud
@gaetancollaud
Nov 24 2014 12:07
And is it possible to add the locomote.js and locomote.map in the release too ?
For debuging purpose
Alexander Olsson
@noseglid
Nov 24 2014 12:08
(they must be committed in this case if they're to be used with bower)
Gaétan Collaud
@gaetancollaud
Nov 24 2014 12:08
ok I see
Alexander Olsson
@noseglid
Nov 24 2014 12:09
Yupp, that would be reasonable I think. Often you don't want to include the minified version at all, since if you're building a web-app, chances are you're already minimizing yourself so it would be more valuable with the original source for debugging purposes.
Gaétan Collaud
@gaetancollaud
Nov 24 2014 12:09
For the moment I copy manually the source js. It's not a big deal, but could be helpful to have directly in the same folder
Did you check my pull requests already ? I'ts the end of the sprint for us and I won't be able to work on this until the next Monday. It could be great if we can close this today.
This way I can destroy my fork and recreate it from the head of your master branch
Alexander Olsson
@noseglid
Nov 24 2014 12:31
I'm not gonna be able to look at it right this moment unfortunately. I'll try and get around to it asap though.
Gaétan Collaud
@gaetancollaud
Nov 24 2014 12:32
Too bad
See you soon
Alexander Olsson
@noseglid
Nov 24 2014 12:51
@gaetancollaud The changes in url.as seems totally useless :)
Other than that it looks good. If you rebase it towards latest master, then it can be merged.
Gaétan Collaud
@gaetancollaud
Nov 24 2014 14:40
yeah I logged something because I don't have the flash debugger
Ok I will do that
Gaétan Collaud
@gaetancollaud
Nov 24 2014 15:11
I'm struggling with the rebase... How do I do that ?
because my master branch is ahead of yours
should I create a branch from your master and start again from that branch ?
Alexander Olsson
@noseglid
Nov 24 2014 15:13
git fetch origin
git rebase origin/master
<solve conflicts>
That's how I would do it.
Gaétan Collaud
@gaetancollaud
Nov 24 2014 15:14
this is what I do, but it keep my master branch changes
I will create a branch from your master, it will be easier
Gaétan Collaud
@gaetancollaud
Nov 24 2014 15:21
AxisCommunications/locomote-video-player#113
pfiou, it works
Gaétan Collaud
@gaetancollaud
Nov 24 2014 15:28
AxisCommunications/locomote-video-player#114
same thing
once your merged all my pull request I will destroy my fork and create a new one from yours
Gaétan Collaud
@gaetancollaud
Nov 24 2014 15:38
And I will implement something for the mouse event because we will need it for the next sprint
Alexander Olsson
@noseglid
Nov 24 2014 15:53
@gaetancollaud The rest of the code uses more spaces when writing conditionals (e.g. if (cond) {} rather than if(cond){}). If allright with you, I will merge this and make a commit which fixes this?
Gaétan Collaud
@gaetancollaud
Nov 24 2014 15:58
Yes it's ok for me
So for the next time you want me to put more space in the condition ?
Alexander Olsson
@noseglid
Nov 24 2014 15:58
Excellent. Thanks for the contribution!
Gaétan Collaud
@gaetancollaud
Nov 24 2014 15:59
Sorry I don't use flash builder
I use netbeans
And I'm afraid to use the formatter because it will mess everything.
Alexander Olsson
@noseglid
Nov 24 2014 15:59
I'm planning on adding a code style checker. I don't really have the "style" written down so I can't really require anyone to follow anything specific.
That's allright, I use githubs 'Atom' editor myself
Gaétan Collaud
@gaetancollaud
Nov 24 2014 16:00
^^
Alexander Olsson
@noseglid
Nov 24 2014 16:00
(recently switched from Vim)
But for now, I will just "correct" the code-style after the merge, so I'm not gonna be too picky about that.
Gaétan Collaud
@gaetancollaud
Nov 24 2014 16:00
ok
Alexander Olsson
@noseglid
Nov 24 2014 16:00
Assuming it's okay by you
Alexander Olsson
@noseglid
Nov 24 2014 16:19
@gaetancollaud So, it doesn't really work.. It appears as variable is Object holds true if variableis either String or Object. Reversing the logic should work though, and test for param is String instead (which is false for an Object). Basically every class inherits from Object.
Also, the new URL will never be parsed if there currently is a client running. The parsing needs to be moved above the if (client) { statement.
Gaétan Collaud
@gaetancollaud
Nov 24 2014 16:20
Sorry I took your snipet without really testing
I just tested rtmp
Alexander Olsson
@noseglid
Nov 24 2014 16:20
It doesn't work with a string at all now :fire:
Gaétan Collaud
@gaetancollaud
Nov 24 2014 16:22
yep, I will put the parser before the if(client)
done
It works, I just tested (rtmp and http streams)
Alexander Olsson
@noseglid
Nov 24 2014 18:38
Did you also try doing a play, while another stream was playing?
Gaétan Collaud
@gaetancollaud
Nov 24 2014 20:47
I never do that in my application. I always invoke stop before play something else, so no I didn't try that
Regarding the previous implementation it should works.
BTW, use return in the middle of a method is not recommended because you can miss it (like I just did). Using if else statement is better for reading the code.