-
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
[Proposal] [v4] Lazily send newAvailablePeriods #1265
Merged
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
1d78110
to
5fa1e9b
Compare
005618a
to
d299a6b
Compare
6135740
to
95ed769
Compare
95ed769
to
bc6719c
Compare
peaBerberian
added a commit
that referenced
this pull request
Aug 23, 2023
[Proposal] [v4] Lazily send newAvailablePeriods
peaBerberian
added a commit
that referenced
this pull request
Aug 31, 2023
[Proposal] [v4] Lazily send newAvailablePeriods
peaBerberian
added a commit
that referenced
this pull request
Aug 31, 2023
[Proposal] [v4] Lazily send newAvailablePeriods
peaBerberian
added a commit
that referenced
this pull request
Aug 31, 2023
[Proposal] [v4] Lazily send newAvailablePeriods
peaBerberian
added a commit
that referenced
this pull request
Sep 15, 2023
[Proposal] [v4] Lazily send newAvailablePeriods
peaBerberian
added a commit
that referenced
this pull request
Sep 15, 2023
[Proposal] [v4] Lazily send newAvailablePeriods
peaBerberian
added a commit
that referenced
this pull request
Sep 22, 2023
[Proposal] [v4] Lazily send newAvailablePeriods
peaBerberian
added a commit
that referenced
this pull request
Sep 26, 2023
[Proposal] [v4] Lazily send newAvailablePeriods
peaBerberian
added a commit
that referenced
this pull request
Sep 26, 2023
[Proposal] [v4] Lazily send newAvailablePeriods
peaBerberian
added a commit
that referenced
this pull request
Sep 27, 2023
[Proposal] [v4] Lazily send newAvailablePeriods
peaBerberian
added a commit
that referenced
this pull request
Sep 27, 2023
[Proposal] [v4] Lazily send newAvailablePeriods
peaBerberian
added a commit
that referenced
this pull request
Sep 27, 2023
[Proposal] [v4] Lazily send newAvailablePeriods
peaBerberian
added a commit
that referenced
this pull request
Sep 29, 2023
[Proposal] [v4] Lazily send newAvailablePeriods
Merged
peaBerberian
added a commit
that referenced
this pull request
Oct 13, 2023
[Proposal] [v4] Lazily send newAvailablePeriods
peaBerberian
added a commit
that referenced
this pull request
Oct 13, 2023
[Proposal] [v4] Lazily send newAvailablePeriods
peaBerberian
added a commit
that referenced
this pull request
Oct 13, 2023
[Proposal] [v4] Lazily send newAvailablePeriods
peaBerberian
added a commit
that referenced
this pull request
Oct 19, 2023
[Proposal] [v4] Lazily send newAvailablePeriods
peaBerberian
added a commit
that referenced
this pull request
Oct 26, 2023
[Proposal] [v4] Lazily send newAvailablePeriods
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.
This is a relatively simple to implement optimization that may be brought to the v4 beta.
The idea is that the
newAvailablePeriods
event and thegetAvailablePeriods
methods - which allow an application to select the right audio/video/text track or quality for any so-called "Period" in the content (for example on a live channel, a film may have more qualities and dynamic ranges than the ad that comes before) - may trigger performance issues on some very large Manifests.Until now, both communicated about all new Periods seen in the last loaded Manifest.
This is fine, but it may be sub-optimal in a future where a lot of Periods (with themselves a lot of track choices and themselves a lot of quality choices) might be present. There, we would potentially spend a lot of time inside the application choosing for the right track and quality as soon as the Manifest is first loaded (this is only theoretical as of now though, current Manifests seen in the wild do not have sufficient complexity for this becoming an issue for now).
Yet in most if-not-all usages I can think of, the application doesn't really care about the quality and track choice of other Periods as long as those Periods are not yet loaded by the RxPlayer.
To take an example, if you begin loading a live channel at the time of a film, you generally at this point only want to select the track and quality relative to this film - and not e.g. relative to the still-available ad that came before this film that you probably will never play.
If ever the RxPlayer starts considering other Periods, the
newAvailablePeriods
event would still be sent before their first segments are loaded - so no regression there, and the application can still select the right track and quality at the right time.This PR implements just that. The idea is to only send
newAvailablePeriods
events for Periods when they begin to be considered by the RxPlayer (as in: their segments will start to be loaded), and not just when the RxPlayer has heard of it through the Manifest, like before.Likewise,
getAvailablePeriods
only report Periods that have been sent through anewAvailablePeriods
until now.Because I've been thinking of that trick since the start of the v4, the API documentation was already accounting for that implementation, so no API breakage here.
The main issue I could think of with this new logic is that it is not possible anymore for an application to just list all the Periods currently known in the Manifest, or - even if I cannot think of one real world use case - being able to list/select a track or quality well before the RxPlayer starts considering a Period.
However for both of those cases, work-arounds are possible:
getAvailablePeriods
to be able to list ALL Periods seen in the MPD.newAvailablePeriods
events as long as the application is in possession of its id (actually they can right now do just that, it already works!) - which they might just do thanks to the new aforementionedgetAvailablePeriods
parametergetAvailablePeriods
again - though this does seem easily fixable (event anouncing a Manifest refresh?)Yet I don't know if this is a real thing applications want.
I'm still unsure of whether to merge this because the gain is for now only theoretical.
But at the same time, I don't want applications to rely on the previous behavior right away - in which case this new behavior would be harder to install (even if it doesn't break the API, sometimes applications are known to rely on empirical behavior).
At least, if we merge this, it shouldn't break code if we ever rollback it for simplicity's sake.