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.

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

defer assigning to video.currentTime Fixes #101 #103

Merged
merged 2 commits into from
Jun 19, 2015

Conversation

nickdesaulniers
Copy link

Tested in FF (Nightly), Opera (Beta), and Safari

@nickdesaulniers
Copy link
Author

ready for rereview @joeyparrish

@nickdesaulniers
Copy link
Author

rereview? @joeyparrish

@@ -1110,7 +1110,17 @@ shaka.player.StreamVideoSource.prototype.startStreams_ = function(
// Set the video's current time before starting the streams so that the
// streams begin buffering at the stream start time.
shaka.log.info('Starting each stream from', streamStartTime);
this.video.currentTime = streamStartTime;

function setInitialCurrentTime() {
Copy link
Member

Choose a reason for hiding this comment

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

If this is run in the context of video, please annotate with @this {!HTMLVideoElement}. Alternately, use bind and run it in the context of StreamVideoSource.

@nickdesaulniers
Copy link
Author

rereview? @joeyparrish , double check the annotations, I've never written them before. Guessing they're used for closure compiler.

* @param {number} streamStartTime
* @private
*/
shaka.player.StreamVideoSource.prototype.setInitialCurrentTime = function(
Copy link
Member

Choose a reason for hiding this comment

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

Run the compiler & linter with build/all.sh. It will complain that this doesn't end in an underscore, which is the style we follow for private functions.

@joeyparrish
Copy link
Member

Thanks for the revisions. I really appreciate you working on this patch.

Fixes shaka-project#101
Required for Firefox's and IE's MediaElement implementations.
@nickdesaulniers
Copy link
Author

made use of EventManager. rereview? @joeyparrish

@joeyparrish
Copy link
Member

Looks good to me. Would you please give a quick read through CONTRIBUTING? You'll need to sign the license agreement, and add yourself to AUTHORS & CONTRIBUTORS.

@nickdesaulniers
Copy link
Author

@joeyparrish , done. rereview?

@joeyparrish
Copy link
Member

Excellent. Thanks for your contribution!

joeyparrish added a commit that referenced this pull request Jun 19, 2015
defer assigning to video.currentTime Fixes #101
@joeyparrish joeyparrish merged commit 472ec24 into shaka-project:master Jun 19, 2015
@nickdesaulniers nickdesaulniers deleted the firefox branch June 19, 2015 16:54
@joeyparrish
Copy link
Member

Hi Nick, sorry to have to say, but this patch breaks live streaming. You can see this by running this subset of the integration tests: spec_runner.html?spec=Player%20live

The symptom is that these live streams simply do not start at all, at least on Chrome. Unfortunately, I have to revert your patch. I really do appreciate you getting Shaka Player working nicely on Firefox, and I hope you're still willing to try again when you have the time.

@nickdesaulniers nickdesaulniers restored the firefox branch June 19, 2015 20:17
@nickdesaulniers
Copy link
Author

No problema, about to hop a plane for a company work week, but I'd like to see this project working in FF, even if I have to write the patches. Will take a look later.

@joeyparrish
Copy link
Member

Thanks, Nick. We want to see this working well on Firefox, too, but haven't been able to make it a priority yet to figure out why Firefox's HTML5/MSE implementation seems to behave a little bit differently from Chrome's. So your efforts are much appreciated.

@github-actions github-actions bot added the status: archived Archived and locked; will not be updated label Jul 25, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 25, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
status: archived Archived and locked; will not be updated
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants