fix: streaming video by adding a limit to streaming chunks#1949
Merged
joachimvh merged 1 commit intoCommunitySolidServer:mainfrom Oct 14, 2024
jaxoncreed:fix/issue-1948
Merged
fix: streaming video by adding a limit to streaming chunks#1949joachimvh merged 1 commit intoCommunitySolidServer:mainfrom jaxoncreed:fix/issue-1948
joachimvh merged 1 commit intoCommunitySolidServer:mainfrom
jaxoncreed:fix/issue-1948
Conversation
joachimvh
requested changes
Oct 8, 2024
Member
joachimvh
left a comment
joachimvh
left a comment
There was a problem hiding this comment.
Some minor changes requested.
RELEASE_NOTES.md
Outdated
| - The `StaticAssetHandler` can now be used to link static pages to containers. | ||
| This can be used to set a static page for the root container of a server. | ||
| See the `/config/app/init/static-root.json` config for an example. | ||
| - Streaming video now works on Firefox and Chrome. |
Member
There was a problem hiding this comment.
When I implemented this I tested on Firefox and video streaming worked for me. But I'm guessing the difference is specifically for large videos?
But this line shouldn't be added here, these were the release notes for v7.0.0. Fixes like this one will just be mentioned in the github release.
| import type { ResponseDescription } from '../output/response/ResponseDescription'; | ||
| import type { OperationHandlerInput } from './OperationHandler'; | ||
| import { OperationHandler } from './OperationHandler'; | ||
|
|
| protected readonly logger = getLoggerFor(this); | ||
| private readonly defaultSliceSize: number; | ||
|
|
||
| public constructor(source: T, defaultSliceSize: number) { |
Member
There was a problem hiding this comment.
Suggested change
| public constructor(source: T, defaultSliceSize: number) { | |
| public constructor(source: T, defaultSliceSize = 0) { |
This needs to be an optional parameter to not be a breaking change. And just in general it's not bad to have this be optional.
| const size = termToInt(result.metadata.get(POSIX.terms.size)); | ||
|
|
||
| // Set the default end size if not set already | ||
| if (typeof end !== 'number' && typeof size === 'number' && start >= 0) { |
Member
There was a problem hiding this comment.
Suggested change
| if (typeof end !== 'number' && typeof size === 'number' && start >= 0) { | |
| if (this.defaultSliceSize > 0 && typeof end !== 'number' && typeof size === 'number' && start >= 0) { |
Comment on lines
61
to
62
| const adustedEnd = start + this.defaultSliceSize - 1; | ||
| end = adustedEnd < size ? adustedEnd : size - 1; |
Member
There was a problem hiding this comment.
Suggested change
| const adustedEnd = start + this.defaultSliceSize - 1; | |
| end = adustedEnd < size ? adustedEnd : size - 1; | |
| end = Math.min(size, start + this.defaultSlizeSize) - 1; |
| @@ -24,6 +24,12 @@ import type { ResourceStore } from './ResourceStore'; | |||
| */ | |||
Member
There was a problem hiding this comment.
Suggested change
| */ | |
| * | |
| * The `defaultSliceSize` parameter can be used to set how large a slice should be if the end of a range is not defined. | |
| * Setting this to 0, which is the default, will cause the end of the stream to be used as the end of the slice. | |
| */ |
Contributor
Author
|
Fantastic. I've made the adjustments. |
joachimvh
approved these changes
Oct 14, 2024
Member
joachimvh
left a comment
joachimvh
left a comment
There was a problem hiding this comment.
Looks good. I'll do a new release with this soon.
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
📁 Related issues
Fixes #1948
✍️ Description
Firefox and Chrome both use range headers with ambiguous endings to stream video (for example
range: 0-rather thanrange: 0-5000000). As built, CSS was trying to deliver an entire video in a single request because of this. This lead to long load times for large videos.I followed the solution posed by Zeng Xu (https://www.zeng.dev/post/2023-http-range-and-play-mp4-in-browser/) and arbitrarily added a default number of bytes to deliver if the byte range end wasn't provided. This works on Firefox, Chrome, and Safari.
Unfortunately, this does break RFC 7233 (https://www.rfc-editor.org/rfc/rfc7233#section-2.1), but as of this moment, it is the only way I found to get streaming video to work on Firefox and Chrome.
✅ PR check list
Before this pull request can be merged, a core maintainer will check whether