-
Notifications
You must be signed in to change notification settings - Fork 133
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
fix(safari): initial seek time not taken into account with fromLastPosition #1548
Conversation
* To solve this, the seek should be done after readyState HAVE_CURRENT_DATA (2), | ||
* at that time the previously mentioned attributes are correctly initialized and | ||
* the range in which it is possible to seek is correctly known. | ||
* @param { string|undefined } seekingTime The wanted time to seek. If `undefined` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the JSDoc:
- Here it's
number
, notstring
- there's no space in the type annotation
- Also, there may be a dash between the variable name and its description, that I prefer to add for readability personally, as you wish
- In the param documentation, for
undefined
, I don't think just repeating the current implementation has value in this case, it just repeats what the current code does.
I'm wondering about (4), if the undefined
seeking time notion leading to a readyState
of 2
is not too specific for the general compat files.
The source undefined
is returned by getInitialTime
when it cannot determine the initial time, but I feel that making compat return 2
here so the RxPlayer logic may check later with getInitialTime
again is awkward: compat
has no idea what the initial-seek logic is.
To me compat should just say: this things doesn't work on the current device, or this value shoul be used on the current device. It's kind of what the function name (getMinimumReadyStateForSeeking
) is suggesting but checking for the first getInitialTime
's return value in it is kind of out-of-place I feel.
For implementation-related details such as "we will re-call getInitialTime
later", it should be constrained to the location where getInitialTime
is called IMO.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes will update the formating / type error, the formatter does not format JSDoc right ?
I agree with what you are saying with the concept of initial time does not belong to the compat folder.
Do you think this is better ?
// compat
getMinimumReadyStateForSeeking() {
return isSafari ? 2 : 1;
}
// initial_seek_and_play
const initiallySeekedTime = typeof startTime === "number" ? startTime : startTime();
const minReadyStateForSeeking = getMinimumReadyStateForSeeking();
The difference is that it will always seek at readyState 2 depending or not if the seek time is known or undefined
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
formatter does not format JSDoc right ?
I don't think so
Do you think this is better ?
For me the issue is not that there's a minimum ready state for seeking, it's 1
for both in absolute, it's more that 1
does not reliably means a known duration on safari. I think the compat util should reflect that.
} | ||
return Math.max(0, duration + startAt.fromLastPosition); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I generally like to add a namespace:
at the beginning of logs to simplify treatments (e.g. by filering/excluding some "family" of logs) and indicate quickly where the logic runs.
Here we could prepend it with Init:
Though this is not done with the other warn here, I don't know why. It seems to however be done with other calls on that file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep I basically copy/paste the existing log
I'm adding the prefix for the one missing
typeof startTime === "number" ? startTime : startTime(); | ||
if ( | ||
initiallySeekedTime === undefined && | ||
obs.readyState < 2 /* HAVE_CURRENT_DATA */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You have https://developer.mozilla.org/en-US/docs/Web/API/HTMLMediaElement/readyState#htmlmediaelement.have_current_data that allow to remove the need to have a comment here
…than next to the live edge when using startAt:fromLastPosition
1fc385f
to
3d660c9
Compare
fix a bug on Safari where a live stream will play at the time=0 rather than next to the live edge when using
startAt:fromLastPosition
. This appears on some contents that have different protection Key for each of their track.