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

feat: support MPD.Location #926

Merged
merged 8 commits into from
Aug 12, 2020
Merged

feat: support MPD.Location #926

merged 8 commits into from
Aug 12, 2020

Conversation

gkatsev
Copy link
Member

@gkatsev gkatsev commented Aug 7, 2020

Depends on videojs/mpd-parser#102
With it, I can see that we properly update the srcUrl internally based on the location, for example with a stream like https://livesim.dashif.org/livesim/startrel_-20/stoprel_20/timeoffset_0/testpic_2s/Manifest.mpd

TODO

  • Add some tests
  • Make sure that a more streams with Location that don't also do a 302 redirect on the main manifest function properly
  • update mpd-parser

@gkatsev
Copy link
Member Author

gkatsev commented Aug 7, 2020

Are we supposed to use Location as the new baseUrl as well, if a baseUrl isn't provided?
At the very least I think with the above stream and https://livesim.dashif.org/livesim/startrel_-60/stoprel_-20/timeoffset_0/testpic_2s/Manifest.mpd, we need to update the baseUrl based on the 302 redirect.

@gkatsev
Copy link
Member Author

gkatsev commented Aug 7, 2020

Seems like handleManifestRedirect seems to make those streams work without requiring this change, so, we'll want to get another stream to test that we actually make subsequent requests appropriately.

@gkatsev
Copy link
Member Author

gkatsev commented Aug 10, 2020

OK, I've confirmed this does the right thing and requests the updates from the MPD.Location. I did so by grabbing https://livesim.dashif.org/livesim/startrel_-60/stoprel_-20/timeoffset_0/testpic_2s/Manifest.mpd locally, adding an MPD.BaseURL with the same url as MPD.Location. I also added a minimumUpdatePeriod so that we request updates to the manifest. I can confirm that when MPD.Location is present, it'll make requests there instead of localhost.

@gkatsev
Copy link
Member Author

gkatsev commented Aug 10, 2020

Tests will start passing once mpd-parser is updated and released.

brandonocasey
brandonocasey previously approved these changes Aug 11, 2020
gesinger
gesinger previously approved these changes Aug 12, 2020
Copy link
Contributor

@gesinger gesinger left a comment

Choose a reason for hiding this comment

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

this.master = updatedManifest;

// if locations isn't set or is an empty array, exist early
if (!this.master.locations || this.master.locations && !this.master.locations.length) {
Copy link
Contributor

@brandonocasey brandonocasey Aug 12, 2020

Choose a reason for hiding this comment

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

should this just be !this.master.locations || !this.master.locations.length

Copy link
Member Author

Choose a reason for hiding this comment

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

Let me check. I thought I tried this and it was showing an error but I may have tried it with &&?

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah, I think I had && locally. with || seems to work.

src/dash-playlist-loader.js Outdated Show resolved Hide resolved
Co-authored-by: Brandon Casey <2381475+brandonocasey@users.noreply.github.com>
brandonocasey
brandonocasey previously approved these changes Aug 12, 2020
Co-authored-by: Garrett Singer <gesinger@gmail.com>
@gkatsev gkatsev merged commit c4a43d7 into main Aug 12, 2020
@gkatsev gkatsev deleted the dash-location-element branch August 12, 2020 21:32
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.

3 participants