feat: Add raw container format with fixed u64 timestamp encoding (JS) #784
Conversation
WalkthroughA new container format system has been introduced to support multiple timestamp encoding formats across the publication and consumption pipeline. A new Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
js/hang/src/catalog/index.ts (1)
4-5: Consider removing redundant export.Line 5 explicitly re-exports
DEFAULT_CONTAINER, but line 4 already exports everything from "./container" (which includesDEFAULT_CONTAINER). The explicit export is redundant unless intended for documentation purposes.Apply this diff if you want to remove the redundancy:
export * from "./container"; -export { DEFAULT_CONTAINER } from "./container";js/hang/src/publish/video/encoder.ts (1)
85-87: Consider using debug-level logging or removing after stabilization.This console.log will execute on every video track serve operation. For production environments, consider either removing this log once the feature is stable, or making it conditional on a debug flag.
Apply this diff if you prefer to remove it:
- const container = this.#container ?? "raw"; - console.log(`[Video Publisher] Using container format: ${container}`); - + const container = this.#container ?? "raw";js/hang/src/container/codec.ts (1)
134-144: Consider documenting precision loss for large u64 values.Like the VarInt decoder, this function will lose precision for values above 2^53 when converting from BigInt to Number. Consider adding a comment similar to line 89 for consistency.
/** * Decodes a fixed 8-byte big-endian unsigned 64-bit integer. + * NOTE: Precision loss above 2^53 */ function decodeU64(buf: Uint8Array): [number, Uint8Array] {
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
js/hang/src/catalog/audio.ts(2 hunks)js/hang/src/catalog/container.ts(1 hunks)js/hang/src/catalog/index.ts(1 hunks)js/hang/src/catalog/video.ts(2 hunks)js/hang/src/container/codec.ts(1 hunks)js/hang/src/container/index.ts(1 hunks)js/hang/src/frame.ts(7 hunks)js/hang/src/publish/audio/encoder.ts(5 hunks)js/hang/src/publish/video/encoder.ts(5 hunks)js/hang/src/watch/audio/source.ts(1 hunks)js/hang/src/watch/video/source.ts(1 hunks)
🔇 Additional comments (28)
js/hang/src/container/index.ts (1)
1-1: LGTM! Clean barrel export pattern.The re-export of the codec module follows standard TypeScript conventions for organizing and exposing public APIs.
js/hang/src/catalog/audio.ts (2)
2-2: LGTM! Correct import for container schema.The import brings in the ContainerSchema needed for the audio configuration.
17-18: LGTM! Container field properly added with backward compatibility.The optional container field enables timestamp encoding format specification while maintaining backward compatibility. The descriptive comment clarifies the field's purpose.
js/hang/src/watch/video/source.ts (1)
199-204: LGTM! Backward-compatible container handling.The implementation correctly defaults to "legacy" format when the container field is absent, ensuring compatibility with Rust publishers. The logging statement aids debugging and verification.
js/hang/src/catalog/video.ts (2)
2-2: LGTM! Correct import for container schema.The import brings in the ContainerSchema needed for the video configuration.
16-17: LGTM! Container field properly added with backward compatibility.The optional container field enables timestamp encoding format specification while maintaining backward compatibility. The descriptive comment clarifies the field's purpose.
js/hang/src/watch/audio/source.ts (1)
170-175: LGTM! Backward-compatible container handling.The implementation correctly defaults to "legacy" format when the container field is absent, ensuring compatibility with Rust publishers. The logging statement aids debugging and verification. This mirrors the video source implementation for consistency.
js/hang/src/catalog/container.ts (1)
1-18: LGTM! Well-designed container format system.The container format enum is clearly defined with comprehensive documentation. The three formats ("legacy", "raw", "fmp4") are appropriately described, and the default of "raw" aligns with the PR objectives for JavaScript publishers. The JSDoc comments provide clear guidance on when to use each format.
js/hang/src/publish/audio/encoder.ts (6)
40-40: LGTM! Container field added for format configuration.The private container field enables per-instance container format configuration.
59-59: LGTM! Constructor properly accepts optional container parameter.The optional container parameter maintains backward compatibility while enabling container format customization.
65-65: LGTM! Container parameter stored correctly.The container value is properly stored for use throughout the encoder lifecycle.
127-134: LGTM! Config generation includes container with correct default.The container field is properly included in the audio configuration, defaulting to "raw" for publishers as per the PR design.
202-203: LGTM! Container logging with proper default.The logging statement correctly defaults to "raw" and provides useful debugging information for verifying the container format in use.
191-191: Verify Frame.encode handles undefined container parameter.Line 191 passes
this.#container(which can be undefined) toFrame.encode(). Ensure thatFrame.encode()either accepts an optional container parameter or that the encoder guarantees a default value before this point.js/hang/src/publish/video/encoder.ts (3)
61-66: LGTM! Container parameter properly integrated.The optional container parameter is correctly added to the constructor signature and stored in the private field, maintaining backward compatibility.
Also applies to: 71-71
103-103: LGTM! Container properly propagated to frame encoding.The container parameter is correctly passed to
Frame.encode, enabling container-aware timestamp encoding.
149-157: LGTM! Catalog generation includes container metadata.The container field is correctly added to the VideoConfig catalog with appropriate default to "raw", ensuring subscribers receive container format information.
js/hang/src/container/codec.ts (6)
12-21: LGTM! Clean container-aware timestamp encoding.The function correctly dispatches to format-specific encoders based on container type, with appropriate default and error handling for unimplemented formats.
30-46: LGTM! Decode function mirrors encode structure.The decoding logic correctly delegates to format-specific decoders and returns the standard tuple format of
[timestamp, remaining buffer].
55-64: LGTM! Timestamp size correctly returns maximum.The function correctly returns the maximum size needed for timestamp encoding. For "legacy" varint format, this is the maximum size (8 bytes), while "raw" u64 is always 8 bytes. The comment at line 50 clarifies this behavior.
75-96: LGTM! VarInt decoder correctly implements QUIC varint format.The decoding logic properly handles all four size variants (1, 2, 4, 8 bytes) and correctly documents the JavaScript precision limitation for values above 2^53.
98-125: LGTM! VarInt encoder efficiently encodes to minimal size.The encoding logic correctly selects the smallest representation and properly handles JavaScript's safe integer range with clear overflow errors.
150-155: LGTM! U64 encoder is straightforward and correct.The fixed-size encoding is simple and efficient, properly using BigInt for the full 64-bit range.
js/hang/src/frame.ts (5)
3-4: LGTM! Required imports added for container support.The Catalog type and Container module imports are necessary for the container-aware encoding/decoding implementation.
19-38: LGTM! Frame encoding properly uses container-aware timestamps.The encode function correctly delegates timestamp encoding to the Container module and properly constructs the frame with timestamp header followed by payload.
41-45: LGTM! Frame decoding mirrors encoding structure.The decode function correctly uses container-aware timestamp decoding and returns the expected format of
{ data, timestamp }.
50-50: LGTM! Producer correctly propagates container through encoding.The container parameter is properly stored and threaded through to the frame encoding call, maintaining backward compatibility with the optional parameter.
Also applies to: 52-55, 65-65
77-77: LGTM! Consumer correctly handles container-aware decoding.The container parameter is properly integrated into the Consumer through the props interface and correctly propagated to frame decoding, maintaining backward compatibility.
Also applies to: 89-89, 101-101, 155-155
js/hang/src/catalog/audio.ts
Outdated
| codec: z.string(), | ||
|
|
||
| // Container format for timestamp encoding | ||
| container: ContainerSchema.optional(), |
There was a problem hiding this comment.
Is there a way to have this default to DEFAULT_CONTAINER instead? And remove undefined as an option.
There was a problem hiding this comment.
Done. The fallback to the default values were a mess in general. Hope it is clear now.
There was a problem hiding this comment.
FYI: DEFAULT_CONTAINER is now used elsewhere, and the previously hardcoded values have been removed from various places.
However at the moment, I’m overriding the default value in publish/element.ts during the Broadcast class initialization, to ensure that new web publishers use raw by default.
| * @param container - The container format | ||
| * @returns Size in bytes | ||
| */ | ||
| export function getTimestampSize(container: Catalog.Container = DEFAULT_CONTAINER): number { |
There was a problem hiding this comment.
Is this used? I don't think it's useful.
There was a problem hiding this comment.
Yeah. true. Removed.
js/hang/src/publish/audio/encoder.ts
Outdated
| const worklet = effect.get(this.#worklet); | ||
| if (!worklet) return; | ||
|
|
||
| const container = this.#container ?? "raw"; |
There was a problem hiding this comment.
We should default to "legacy" for now.
There was a problem hiding this comment.
Done. The fallback to the default values were a mess in general. Hope it is clear now.
js/hang/src/publish/audio/encoder.ts
Outdated
| #signals = new Effect(); | ||
|
|
||
| constructor(props?: EncoderProps) { | ||
| constructor(props?: EncoderProps, container?: Catalog.Container) { |
There was a problem hiding this comment.
Can you add container to EncoderProps?
There was a problem hiding this comment.
Done. I doubted initially if container should be part of EncoderProps. It is unrealistic to set the container type for audio and video but I think it is ok for now :)
There was a problem hiding this comment.
IMO seems perfectly fine to use separate containers for audio/video. Raw makes a lot of sense for audio, while something else (or custom) could make sense for video.
js/hang/src/publish/video/encoder.ts
Outdated
| frame: Getter<VideoFrame | undefined>, | ||
| source: Signal<Source | undefined>, | ||
| props?: EncoderProps, | ||
| container?: Catalog.Container, |
There was a problem hiding this comment.
Same deal, should be in EncoderProps
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
js/hang/src/publish/audio/encoder.ts (1)
60-66: Address past review feedback: Add container to EncoderProps.A previous review comment requested adding
containertoEncoderPropsrather than as a separate constructor parameter. The current implementation uses a separate parameter, which differs from the architectural pattern and makes the API less discoverable.🔎 Suggested refactor to include container in EncoderProps:
export type EncoderProps = { enabled?: boolean | Signal<boolean>; source?: Source | Signal<Source | undefined>; muted?: boolean | Signal<boolean>; volume?: number | Signal<number>; + // Container format for timestamp encoding (default: "legacy") + container?: Catalog.Container; + // The size of each group. Larger groups mean fewer drops but the viewer can fall further behind. // NOTE: Each frame is always flushed to the network immediately. maxLatency?: Time.Milli; }; export class Encoder { ... - constructor(props?: EncoderProps, container?: Catalog.Container) { + constructor(props?: EncoderProps) { this.source = Signal.from(props?.source); this.enabled = Signal.from(props?.enabled ?? false); this.muted = Signal.from(props?.muted ?? false); this.volume = Signal.from(props?.volume ?? 1); this.maxLatency = props?.maxLatency ?? (100 as Time.Milli); - this.#container = container; + this.#container = props?.container;js/hang/src/publish/video/encoder.ts (1)
61-71: Address past review feedback: Add container to EncoderProps.Similar to the audio encoder, a previous review requested that
containerbe added toEncoderPropsrather than as a separate constructor parameter. This would improve API consistency and discoverability.🔎 Suggested refactor:
export interface EncoderProps { enabled?: boolean | Signal<boolean>; config?: EncoderConfig | Signal<EncoderConfig | undefined>; + // Container format for timestamp encoding (default: "legacy") + container?: Catalog.Container; } -constructor( - frame: Getter<VideoFrame | undefined>, - source: Signal<Source | undefined>, - props?: EncoderProps, - container?: Catalog.Container, -) { +constructor( + frame: Getter<VideoFrame | undefined>, + source: Signal<Source | undefined>, + props?: EncoderProps, +) { this.frame = frame; this.source = source; this.enabled = Signal.from(props?.enabled ?? false); this.config = Signal.from(props?.config); - this.#container = container; + this.#container = props?.container;
🧹 Nitpick comments (5)
js/hang/src/publish/element.ts (1)
76-76: Consider making the container format configurable.The container format is hardcoded as
"raw"at this level, which bypasses the configurablecontainerfield inBroadcastProps. This creates inflexibility and prevents runtime configuration changes without modifying the code.Consider:
- Making it an attribute of the
hang-publishelement- Using a constant defined at the module level for easier maintenance
- Documenting why "raw" is the default choice for publishers
🔎 Possible approach using a module-level constant:
+// Publishers default to "raw" container format for fixed u64 timestamp encoding +const DEFAULT_PUBLISHER_CONTAINER: Catalog.Container = "raw"; + export default class HangPublish extends HTMLElement { ... this.broadcast = new Broadcast({ connection: this.connection.established, enabled: this.#enabled, path: this.path, - container: "raw", + container: DEFAULT_PUBLISHER_CONTAINER,js/hang/src/watch/video/source.ts (1)
199-204: Consider using structured logging or reducing log verbosity.The
console.logstatement for container format selection is informative during development but may create excessive noise in production environments. Consider:
- Using a debug-level logger instead of
console.log- Making this conditional on a debug flag
- Logging only when the container differs from the default
🔎 Example using conditional logging:
-// Container defaults to "legacy" via Zod schema for backward compatibility -console.log(`[Video Subscriber] Using container format: ${config.container}`); +// Container defaults to "legacy" via Zod schema for backward compatibility +if (config.container !== "legacy") { + console.log(`[Video Subscriber] Using non-default container format: ${config.container}`); +} const consumer = new Frame.Consumer(sub, { latency: this.latency, container: config.container,js/hang/src/watch/audio/source.ts (1)
170-175: Consider using structured logging or reducing log verbosity.Similar to the video subscriber, the
console.logstatement for container format may create excessive noise in production. Consider applying the same logging strategy across both audio and video subscribers for consistency.js/hang/src/publish/audio/encoder.ts (1)
203-204: Consider reducing log verbosity or using debug logging.Similar to the subscriber logs, this publisher log may create excessive noise in production. Consider conditional logging or using a structured logging framework.
js/hang/src/publish/video/encoder.ts (1)
85-86: Consider reducing log verbosity or using debug logging.Consistent with the other encoder/decoder logs, consider using conditional or debug-level logging to reduce production noise.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
js/hang/src/catalog/audio.ts(2 hunks)js/hang/src/catalog/container.ts(1 hunks)js/hang/src/catalog/video.ts(2 hunks)js/hang/src/publish/audio/encoder.ts(6 hunks)js/hang/src/publish/broadcast.ts(2 hunks)js/hang/src/publish/element.ts(1 hunks)js/hang/src/publish/video/encoder.ts(6 hunks)js/hang/src/publish/video/index.ts(2 hunks)js/hang/src/watch/audio/source.ts(1 hunks)js/hang/src/watch/video/source.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- js/hang/src/catalog/video.ts
- js/hang/src/catalog/container.ts
- js/hang/src/catalog/audio.ts
🧰 Additional context used
🧬 Code graph analysis (4)
js/hang/src/publish/video/index.ts (3)
js/hang/src/catalog/container.ts (1)
Container(12-12)js/hang/src/publish/audio/encoder.ts (1)
Encoder(32-252)js/hang/src/publish/video/encoder.ts (1)
Encoder(38-375)
js/hang/src/watch/audio/source.ts (1)
js/hang/src/frame.ts (2)
consumer(113-145)Frame(12-17)
js/hang/src/publish/broadcast.ts (3)
js/hang/src/catalog/container.ts (1)
Container(12-12)js/hang/src/catalog/audio.ts (1)
Audio(59-59)js/hang/src/catalog/video.ts (1)
Video(100-100)
js/hang/src/publish/audio/encoder.ts (2)
js/hang/src/catalog/container.ts (2)
Container(12-12)DEFAULT_CONTAINER(18-18)js/hang/src/catalog/index.ts (1)
DEFAULT_CONTAINER(5-5)
🔇 Additional comments (3)
js/hang/src/publish/broadcast.ts (1)
21-21: LGTM! Clean container propagation architecture.The optional
containerfield inBroadcastPropsprovides flexibility while maintaining backward compatibility. The propagation to both Audio.Encoder and Video.Root ensures consistent container handling across media types.Also applies to: 46-47
js/hang/src/publish/video/index.ts (1)
16-16: LGTM! Consistent container propagation.The container is correctly propagated to both HD and SD video encoders, ensuring consistent timestamp encoding across different quality renditions.
Also applies to: 39-40
js/hang/src/publish/audio/encoder.ts (1)
192-192: Frame.encode properly handles an undefined container through the default parameter in Container.encodeTimestamp.Line 192 passes
this.#containertoFrame.encode(), which accepts an optional container parameter. TheFrame.encode()function then passes this parameter toContainer.encodeTimestamp(), which has a default parameter value ofDEFAULT_CONTAINER. This design correctly handles the case whencontaineris undefined, making the explicit?? DEFAULT_CONTAINERpattern unnecessary at the call site.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
js/hang/src/publish/video/encoder.ts (1)
61-71: Container parameter should be moved toEncoderProps.As noted in a previous review, having
containeras a trailing positional parameter after the optionalpropscreates an awkward API. Callers that want to specify onlycontainermust passundefinedforprops.Consider adding
container?: Catalog.Containerto theEncoderPropsinterface and accessing it viaprops?.container ?? DEFAULT_CONTAINER.🔎 Suggested refactor:
export interface EncoderProps { enabled?: boolean | Signal<boolean>; config?: EncoderConfig | Signal<EncoderConfig | undefined>; + container?: Catalog.Container; } // ... constructor( frame: Getter<VideoFrame | undefined>, source: Signal<Source | undefined>, props?: EncoderProps, - container?: Catalog.Container, ) { this.frame = frame; this.source = source; this.enabled = Signal.from(props?.enabled ?? false); this.config = Signal.from(props?.config); - this.#container = container ?? DEFAULT_CONTAINER; + this.#container = props?.container ?? DEFAULT_CONTAINER;
🧹 Nitpick comments (1)
js/hang/src/publish/video/encoder.ts (1)
85-86: Consider log level for production.The
console.logis useful for debugging but may be noisy in production. Consider using a configurable logger or removing it once the feature is stable.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
js/hang/src/publish/video/encoder.ts(6 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
js/hang/src/publish/video/encoder.ts (2)
js/hang/src/catalog/container.ts (2)
Container(12-12)DEFAULT_CONTAINER(18-18)js/hang/src/catalog/index.ts (1)
DEFAULT_CONTAINER(5-5)
🔇 Additional comments (3)
js/hang/src/publish/video/encoder.ts (3)
4-4: LGTM!Import correctly brings in
DEFAULT_CONTAINERfor the fallback logic.
102-103: LGTM!The container is correctly passed to
Frame.encode. Since the fallback toDEFAULT_CONTAINERis applied at construction time,this.#containeris guaranteed to have a valid value.
148-156: LGTM!The catalog correctly includes the
containerfield, enabling subscribers to determine the appropriate decoding strategy.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
js/hang/src/publish/audio/encoder.ts (1)
179-179: Useconsole.debuginstead ofconsole.logfor debug output.Debug logging statements should use
console.debugrather thanconsole.logto allow proper filtering in production environments.🔎 Apply this diff:
- console.log(`[Audio Publisher] Using container format: ${this.#container}`); + console.debug(`[Audio Publisher] Using container format: ${this.#container}`);
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
js/hang/src/publish/audio/encoder.ts(7 hunks)js/hang/src/publish/element.ts(1 hunks)js/hang/src/publish/video/encoder.ts(7 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- js/hang/src/publish/video/encoder.ts
🧰 Additional context used
🧬 Code graph analysis (1)
js/hang/src/publish/audio/encoder.ts (3)
js/hang/src/catalog/container.ts (2)
Container(12-12)DEFAULT_CONTAINER(18-18)js/hang/src/catalog/index.ts (1)
DEFAULT_CONTAINER(5-5)js/hang/src/frame.ts (1)
Frame(12-17)
🔇 Additional comments (5)
js/hang/src/publish/element.ts (1)
79-79: Clarify the intended default container format.These lines hardcode
container: "raw", but a past review comment onencoder.tsrequested defaulting to "legacy", which was marked as resolved. While the PR objectives mention "Publishers (JS) now default to the 'raw' container format," this creates inconsistency:
DEFAULT_CONTAINERis set to "legacy" injs/hang/src/catalog/container.ts- The encoder properly falls back to
DEFAULT_CONTAINERwhen not specified- This file bypasses that fallback by explicitly setting "raw"
Should these be changed to omit the container field (allowing the encoder to use
DEFAULT_CONTAINER), or is the explicit "raw" setting intentional to override the "legacy" default? If "raw" is correct, consider updatingDEFAULT_CONTAINERto "raw" for consistency across the codebase.Also applies to: 84-84
js/hang/src/publish/audio/encoder.ts (4)
4-4: LGTM: Container property added to EncoderProps.The import of
DEFAULT_CONTAINERand the addition of the optionalcontainerfield toEncoderPropscorrectly implement the requested changes from past reviews. This allows callers to specify a container format while providing a sensible fallback.Also applies to: 30-31
43-43: LGTM: Container field properly initialized.The private
#containerfield is correctly declared and initialized with a fallback toDEFAULT_CONTAINERwhen not provided in props, following the same pattern as other fields in the constructor.Also applies to: 68-68
135-135: LGTM: Container included in audio config.The container field is correctly propagated to the audio configuration, ensuring the setting flows through the catalog system as intended.
195-195: LGTM: Container parameter passed to Frame.encode.The container setting is correctly propagated to the frame encoding function, completing the container-aware encoding pipeline.
2 participants
Introduces support for a new "raw" container format that uses fixed 8-byte u64 encoding for frame timestamps.
Implements partially (js only) #763