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

Fix a pb if realPeriodId is "". #2156

Conversation

jeremco
Copy link
Contributor

@jeremco jeremco commented Sep 4, 2017

Using 4K stream (https://dash.akamaized.net/akamai/streamroot/050714/Spring_4Ktest.mpd)
In the manifest, period id value is "". Then the method DashAdapter::getAdaptationForMediaInfo returns null, because of the test (!mediaInfo.streamInfo.id).

This PR fixes the pb by checking length of id

In this case, id value is "". The the method DashAdapter::getAdaptationForMediaInfo returns null, because of the test if (!mediaInfo.streamInfo.id)
Check 4K stream
@epiclabsDASH
Copy link
Contributor

This same issue is fixed, in a different way, in #2140 (commit b85802b).

This is open to discussion but, IMHO, I prefer this approach: assigning an id to whatever period definition that didn't indicate one or that let's it empty (assuming that let's it empty is equivalent do don't set an id).

@nicosang, @jeremco, what do you think?

@nicosang
Copy link
Contributor

nicosang commented Sep 5, 2017

Hi @epiclabsDASH,

according to me, both modifications are not opposite. Indeed, @jeremco 's modification seems to be better. Yet, a unit test of the getAdaptationForMediaInfo function could failed with an id equals to ''. So, both modifications may remain so.

Nicolas

@epiclabsDASH
Copy link
Contributor

Ok. We will be testing a situation that is never going to happen (id equals '') but been strict it is ok to include that kind of test scenario as part of the unit tests.

@nicosang, agree with you.

@nicosang
Copy link
Contributor

nicosang commented Sep 5, 2017

I do not agree with you, it's not a situation that is never going to happen. For instance, a user can call an attachSource with a already parsed manifest..... ;-)

@epiclabsDASH
Copy link
Contributor

epiclabsDASH commented Sep 5, 2017

Touché! Good point, you are right ;)

@epiclabsDASH epiclabsDASH merged commit cb92d37 into Dash-Industry-Forum:development Sep 5, 2017
@nicosang nicosang deleted the Fix_PeriodId_Empty branch September 6, 2017 07:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants