-
Notifications
You must be signed in to change notification settings - Fork 104
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(streams)!: Node.js Buffer+Stream -> JS Uint8Array/Web Streams API #1069
Conversation
This file contains 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
92f6b03
to
ae57930
Compare
a9621ea
to
b03acc2
Compare
898c429
to
e146e60
Compare
4589747
to
fa9f1b8
Compare
lekoaf
reviewed
Jan 8, 2025
lekoaf
reviewed
Jan 8, 2025
lekoaf
reviewed
Jan 8, 2025
lekoaf
reviewed
Jan 8, 2025
lekoaf
reviewed
Jan 9, 2025
lekoaf
reviewed
Jan 9, 2025
lekoaf
reviewed
Jan 9, 2025
lekoaf
reviewed
Jan 9, 2025
lekoaf
reviewed
Jan 9, 2025
lekoaf
reviewed
Jan 9, 2025
rikteg
requested changes
Jan 9, 2025
lekoaf
reviewed
Jan 9, 2025
lekoaf
previously approved these changes
Jan 9, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, overall the code looks fine. I trust that you test it thoroughly.
85def17
to
c5ccaaa
Compare
Closed
d1f15ff
to
eef0017
Compare
Replaces use of `Buffer` with `Uint8Array`. The latter is now widely supported and available in Node.js and Browsers. Notable differences: - `Buffer.slice(...)` has been replaced with `Uint8Array.subarray(...)` as that is the actual behaviour of the original method (`Uint8Array.slice(...)` makes a copy). - Converting to and from strings is handled by `TextEncoder`/`TextDecoder` - Converting to and from differently sized big-endian integers is handled by using `DataView` Replaces use of Node.js Stream module with Web Streams API. Since the latter is substantially different in some important details regarding stream pipelining, the way pipelines are built are redefined. The component concept as module blocks is removed and instead the components themselves expose streams that can be combined together. The pipelines are then simple stream compositions. Because this is a major (breaking) change, it's done in concert with other planned improvements that are also breaking changes (see section below for details). Improvements: - replacing the Node.js stream module with Web Streams API removes the dependency on the (legacy) stream-browserify package and results in a much smaller library (bundle) size, so there is no longer a need for a separate "light" version - `debug` package replaced by custom internal logging utilities (allowing proper ES module support) Fixes #990, Closes #992 - added audio test signal to the H.264 test Refactoring: - RTSP session and parser are combined in a single component and the session controller has been rewritten as a request-response flow. An async `start` method starts the streams and returns SDP + range. - RTP depay is combined into a single component that detects the proper format based on payloadType, and allows registering a "peeker" that can inspect messages (instead of having to insert an extra transform stream) - Extended use of TypeScript in areas where this was lacking BREAKING CHANGES: - No support for CommonJS: - Node.js has support for ES modules - Browsers have support for ES modules, but you can also still use the IIFE global variable, or use a bundler (all of which support ES modules) - No distinction between Node.js/Browser: - The library targets mainly Browser, so some things rely on `window` and expect it to be present, however most things work both platforms. - Node-only pipelines are removed, these are trivial to re-implement with Web Streams API if necessary. The CLI player has its own TCP source for that reason (replacing the CliXyz pipelines). - The generic "component" and "pipeline" classes were removed: - Components extend Web Streams API instead - Pipelines rely on `pipeTo`/`pipeThrough` composition and a `start` method to initiate flow of data. - Some public methods on pipelines have been removed (refer to their type for details) in cases where a simple alternative is available, or check the examples to see how to modify usage. There are less pipelines but they are more versatile, with accessible readonly components. In general, promises/async methods are preferred over callbacks. Co-authored-by: Rikard Tegnander <[email protected]> Co-authored-by: Victor Ingvarsson <[email protected]>
rikteg
approved these changes
Jan 10, 2025
victoringvarsson
approved these changes
Jan 10, 2025
lgtm, ship it |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Replaces use of
Buffer
withUint8Array
. The latter is now widelysupported and available in Node.js and Browsers.
Notable differences:
Buffer.slice(...)
has been replaced withUint8Array.subarray(...)
as that is the actual behaviour of the original method
(
Uint8Array.slice(...)
makes a copy).TextEncoder
/TextDecoder
handled by using
DataView
Replaces use of Node.js Stream module with Web Streams API.
Since the latter is substantially different in some important details
regarding stream pipelining, the way pipelines are built are redefined.
The component concept as module blocks is removed and instead the
components themselves expose streams that can be combined together.
The pipelines are then simple stream compositions.
Because this is a major (breaking) change, it's done in concert with
other planned improvements that are also breaking changes (see section
below for details).
Improvements:
the dependency on the (legacy) stream-browserify package and
results in a much smaller library (bundle) size, so there is no longer
a need for a separate "light" version
debug
package replaced by custom internal logging utilities(allowing proper ES module support) Fixes Cannot use versions >v11.2.0 in a pure ESM project #990, Closes Replace
debug
withtslog
#992Refactoring:
session controller has been rewritten as a request-response flow.
An async
start
method starts the streams and returns SDP + range.the proper format based on payloadType, and allows registering
a "peeker" that can inspect messages (instead of having to insert
an extra transform stream)
BREAKING CHANGES:
use the IIFE global variable, or use a bundler (all of which
support ES modules)
window
and expect it to be present, however most things work both platforms.
with Web Streams API if necessary. The CLI player has its own TCP
source for that reason (replacing the CliXyz pipelines).
pipeTo
/pipeThrough
composition and astart
method to initiate flow of data.
type for details) in cases where a simple alternative is available, or
check the examples to see how to modify usage. There are less pipelines
but they are more versatile, with accessible readonly components.
In general, promises/async methods are preferred over callbacks.
Co-authored-by: Rikard Tegnander [email protected]
Co-authored-by: Victor Ingvarsson [email protected]