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.

Sign up for GitHub

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

Jump to bottom

Flash Player Events Data #2748

Closed
wants to merge 3 commits into from
Closed

Flash Player Events Data #2748

wants to merge 3 commits into from

Conversation

alex-phillips
Copy link
Contributor

These changes add the textdata event emitted from the Flash module (PR: videojs/video-js-swf#188). In order for this event to receive the text data from the SWF, this change also passes all additional parameters from the triggered event into the handler in the JS.

@pam
Copy link

pam commented Oct 27, 2015

Tests failed. Automated cross-browser testing via Sauce Labs and Travis CI shows that the JavaScript changes in this pull request are: BUSTED

Commit: a4b4f9a52a845440a480ec7e0e4873f1c0f8cc65
Build details: https://travis-ci.org/pam/video.js/builds/87662071

(Please note that this is a fully automated comment.)

@alex-phillips alex-phillips reopened this Nov 17, 2015
@alex-phillips
Copy link
Contributor Author

Can we get this test to rerun without pushing a commit? I'm not sure the failure was related to my code change.

@gkatsev
Copy link
Member

gkatsev commented Nov 17, 2015

Yes, though, I'm not sure I want to take on the maintenance for this since we're trying to move away from flash as much as possible.

@pam retry

@alex-phillips
Copy link
Contributor Author

I totally understand, but it really is a only a single event passed up from the SWF though and the possibility of other arguments passed up (otherwise, undefined or null). Would it really be that much more maintenance?

@alex-phillips
Copy link
Contributor Author

Seemed to fail again. I have a BrowserStack account of my own. I should be able to run these tests on my own machine as long as I export the proper ENV variables, correct?

@gkatsev
Copy link
Member

gkatsev commented Nov 17, 2015

@pam retry

@gkatsev
Copy link
Member

gkatsev commented Nov 17, 2015

Now it's trying again.

@pam
Copy link

pam commented Nov 17, 2015

Tests failed. Automated cross-browser testing via Sauce Labs and Travis CI shows that the JavaScript changes in this pull request are: BUSTED

Commit: a4b4f9a52a845440a480ec7e0e4873f1c0f8cc65
Build details: https://travis-ci.org/pam/video.js/builds/91634231

(Please note that this is a fully automated comment.)

@alex-phillips
Copy link
Contributor Author

@gkatsev is there any way to run the coveralls test locally? As I said, I have my own BrowserStack credentials but if I attempt to run it I get the following output:

Running "coveralls:all" (coveralls) task
>> No src files could be found for grunt-coveralls
Warning: Task "coveralls:all" failed. Use --force to continue.

Aborted due to warnings.

Just a little information on why we're wanting this feature: this has great value, especially for broadcasters (we are a news station and are working to integrate this player as our new video player) who, by law, have to provide closed captions for live streams, which is currently done through RTMP in our system.

@gkatsev
Copy link
Member

gkatsev commented Nov 17, 2015

Actually, it's failing because of the various warnings. If you rebase against master it should succeed.

@pam
Copy link

pam commented Nov 17, 2015

Tests passed. Automated cross-browser testing via Sauce Labs and Travis CI shows that the JavaScript changes in this pull request are: CONFIRMED

Commit: 7a7ef14de0b8ba031441239f2ea9e4f5884514e1
Build details: https://travis-ci.org/pam/video.js/builds/91653286

(Please note that this is a fully automated comment.)

@alex-phillips
Copy link
Contributor Author

@gkatsev Awesome, thanks for the help. For what it's worth, this goes with the video-js-swf PR here: videojs/video-js-swf#188

@alex-phillips
Copy link
Contributor Author

Is there any update on if this could get merged in along with its companion SWF PR? It's extremely important as broadcasters to have flash captions available to comply with legal regulations for our video streams. Thanks!

@alex-phillips
Copy link
Contributor Author

Is there any update on this?

@gkatsev
Copy link
Member

gkatsev commented Jan 12, 2016

Sorry, with the holidays and everything it's been busy and hectic. We'll try to get to this soon. Thanks.

@alex-phillips
Copy link
Contributor Author

Completely understand, just didn't want this to get lost. Thanks for the response!

@forbesjo forbesjo mentioned this pull request Jan 21, 2016
Merged
@alex-phillips
Copy link
Contributor Author

Any update on this?

@gkatsev
Copy link
Member

gkatsev commented Jun 20, 2016

@alex-phillips hey, sorry for the super long wait, it keeps getting away from us. Any ideas on how to test this? I'm not sure how to create an rtmp stream with textdata.

@gkatsev
Copy link
Member

gkatsev commented Jun 21, 2016

Got it working by using the trial wowza server. Couldn't really get it any other way.

@gkatsev gkatsev added confirmed minor This PR can be added to a minor release. It should not be added to a patch release. labels Jun 21, 2016
gkatsev
gkatsev reviewed Jun 21, 2016
View reviewed changes
src/js/player.js
@@ -2767,6 +2776,8 @@ Player.prototype.options_ = {
*/
Player.prototype.handleLoadedMetaData_;

Player.prototype.handleTextData_;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add a jsdoc comment here, similar to how loadeddata is below, for example.
I can add it myself when I'm merging if it isn't done by then.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gkatsev Added the JSDoc. Thanks!

@gkatsev
Copy link
Member

gkatsev commented Jul 18, 2016

The swf part of this has finally been released as 5.1.0, would you be able to update the dependency to point to that?
Thanks!

@alex-phillips
Copy link
Contributor Author

@gkatsev Version's been updated. Thanks!

@gkatsev
Copy link
Member

gkatsev commented Jul 18, 2016

@alex-phillips cool! Did we lose the jsdoc comment?

@alex-phillips
Copy link
Contributor Author

@gkatsev D'oh, missed that during the merge. Fixed!

@gkatsev
Copy link
Member

gkatsev commented Jul 18, 2016

It happens. Thanks.

@misteroneill
Copy link
Member

LGTM

@gkatsev gkatsev closed this in e7ca49e Jul 18, 2016
@gkatsev
Copy link
Member

gkatsev commented Jul 18, 2016

It's merged! Thanks @alex-phillips.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
confirmed minor This PR can be added to a minor release. It should not be added to a patch release.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@alex-phillips @pam @gkatsev @misteroneill