-
Notifications
You must be signed in to change notification settings - Fork 3
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(FEC-10773): reuse Shaka load option to start load from the detached point #130
Conversation
…ed point Issue: attachMediaSource start to load from the beginning instead of the last detach point. Solution: start load from the last detach point.
src/dash-adapter.js
Outdated
@@ -553,7 +539,11 @@ export default class DashAdapter extends BaseMediaSourceAdapter { | |||
*/ | |||
detachMediaSource(): void { | |||
if (this._shaka) { | |||
this._lastTimeDetach = this.currentTime; | |||
if (parseInt(this.currentTime) === parseInt(this.duration)) { |
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.
why do we need to parseInt? They are both numbers, no?
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.
Right but they're floats, I wanted to say if it's close enough until rounding ms from the end it's the end.
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, i think it is better. ParseInt expects string.
Did it run flow?
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 see that it passed flow. Anyway it is more clearer to math round. Someone might think like me it is was done by mistake
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.
Sure, it was also in use before - take a look on the line 543 that removed
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.
Super edge case that I see now is 800.1 and 799.9 which return false so I added handling for this as well
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.
The worst case scenario is that the user will see the end of the playback and click replay, no?
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.
So Actually Math.round is better, no?
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.
Same for this solution
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 wonder if we need the floor or keep it simple
this._lastTimeDetach = 0; | ||
} else { | ||
this._lastTimeDetach = this.currentTime; | ||
} |
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.
this overrides the startPosition passed by the app in case of pre-roll
Description of the Changes
Issue: attachMediaSource start to load from the beginning instead of the last detach point.
Solution: start load from the last detach point.
CheckLists