-
Notifications
You must be signed in to change notification settings - Fork 139
CMAF passthrough attempt v2 #859
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
base: main
Are you sure you want to change the base?
Conversation
Co-authored-by: Emil Santurio <emilsas@gmail.com>
|
Some of this has been merged as #867 |
jdreetz
left a comment
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.
Been over this a few times (didn't look to close at the Rust code though). Seems fine to me. Just some nits about naming and comments, but overall seems fine. Haven't tested locally.
| function decodeVarInt(buf: Uint8Array): [number, Uint8Array] { | ||
| const size = 1 << ((buf[0] & 0xc0) >> 6); | ||
|
|
||
| const view = new DataView(buf.buffer, buf.byteOffset, size); | ||
| const remain = new Uint8Array(buf.buffer, buf.byteOffset + size, buf.byteLength - size); | ||
| let v: number; | ||
|
|
||
| if (size === 1) { | ||
| v = buf[0] & 0x3f; | ||
| } else if (size === 2) { | ||
| v = view.getUint16(0) & 0x3fff; | ||
| } else if (size === 4) { | ||
| v = view.getUint32(0) & 0x3fffffff; | ||
| } else if (size === 8) { | ||
| // NOTE: Precision loss above 2^52 | ||
| v = Number(view.getBigUint64(0) & 0x3fffffffffffffffn); | ||
| } else { | ||
| throw new Error("impossible"); | ||
| } | ||
|
|
||
| return [v, remain]; | ||
| } | ||
|
|
||
| function encodeVarInt(v: number): Uint8Array { |
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.
should we have tests for any of this stuff?
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.
IMO We should export this from @moq/lite and yeah add some tests.
| NOTE: `reload` will detect when the broadcast goes offline/online and automatically reconnect. | ||
| TODO: Cloudflare doesn't support it yet (SUBSCRIBE_NAMESPACE), so make sure you remove it if you're using Cloudflare. | ||
|
|
||
| NOTE: You can use a <video> element instead of a <canvas> element to use the MSE backend. |
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.
I would put this comment above the video tag.
js/hang/src/watch/mse/cmaf.ts
Outdated
| throw new Error("Missing required fields to create video init segment"); | ||
| } | ||
|
|
||
| const timescale = 1_000_000; // microseconds |
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.
should we use the Time.Micro type here?
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.
Also if you have any opinions on the timer helpers. It's a little annoying how much casting is required because the math operations convert them back to numbers.
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.
It's a little annoying how much casting is required because the math operations convert them back to numbers
I've used alias types for stuff like this too, e.g. type Microseconds = numbers. You don't have to cast everywhere (I think) but you get the benefit of additional readability.
js/hang/src/watch/mse/cmaf.ts
Outdated
| }; | ||
|
|
||
| // Build codec-specific sample entry | ||
| const sampleEntry = createAudioSampleEntry(codec, sampleRate, numberOfChannels, description); |
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.
can you explain the point of a sample entry maybe?
| } else { | ||
| this.syncStatus.set({ state: "ready" }); | ||
|
|
||
| if (sleep <= BUFFERING_MS) { |
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.
could we add a comment about why this is relevant?
| if (isBuffering) return "#f87171"; // red | ||
| if (index > 0) return "#facc15"; // yellow | ||
| return "#4ade80"; // green |
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.
it'd be good to use css variables for this eventually, keep all that stuff in one place, make it easy to theme.
Replaces #822
Overview:
--passthrough(Rust only) when using HLS and fMP4 import to write the raw MP4 bytes.Backend, so it will be tree-shaken when not used.<hang-watch>element takes either a<video>or<canvas>tag to switch between backends.Watch.Broadcastno longer owns the video/audio component; they have to be constructed separately.Watch.Audio.SourceandWatch.Video.Source.Broadcast.rawcontainer.moq-litelayer that we could reuse.init_track, used to initialize the decoder.CMSFdraft, but I plan to use it via a separate track for HLS fallback.Backendinterface.watch.audio.mutedinstead ofwatch.muted.TODO:
--passthroughnot working with bbbjitterin the catalog so we can increase the latency on demand?init_trackand build it in memory instead?