-
Notifications
You must be signed in to change notification settings - Fork 125
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
Refacto current position handling again just to work-around tizen mischievous seek-backs #1327
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
740ed29
to
16e7868
Compare
16e7868
to
c45a14a
Compare
a477e6e
to
3332783
Compare
c45a14a
to
d0b886d
Compare
d0b886d
to
5d682a7
Compare
Closed
Florent-Bouisset
approved these changes
Dec 18, 2023
peaBerberian
added a commit
that referenced
this pull request
Dec 19, 2023
Refacto current position handling again just to work-around tizen mischievous seek-backs
peaBerberian
added a commit
that referenced
this pull request
Dec 20, 2023
Refacto current position handling again just to work-around tizen mischievous seek-backs
peaBerberian
added a commit
that referenced
this pull request
Dec 22, 2023
Refacto current position handling again just to work-around tizen mischievous seek-backs
peaBerberian
added a commit
that referenced
this pull request
Jan 3, 2024
Refacto current position handling again just to work-around tizen mischievous seek-backs
peaBerberian
added a commit
that referenced
this pull request
Jan 3, 2024
Refacto current position handling again just to work-around tizen mischievous seek-backs
peaBerberian
added a commit
that referenced
this pull request
Jan 11, 2024
Refacto current position handling again just to work-around tizen mischievous seek-backs
peaBerberian
added a commit
that referenced
this pull request
Jan 11, 2024
Refacto current position handling again just to work-around tizen mischievous seek-backs
peaBerberian
added a commit
that referenced
this pull request
Jan 15, 2024
Refacto current position handling again just to work-around tizen mischievous seek-backs
peaBerberian
added a commit
that referenced
this pull request
Jan 23, 2024
Refacto current position handling again just to work-around tizen mischievous seek-backs
Merged
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Tizen "seek-backs"
Tizen devices (mainly samsung TVs) are the only devices on which seek operations is not precize.
On those devices for example, you might want to seek at a given position, let's say
51
seconds, and tizen could actually after media segments are loaded decide to finally play at an earlier position instead, let's say50
seconds.The choice actually made by tizen seems to correspond to the closest IDR video frame instead of what the RxPlayer wants, we guess because of performance reasons.
This caused many problems in the RxPlayer as we generally want to seek at a very specific position for very valid reasons, for example it could be because we need the position to be within a specific Period's boundaries or because we need to make sure the current position is after a known audio discontinuity to ensure smooth playback.
The fact that tizen may re-seek back in the past, what we now call "seek-backs", can lead to very problematic player behaviors, such as seeking loops (tizen seeking back, RxPlayer seeking forth) or worse in the recently-seen case, content reloading loops.
The previous strategy was to directly make exceptions, just for Tizen devices, at each place in the code where this seek-back behavior led to problems. After 5 or 6 patches, we thought for many years that we had no remaining issues.
The newly seen issue
However we very recently found out a new rare and subtle issue linked to those same pesky seek-backs, this time when:
onCodecSwitch
loadVideo
option is set toreload
We finally decided to try a general solution which is the point of this PR.
New way of dealing with it
What I decided to do here, was to reinforce a difference between:
HTMLMediaElement
'scurrentTime
attribute, which is the position we're actually playing on the media element, andSimply written in the Tizen case, the former will actually be affected by seek-backs and might thus not always reflect what the RxPlayer wants the position to be, but the second will only indicate the position the RxPlayer actually wants to play at.
Then the rest of the RxPlayer code will use one or the other, depending on what it wants to do:
currentTime
, which may have been affected by Tizen seek-backsI've been able to reuse the same 2-positions concept to also simplify the handling of the initial seek performed by the RxPlayer:
readyState
before being able to seek), thecurrentTime
is set to0
Why only for the v4?
This is the first bug fix we've implemented only for the v4. We may backport it for the
v3
but the code update was big enough and the bug rare enough for me to prefer targeting the v4 first.