-
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
[Proposal] Allow seeking through seekTo
before the HTMLMediaElement is ready
#1607
Open
peaBerberian
wants to merge
2
commits into
dev
Choose a base branch
from
misc/seek-to-before-initial
base: dev
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
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
peaBerberian
force-pushed
the
misc/seek-to-before-initial
branch
2 times, most recently
from
December 19, 2024 17:19
1a7a15d
to
f477269
Compare
peaBerberian
added
API
Relative to the RxPlayer's API
proposal
This Pull Request or Issue is only a proposal for a change with the expectation of a debate on it
Performance checks
Performance tests are run on this issue / PR
labels
Dec 19, 2024
peaBerberian
force-pushed
the
misc/seek-to-before-initial
branch
2 times, most recently
from
December 19, 2024 18:48
8bb2583
to
6862827
Compare
peaBerberian
added
Performance checks
Performance tests are run on this issue / PR
and removed
Performance checks
Performance tests are run on this issue / PR
labels
Dec 19, 2024
peaBerberian
force-pushed
the
misc/seek-to-before-initial
branch
from
December 20, 2024 09:35
6862827
to
0988a00
Compare
peaBerberian
added a commit
that referenced
this pull request
Dec 20, 2024
We have "performance" tests which allow to ensure that we're not having what we could call "performance regressions", such as a higher delay to have a content loaded from a `loadVideo` call. I wanted to run them yesterday for #1607, but they hanged indefinitely. Turns out that our static server script, that we use here just to host and serve our testing scripts returned a promise that was never resolved. _(Note: we don't rely on external test runners nor on test frameworks for our performance tests, we manually detect and launch local chrome + firefox binaries on a local server serving our bundled scripts (themselves importing the RxPlayer), and at runtime those scripts send results to another "result" server when they're done)_ It was never resolved because we never properly handled the case in that script were we wanted to launch an HTTP server but not an HTTPS one for the same resource(s). This is now fixed. Hopeful that performance tests now work correctly, it's been a long time since I last ran them.
peaBerberian
added a commit
that referenced
this pull request
Dec 20, 2024
This commit builds on #1607 to allow users of the demo to seek in the content even as it is still loading. In real use cases, a final user might rely on e.g. seeking thumbnails to already know where to seek to without having to wait for initial segments to be loaded. This is a functionality we do see with other media players, YouTube also does this for example. To make the seeking bar more visible, I lightly changed its CSS rules. Also, I conditioned the appearance of the "ProgressBar" based on if the minimum and maximum seekable positions are known, as else, seeking in that bar is not going to lead to a precize seek. This makes the content not seekable until the Manifest has been loaded.
peaBerberian
added a commit
that referenced
this pull request
Dec 20, 2024
This commit builds on #1607 to allow users of the demo to seek in the content even as it is still loading. In real use cases, a final user might rely on e.g. seeking thumbnails to already know where to seek to without having to wait for initial segments to be loaded. This is a functionality we do see with other media players, YouTube also does this for example. To make the seeking bar more visible, I lightly changed its CSS rules. Also, I conditioned the appearance of the "ProgressBar" based on if the minimum and maximum seekable positions are known, as else, seeking in that bar is not going to lead to a precize seek. This makes the content not seekable until the Manifest has been loaded.
As #1600 signalled, there's currently no practical way to update the initial position (configured through the `startAt` `loadVideo` option) until `startAt` has actually been applied. More generally, it's not possible to perform a `seekTo` call until the initial seek has been performed on the content (the `seekTo` call will just be ignored). This commit proposes the following code updates to improve on that situation: 1. Tangentially related, we now schedule a JavaScript micro-task right at `ContentTimeBoundariesObserver`'s instantiation so the caller can catch `events` that were previously synchronously sent (I'm thinking here of the warnings for the `MEDIA_TIME_BEFORE_MANIFEST` and the `MEDIA_TIME_AFTER_MANIFEST` codes). Without this, we would wait for the next playback observation, which could happen a whole second later. 2. I noticed that the position indicated through the `startAt` `loadVideo` option was bounded so it's inside the `[minimumPosition, maximumPosition]` range (as obtained from the Manifest). As [stated here](#1600 (comment)) I personally think this is suboptimal. In my opinion, we should let the initial position go outside the range of the Manifest and let the application do its thing based on `MEDIA_TIME_BEFORE_MANIFEST` / `MEDIA_TIME_AFTER_MANIFEST` events. As to not totally change the behavior, I've only done so for dynamic contents (contents for which segments are added or removed, like live and contents that are being downloaded locally). VoD contents continue to have the previous behavior for now. 3. I added a "seek-blocking" mechanism to the `MediaElementPlaybackObserver` and made all seek operations, including the one from `seekTo` go through it. The idea is that when it is blocked (as it is initially), we'll delay any seek operation (by marking it as a "pending seek") and only seek to the last one of those once the `unblockSeeking` method is called (when the RxPlayer considers that the initial seek should be done). I also had to add `getPendingSeekInformation` method, sadly. I feel that we could do without it if we're smarter about things, but I wanted to avoid changing too much code here. I also thought about reworking the initial seek so it is completely handled by the `MediaElementPlaybackObserver` - as it could make everything simpler - but I chose for now to implement that less disruptive "seek-blocking" mechanism for now. Note that technically, we could still have an application directly updating the `HTMLMediaElement.property.currentTime` property, or even a web feature such as picture-in-picture doing that. I ensured that this eventuality did not break anything. Still, there will be a preference for pending seeks performed through the `MediaElementPlaybackObserver` over such "HTML5 seeks" performed during that time (if there is a "pending seek", we will apply it regardless of if an "HTML5 seek" happened since then). I'm now unsure if the `getPosition` or `getCurrentBufferGap` API should now return that planned position. I did nothing for those yet.
This commit builds on #1607 to allow users of the demo to seek in the content even as it is still loading. In real use cases, a final user might rely on e.g. seeking thumbnails to already know where to seek to without having to wait for initial segments to be loaded. This is a functionality we do see with other media players, YouTube also does this for example. To make the seeking bar more visible, I lightly changed its CSS rules. Also, I conditioned the appearance of the "ProgressBar" based on if the minimum and maximum seekable positions are known, as else, seeking in that bar is not going to lead to a precize seek. This makes the content not seekable until the Manifest has been loaded.
peaBerberian
force-pushed
the
misc/seek-to-before-initial
branch
from
December 23, 2024 10:20
5e0767c
to
6c6d7c0
Compare
peaBerberian
added
Performance checks
Performance tests are run on this issue / PR
and removed
Performance checks
Performance tests are run on this issue / PR
labels
Dec 23, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Labels
API
Relative to the RxPlayer's API
Performance checks
Performance tests are run on this issue / PR
proposal
This Pull Request or Issue is only a proposal for a change with the expectation of a debate on it
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.
As #1600 signalled, there's currently no practical way to update the initial position (configured through the
startAt
loadVideo
option) untilstartAt
has actually been applied.More generally, it's not possible to perform a
seekTo
call until the initial seek has been performed on the content (theseekTo
call will just be ignored).This PR proposes the following code updates to improve on that situation:
Tangentially related, we now schedule a JavaScript micro-task right at
ContentTimeBoundariesObserver
's instantiation so the caller can catch events that were previously synchronously sent (I'm thinking here of the warnings for theMEDIA_TIME_BEFORE_MANIFEST
and theMEDIA_TIME_AFTER_MANIFEST
codes).Without this, we would wait for the next playback observation, which could happen a whole second later.
I noticed that the position indicated through the
startAt
loadVideo
option was bounded so it's inside the[minimumPosition, maximumPosition]
range (as obtained from the Manifest). As stated here I personally think this is suboptimal.In my opinion, we should let the initial position go outside the range of the Manifest and let the application do its thing based on
MEDIA_TIME_BEFORE_MANIFEST
/MEDIA_TIME_AFTER_MANIFEST
events.As to not totally change the behavior, I've only done so for dynamic contents (contents for which segments are added or removed, like live and contents that are being downloaded locally).
VoD contents continue to have the previous behavior for now.
I added a "seek-blocking" mechanism to the
MediaElementPlaybackObserver
and made all seek operations, including the one fromseekTo
go through it.The idea is that when it is blocked (as it is initially), we'll delay any seek operation (by marking it as a "pending seek") and only seek to the last one of those once the
unblockSeeking
method is called (when the RxPlayer considers that the initial seek should be done).I also had to add
getPendingSeekInformation
method, sadly. I feel that we could do without it if we're smarter about things, but I wanted to avoid changing too much code here.I also thought about reworking the initial seek so it is completely handled by the
MediaElementPlaybackObserver
- as it could make everything simpler - but I chose for now to implement that less disruptive "seek-blocking" mechanism.Note that technically, we could still have an application directly updating the
HTMLMediaElement.prototype.currentTime
property, or even a web feature such as picture-in-picture doing that. I ensured that this eventuality did not break anything. Still, there will be a preference for pending seeks performed through theMediaElementPlaybackObserver
over such "HTML5 seeks" performed during that time (if there is a "pending seek", we will apply it regardless of if an "HTML5 seek" happened since then).I'm now unsure if the
getPosition
orgetCurrentBufferGap
API should now return that planned position. I did nothing for those yet.