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

Seekable from source handlers #2376

Closed
wants to merge 2 commits into from
Closed

Seekable from source handlers #2376

wants to merge 2 commits into from

Conversation

dmlap
Copy link
Member

@dmlap dmlap commented Jul 17, 2015

@pam
Copy link

pam commented Jul 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: fbac569f083ab179380831ed48ee9421fe6f1f18
Build details: https://travis-ci.org/pam/video.js/builds/71475939

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

@heff
Copy link
Member

heff commented Jul 17, 2015

@dmlap are you just not running tests before submitting PRs or is there something else going on?

@dmlap
Copy link
Member Author

dmlap commented Jul 17, 2015

Argh... ran the tests in the browser but forgot to check my other tab for jshint results. Clearly I am still adapting to the new workflow :/ Will update, sorry.

@dmlap dmlap force-pushed the seekable-from-source-handlers branch from fbac569 to a5ef4bb Compare July 17, 2015 20:46
@pam
Copy link

pam commented Jul 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: a5ef4bb
Build details: https://travis-ci.org/pam/video.js/builds/71480288

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

@heff
Copy link
Member

heff commented Jul 17, 2015

Why do we need to clamp seeks before it goes to the swf? Will that affect live at all, since seekable will be zero to whatever duration is for live?

heff
heff reviewed Jul 17, 2015
View reviewed changes
test/unit/tech/flash.test.js
@@ -30,14 +31,17 @@ test('currentTime', function() {
// Mock out a Flash instance to avoid creating the swf object
let mockFlash = {
el_: {
vjs_setProperty: function(prop, val){
vjs_setProperty(prop, val){
Copy link
Member

Choose a reason for hiding this comment

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

Wait what? We can use this outside of classes?

Copy link
Member Author

Choose a reason for hiding this comment

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

:)

Copy link
Member

Choose a reason for hiding this comment

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

These are shorthand for functions. They're different from the methods in classes. Notice how there's a comma between them but not in classes.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, I see.

@heff
Copy link
Member

heff commented Jul 17, 2015

A few questions inline but looks good to me otherwise.

@dmlap
Copy link
Member Author

dmlap commented Jul 17, 2015

This is another variation of the problem where the SWF has no concept of appropriate seek values for MSE-style content. Steps 6-8 of the seeking algorithm specify that seeks should be a non-erroring no-op if there are no seekable ranges for a video. The previous Flash behavior would always send the seek through to the SWF, which would always trigger a seeking event, and couldn't clamp because it didn't have access to seekable. I felt it was a better to start migrating more of this logic out rather than having the SWF call back out to JS to find out the current seekable ranges.

I didn't think of live RTMP when making this change. Are you familiar with how that is supposed to work? For most live HLS, seekable isn't actually [0, live]. As the broadcast progresses, segments are typically removed from the m3u8 and seekable is bumped forward to account for that:

[player.seekable().start(0), player.seekable().end(0)]; // [0, 120]
setTimeout(() => {
  [player.seekable().start(0), player.seekable().end(0)]; // [200, 220]
}, 200 * 1000);

@heff
Copy link
Member

heff commented Jul 17, 2015

Flash.seekable gets end(0) from Flash.duration(). I'm not totally clear on what we're supporting for live in Flash, for HLS or RTMP, but if duration is ever -1 or Infinity that seems like it would screw with the time clamping here. Assuming you can still seek in that case.

@dmlap
Copy link
Member Author

dmlap commented Jul 20, 2015

According to the spec, duration should be Infinity for live streams which would definitely screw up the current default implementation of seekable. We could rethink live RTMP and get it aligned with the spec but we may want to consider deprecating the live use-case for 5.0.

@heff
Copy link
Member

heff commented Jul 20, 2015

I think deprecating live RTMP for 5.0 is reasonable request. Would this issue affect live HLS?

@dmlap
Copy link
Member Author

dmlap commented Jul 21, 2015

@heff yes, this change is to support live HLS properly.

@dmlap dmlap force-pushed the seekable-from-source-handlers branch from a5ef4bb to fdee8b9 Compare July 21, 2015 18:02
@dmlap
Copy link
Member Author

dmlap commented Jul 21, 2015

@heff changed the tests to look more like the other Flash tests. Any other thoughts on this one?

@pam
Copy link

pam commented Jul 21, 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: fdee8b9e3bddf8063d1414d3f6e5692c113bd4b7
Build details: https://travis-ci.org/pam/video.js/builds/71988095

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

@heff
Copy link
Member

heff commented Jul 21, 2015

No, this looks good to me

@dmlap
Copy link
Member Author

dmlap commented Jul 21, 2015

@pam retry

@pam
Copy link

pam commented Jul 21, 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: fdee8b9e3bddf8063d1414d3f6e5692c113bd4b7
Build details: https://travis-ci.org/pam/video.js/builds/71997086

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

@dmlap
Copy link
Member Author

dmlap commented Jul 21, 2015

Motion to ignore pam?

@heff
Copy link
Member

heff commented Jul 21, 2015

+1

@dmlap
Defer to the source handler for seekable()
f19f4f4 
If seekable is defined on the active source handler, use that implementation instead of the standard one.
@dmlap
Clamp seeks to seekable ranges
80db9d8 
Bail early (and don't trigger any side-effects) if there are no seekable ranges when setting currentTime. Clamp seeks to the interval between the earliest start and latest end position.
@dmlap dmlap force-pushed the seekable-from-source-handlers branch from fdee8b9 to 80db9d8 Compare July 21, 2015 21:04
@pam
Copy link

pam commented Jul 21, 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: 80db9d8
Build details: https://travis-ci.org/pam/video.js/builds/72017432

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

@dmlap dmlap closed this in d3d5d03 Jul 21, 2015
@dmlap dmlap deleted the seekable-from-source-handlers branch July 21, 2015 21:12
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
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@dmlap @pam @heff @gkatsev