Skip to content
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

getRepresentationsForAdaptation optimization #2447

Conversation

Gontran-Molotov
Copy link
Contributor

This PR adds a new maxRepresentations parameter to DashManifestModel.getRepresentationsForAdaptation(). It allows to specify how many representations to get. Currently, some classes calls getRepresentationsForAdaptation() to uselessly get info for all representations but only use the first one, like in StreamController.onManifestUpdated().

@epiclabsDASH epiclabsDASH modified the milestones: v2.6.7, 2.6.8 Mar 1, 2018
@@ -582,7 +582,7 @@ function StreamController() {

if (mediaInfo) {
voAdaptation = adapter.getDataForMedia(mediaInfo);
useCalculatedLiveEdgeTime = dashManifestModel.getRepresentationsForAdaptation(voAdaptation)[0].useCalculatedLiveEdgeTime;
useCalculatedLiveEdgeTime = dashManifestModel.getRepresentationsForAdaptation(voAdaptation, 1)[0].useCalculatedLiveEdgeTime;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given the requirement for setting the max number of representation requirements only comes from this line, I think could makes more sense adding a method to dashManifestModel that returns useCalculatedLiveEdgeTime of a given Adaptation and abstract StreamController of the way it should be calculated (useCalculatedLiveEdgeTime of the first representation of the adaptation). @Gontran-Molotov, what do you think?

@Gontran-Molotov
Copy link
Contributor Author

Indeed, makes more sense. I'm going to make appropriate changes.

@Gontran-Molotov Gontran-Molotov force-pushed the optimizeGetRepresentationsForAdaptation branch from 2945b90 to 9cddda9 Compare April 10, 2018 14:10
@epiclabsDASH epiclabsDASH merged commit b4b0a84 into Dash-Industry-Forum:development Apr 16, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants