Skip to content
This repository has been archived by the owner on Dec 10, 2020. It is now read-only.

Updated RTMP provider. fixes #8 #17

Merged
merged 1 commit into from
Jul 16, 2013
Merged

Conversation

iamjem
Copy link
Contributor

@iamjem iamjem commented Jun 26, 2013

I've added support for RTMP playback. I also have a pull request I'll send in the next day to the video-js project for the same.

These additions I actually made last year and have been using in production on several sites without any issues so far.

I'm not a seasoned actionscript developer, but this also wasn't that complicated. I have no doubt there's more elegant ways of handling some of my bandaids. The biggest obstacle I encountered with the RTMP playback is that stream events do not behave the same as progressive playback, so external events wouldn't trigger when needed, or even at all.

@iamjem
Copy link
Contributor Author

iamjem commented Jun 27, 2013

I've submitted a pull request to video-js as well, which can be viewed here:
videojs/video.js#605

@jrw95
Copy link
Contributor

jrw95 commented Jul 8, 2013

Hello,
Can you point us toward one or two sites that are using this?

@iamjem
Copy link
Contributor Author

iamjem commented Jul 8, 2013

The only live sites I've used this on were using the older version of video.js (3.2.x). The codebase has changed significantly since then, and I also have a custom configuration class that all of my sites use to load player configuration from a centralized JSON file. Implementation-wise it would be of little use to you.

I'd recommend following the video-js issue link in my comment above to see implementation details, and you can checkout the branch of video-js containing the feature to play with the code yourself.

@bdeitte
Copy link
Contributor

bdeitte commented Jul 10, 2013

We've been testing this pull request out at Brightcove, and so far it looks good. We're going to do a bit more testing and then will see if Steve will pull it in.

One thing we'd like to change before it gets pulled in, which I'll look at here sometime soon if no one else does, is to slightly alter the URL delimiter setup:

  1. If a "&" isn't found, just use the last "/" as the delimiter.
  2. Use "&" as the delimiter.

@heff
Copy link
Member

heff commented Jul 10, 2013

We could also change that as a patch after this gets pulled in. Unfortunately the last pull request caused some merge conflicts that need to be resolved for this one to be pulled in. Would anyone be able to do that?

@bdeitte
Copy link
Contributor

bdeitte commented Jul 10, 2013

Ah, so the VideoJS.swf removal caused a problem with merging.

I stand corrected on this looking good in testing- not sure what's up now but having some difficulties that we'll update about here when possible.

@iamjem
Copy link
Contributor Author

iamjem commented Jul 10, 2013

We're doing some QA on the player right now as well before pushing out on some new projects. If you have any direction on what you're seeing problems with I can try and fit that into our QA cycle.

@bdeitte
Copy link
Contributor

bdeitte commented Jul 10, 2013

Thanks Jeremy. I was out for most of the day, so I didn't hear what the problems were or where things ended up today. I'll follow up tomorrow when I learn more.

Also, I never mentioned this earlier, but I did do a code review as well and things looked fine to me.

@bdeitte
Copy link
Contributor

bdeitte commented Jul 11, 2013

Sorry I forgot to follow up here Jeremy, but you'll see in the other pull request that we've integrated these changes and added just a small item. Looks good to me!

@heff heff merged commit 4726e82 into videojs:master Jul 16, 2013
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants