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

Make sure it's safe to call bufferedPercent before the SWF loads #2289

Closed
wants to merge 1 commit into from

Conversation

dmlap
Copy link
Member

@dmlap dmlap commented Jun 25, 2015

Manual time tracking calles bufferedPercent() on an interval and doesn't wait for the tech to be ready. If it's called before the SWF is loaded, it will throw an error. Make sure the methods required by bufferedPercent() don't throw if they're called early. Fixes #2288.

Manual time tracking calles bufferedPercent() on an interval and doesn't wait for the tech to be ready. If it's called before the SWF is loaded, it will throw an error. Make sure the methods required by bufferedPercent() don't throw if they're called early. Fixes videojs#2288.
@gkatsev
Copy link
Member

gkatsev commented Jun 25, 2015

LGTM

@heff
Copy link
Member

heff commented Jun 26, 2015

lgtm. Should we just wrap every swf property call with a function that does this?

@dmlap
Copy link
Member Author

dmlap commented Jun 26, 2015

After writing this commit, I wondered if it would be better to just wait for tech ready to start manual time tracking.

@heff the only downside of that is we have to figure out a reasonable "default" value for each property, which isn't totally obvious (e.g. attributes that return Time Ranges, for instance).

@heff
Copy link
Member

heff commented Jul 6, 2015

Ah, yeah, manual time tracking should definitely be waiting for tech ready. Though it doesn't hurt to also have this added protection built in. Do you want to add the former first or just move forward with this? Looks good to me either way.

@dmlap
Copy link
Member Author

dmlap commented Jul 7, 2015

I'm ok with moving forward with this for now. I can file an issue to fix the latter so we don't forget about it.

@dmlap
Copy link
Member Author

dmlap commented Jul 7, 2015

I can't help but note this sort of problem wouldn't exist if the base tech handled behavior before the official ready event.

@heff
Copy link
Member

heff commented Jul 7, 2015

Yeah, I don't think anyone's arguing that's not a better direction, but what do you mean by the base tech? Should we try to make Tech handle all the defaults?

@dmlap
Copy link
Member Author

dmlap commented Jul 7, 2015

Decided to implement the more comprehensive fix in #2316, so I'm closing this.

@heff I was thinking of Tech providing a basic implementation until the ready event fires, along the lines of what we were discussing to fix the async startup issues with the DASH source handler.

@dmlap dmlap closed this Jul 7, 2015
@heff
Copy link
Member

heff commented Jul 8, 2015

@dmlap #2316 is against master while this was against stable. Did we still want to get a patch out with this fix, or no?

@dmlap dmlap reopened this Jul 8, 2015
@gkatsev
Copy link
Member

gkatsev commented Jul 9, 2015

LGTM

heff pushed a commit to heff/video.js that referenced this pull request Jul 9, 2015
@gkatsev gkatsev closed this Jul 9, 2015
@dmlap dmlap deleted the allow-buffered-before-init branch July 9, 2015 19:43
dmlap added a commit that referenced this pull request Jul 10, 2015
Delay manual progress checks until the tech is ready to avoid errors.
The Flash tech errors if buffered() is called before the SWF has loaded,
for instance.

closes #2316
fixes #2288
rel #2289
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants