-
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
Future-proof the transport code for a v4.0.0 #995
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
a8d4dc1
to
a6a5fe7
Compare
a1bacd1
to
74d6d35
Compare
9f74848
to
d28f42b
Compare
74d6d35
to
622c286
Compare
622c286
to
e99ac3a
Compare
dd73951
to
75547d3
Compare
e56923b
to
19f58c6
Compare
e99ac3a
to
0f42f67
Compare
0f42f67
to
20550c3
Compare
20550c3
to
6de3d58
Compare
6de3d58
to
90eea3e
Compare
-- Overview -- This commit [minimally] refactors the segment pipeline code (again, though this is more of a continuation!) so we can agree on a future-proof high-level architecture for the incoming v4.0.0. I started working on after realizing that developing complex features like #975 was very hard to do with the current `transport` code (in `src/transports`). The reason behind that complexity is linked to the fact that the transport code already contains other complex features relying on precize behavior, limiting the code's flexibility. I'm thinking here especially about the "MetaPlaylist" transport and the `segmentLoader` API. This commit tries to raise the flexibility of the transport code by: - limiting the number of arguments given to a RxPlayer loader and parser to the strict minimum that should be currently needed. This is because adding new arguments seems much easier than removing ones, which might even be passed to APIs (and thus un-removable). - clarifying semantically what those arguments mean. This makes sense especially for features like #975, where for example the `Adaptation` a `Representation` is linked to in the Manifest is different from the `Adaptation` it is linked to currently in the RxPlayer core (this is also technically a possibility today). - avoiding most side-effects (excepted requests done by loaders of course). For example, segment parsers could previously add encryption information to a `Representation` after finding encryption information in a segment. Removing this kind of global mutation from the transport code to move it to a more central place should simplify how we think about the code. -- The problem with existing APIs -- The problem with the aforementioned APIs (MetaPlaylist transport + `segmentLoader`) was mostly the same: they relied on the complete `Manifest`, `Period`, `Adaptation` and `Representation` structures linked to a segment. Giving all this information through the public API (as the `segmentLoader` is part of it) is problematic for two reasons: - mainly, we cannot change any of it without introduce a breaking change - it was also not clear semantically: were those the structures currently linked to the segment in the RxPlayer's `Manifest` global structure or were those the structures linked to the segment according to the Manifest document initially parsed? This question only makes sense because of features such as #975 and how we treat the `urn:mpeg:dash:adaptation-set-switching:2016` DASH's `SupplementalProperty`, which can bring a difference between the two. -- What has been done -- This commit: - reduces drastically the properties given to the `loadSegment` and `parseSegment` functions of a transport pipeline to just what should theoretically be needed to properly load and parse information from segments. - likewise heavily reduces even more the number of properties given to a `segmentLoader` which is part of the API. To load a segment, very few arguments should be needed (mostly an URL and a byte range). - Move out most side-effects performed by segment parsers. For now, they are all moved to the `RepresentationStream`. This means that what is sometimes transport-specific code is moved to the transport-agnostic logic. This could seem like a problem, however those differences had to already be taken into account under another form in the core logic, what is done here is more about providing a complete interface which can handle every possibility in a transport-agnostic way. For example a transport-agnostic `RepresentationIndex` structure could previously be considered "uninitialized", to properly handle some specificities about DASH's SegmentBase elements. This commit moves the task of initializing it from the transport-specific segment parser to the transport-agnostic `RepresentationStream`, the same core logic that checks whether initialization is done. "Initializing" any other kind of index will be a no-op and log an error, as it is unnecessary (the core logic is able to detect whether this action is necessary by checking the already-existing `isInitialized` method first). - I did not update the API documentation yet. I think that the only API that changed under the current code was the `segmentLoader` API, which now has much less arguments given to it - I still don't know how to properly handle the task of adding the information about new segments after segments in the Smooth streaming transport are parsed This is an optimization to avoid refreshing the Manifest too often when playing a smooth content, by using information about future segments declared in ISOBMFF boxes of previous segments. The problem here is that I don't know yet how to define it in a transport-agnostic way so it can also be moved to the `RepresentationStream` code.
…er to the RepresentationStream
90eea3e
to
b8e3499
Compare
peaBerberian
added a commit
that referenced
this pull request
Feb 18, 2022
…facto Future-proof the transport code for a v4.0.0
peaBerberian
added a commit
that referenced
this pull request
Mar 3, 2022
…facto Future-proof the transport code for a v4.0.0
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.
Overview
This branch is a WIP to [minimally] refactor the segment pipeline code (again, though this is more of a continuation!) so we can agree on a future-proof high-level architecture for the incoming v4.0.0.
I started working on after realizing that developing complex features like #975 was very hard to do with the current
transport
code (insrc/transports
).The reason behind that complexity is linked to the fact that the transport code already contains other complex features relying on precize behavior, limiting the code's flexibility.
I'm thinking here especially about the "MetaPlaylist" transport and the
segmentLoader
API.This PR tries to raise the flexibility of the transport code by:
limiting the number of arguments given to a RxPlayer loader and parser to the strict minimum that should be currently needed.
This is because adding new arguments seems much easier than removing ones, which might even be passed to APIs (and thus un-removable).
clarifying semantically what those arguments mean.
This makes sense especially for features like [Proposal] Track organizer API #975, where for example the
Adaptation
aRepresentation
is linked to in the Manifest is different from theAdaptation
it is linked to currently in the RxPlayer core (this is also technically a possibility today).avoiding most side-effects (excepted requests done by loaders of course).
For example, segment parsers could previously add encryption information to a
Representation
after finding encryption information in a segment.Removing this kind of global mutation from the transport code to move it to a more central place should simplify how we think about the code.
The problem with existing APIs
The problem with the aforementioned APIs (MetaPlaylist transport +
segmentLoader
) was mostly the same: they relied on the completeManifest
,Period
,Adaptation
andRepresentation
structures linked to a segment.Giving all this information through the public API (as the
segmentLoader
is part of it) is problematic for two reasons:Manifest
global structure or were those the structures linked to the segment according to the Manifest document initially parsed?This question only makes sense because of features such as [Proposal] Track organizer API #975 and how we treat the
urn:mpeg:dash:adaptation-set-switching:2016
DASH'sSupplementalProperty
, which can bring a difference between the two.What has been done
The current PR:
reduces drastically the properties given to the
loadSegment
andparseSegment
functions of a transport pipeline to just what should theoretically be needed to properly load and parse information from segments.likewise heavily reduces even more the number of properties given to a
segmentLoader
which is part of the API.To load a segment, very few arguments should be needed (mostly an URL and a byte range).
Move out most side-effects performed by segment parsers.
For now, they are all moved to the
RepresentationStream
.This means that what is sometimes transport-specific code is moved to the transport-agnostic logic.
This could seem like a problem, however those differences had to already be taken into account under another form in the core logic, what is done here is more about providing a complete interface which can handle every possibility in a transport-agnostic way.
For example a transport-agnostic
RepresentationIndex
structure could previously be considered "uninitialized", to properly handle some specificities about DASH's SegmentBase elements.This PR moves the task of initializing it from the transport-specific segment parser to the transport-agnostic
RepresentationStream
, the same core logic that checks whether initialization is done."Initializing" any other kind of index will be a no-op and log an error, as it is unnecessary (the core logic is able to detect whether this action is necessary by checking the already-existing
isInitialized
method first).