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

Different S3 serving endpoint (e.g. CDN support) #90

Closed
mattdsteele opened this issue Jul 26, 2020 · 3 comments · Fixed by #91
Closed

Different S3 serving endpoint (e.g. CDN support) #90

mattdsteele opened this issue Jul 26, 2020 · 3 comments · Fixed by #91

Comments

@mattdsteele
Copy link
Contributor

DigitalOcean supports a built-in CDN when using their S3-compatible Spaces storage. After enabling, use of the CDN just requires changing the endpoint; e.g.:

It's also possible to configure a custom domain (using a subdomain you own, for example).

I can try to add a new configuration parameter to support this (something like s3.servingHostname), but wanted to get your thoughts before making the change.

Simplest approach would be to just generate and return the custom URL within s3Storage.Save, but if you think there's value in pulling this up more generally, we could probably move it into monitor.go. This would potentially let other storage providers use the same param. I don't think this would be valuable for local serving, but maybe for other providers in the future? I dunno.

@gabek
Copy link
Member

gabek commented Jul 27, 2020

AWS also offers a feature to change the host to get a CDN'ized version of it, not to mention just having control over where the video segments are in general would be useful across the board.

So as it stands the Segment type (in playlist.go) knows the relative path (via RelativeUploadPath like 1/stream-1595834191.ts) and the absolute remote URL (RemoteID, not a great name, https://gabevideo.us-east-1.linodeobjects.com/gabevideo/hls/0/stream-1595814948.ts)

So in GenerateRemotePlaylist it seems like instead of using just the RemoteID if the config has a servingHostname it could instead piece together its own absolute URL using the overriden host in the config + the relative path. And then if an overridden host doesn't exist in the config then GenerateRemotePlaylist would just do what it already does now and use the RemoteID.

I think? :)

If you want to take a stab at this, go for it! Otherwise I can probably get to it this week.

@gabek
Copy link
Member

gabek commented Jul 27, 2020

Just noticed you already looked at it! So the only thought I had that's different is instead of rewriting the URL in Save we rewrite it in GenerateRemotePlaylist, only because that URL returned from save is technically correct, but we just want to reference it differently in the playlist.

@mattdsteele
Copy link
Contributor Author

Yup, I was just mucking around to see if I could get something working. I'll update the branch and open a PR

@gabek gabek closed this as completed in #91 Jul 28, 2020
gabek pushed a commit that referenced this issue Jul 28, 2020
* Add s3 serving endpoint config. Fixes #90

* Move CDN endpoint generation to GenerateRemotePlaylist

* Include HLS path

* Add docs and config

* Prefer sprintf to string concatenation

* Use config method

* gofmt
gabek pushed a commit that referenced this issue Apr 26, 2022
Bumps [@types/node](https://github.com/DefinitelyTyped/DefinitelyTyped/tree/HEAD/types/node) from 14.14.35 to 14.14.36.
- [Release notes](https://github.com/DefinitelyTyped/DefinitelyTyped/releases)
- [Commits](https://github.com/DefinitelyTyped/DefinitelyTyped/commits/HEAD/types/node)

Signed-off-by: dependabot[bot] <support@github.com>

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
xarantolus pushed a commit to xarantolus/owncast that referenced this issue Feb 14, 2023
Use posix 'command' instead of 'which' in install.sh
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 a pull request may close this issue.

2 participants