draft-03: add delivery properties #14
Conversation
|
Claude finished @kixelated's task —— View job PR Review: Add Delivery Properties to MOQ Lite DraftReview Checklist:
I've completed my review of the changes in this PR that add delivery properties to draft-03 of the MOQ Lite specification. This is a significant enhancement that adds crucial real-time delivery capabilities to the protocol. Technical ReviewStrengths & Improvements ✅
Issues Found 🔍Critical Issues:
Technical Concerns:
Minor Suggestions 💡
Security & Performance 🔒
Overall AssessmentThis is a high-quality addition that significantly enhances the protocol's real-time capabilities. The delivery properties framework is well-designed and addresses key requirements for live media applications. The specification is generally clear and implementable, with only minor formatting and consistency issues that should be addressed. Recommendation: Approve with minor fixes for the typos and technical inconsistencies listed above. |
WalkthroughThe document introduces a two-sided prioritization model with Priority, Ordered, and Max Latency fields for both subscribers and publishers. It adds Instant and Instant Delta semantics to frames for expiration and synchronization purposes. Group handling requirements are strengthened, requiring applications to process out-of-order groups rather than optionally buffering them. Delivery-related concepts are formalized with a new dedicated section. SUBSCRIBE, SUBSCRIBE_UPDATE, and SUBSCRIBE_OK messages are updated with new fields and behavior. The STREAM table is modified to reflect these changes. Editorial updates align terminology throughout, and the changelog is expanded to include moq-lite-03 capabilities. 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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 (7)
draft-lcurley-moq-lite.md (7)
106-111: Consider adding a forward reference to the Delivery section for clarity.The Track definition introduces three new fields (Priority, Ordered, Max Latency) but defers their detailed explanation. While this is acceptable given they're explained later in the Delivery section, adding a brief forward reference (e.g., "See the Delivery section for details") would improve flow for readers who encounter these terms for the first time.
120-121: Clarify the application's responsibility for buffering capacity.The requirement "MUST process or buffer groups out of order" places responsibility on the application but doesn't address what happens if an application lacks buffering capacity for many out-of-order groups. Should the application reset the subscription, or does the expiration mechanism handle this?
Consider adding a note: "An application without sufficient buffering capacity MAY reset the subscription or rely on the Expiration mechanism to discard old groups."
248-290: Clarify the resolution order when Publisher and Subscriber Ordered values differ.The Prioritization section explains Priority as a per-Track field and Ordered as a per-Group field within a Track, with a good example. However, it doesn't explicitly state what happens when
Subscriber OrderedandPublisher Ordereddiffer. Does the publisher always prefer Subscriber Ordered, or does it attempt to honor both (e.g., within Publisher constraints)?Consider adding: "If Subscriber Ordered and Publisher Ordered differ, the publisher SHOULD honor the Subscriber Ordered preference. The Publisher Ordered value MAY be used to resolve conflicts when serving multiple downstream subscriptions from a single upstream subscription."
321-338: Clarify whether expiration uses the minimum of both Max Latency values or per-endpoint logic.Lines 328-330 suggest separate logic per-endpoint (subscriber and publisher independently decide expiration) but state "An implementation MAY use the minimum of both when determining when to expire a group." This creates ambiguity: should implementations use the minimum, or is this optional?
Consider rewording to: "A subscriber SHOULD expire groups based on the Subscriber Max Latency...A publisher SHOULD expire groups based on the Publisher Max Latency...An implementation that proxies subscriptions (e.g., a relay) MAY use the minimum of both values to optimize resource usage."
557-560: Verify that Subscriber Max Latency guidance is clearly tied to expiration semantics.The SUBSCRIBE message now includes
Subscriber Max Latency(line 539), and the field description (lines 557-560) references the Expiration section. The description is clear, but consider whether it should explicitly mention that this value interacts with the Publisher Max Latency field sent in SUBSCRIBE_OK.Consider appending to the description: "The publisher will compare this with the Publisher Max Latency it sends in SUBSCRIBE_OK; see the Expiration section for details."
270-290: Minor: Code block styling inconsistencies.The example code blocks at lines 270-277 and 285-290 use fenced backticks but are missing language specifiers. While this doesn't affect protocol semantics, consider adding a language hint (e.g.,
```textor```plaintext) for consistency with the rest of the document and to appease the markdown linter.
246-246: Minor: Grammar and style issues.
- Line 246: "very important" is an overused intensifier; consider "How these streams are transmitted over the network is critical."
- Line 284: "full screen" should be hyphenated as "full-screen."
- Line 361: "ex." should be "e.g.," for consistency.
- Line 633: "application specific" should be "application-specific."
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
draft-lcurley-moq-lite.md
🧰 Additional context used
🪛 LanguageTool
draft-lcurley-moq-lite.md
[style] ~246-~246: As an alternative to the over-used intensifier ‘very’, consider replacing this phrase.
Context: ...ams get transmitted over the network is very important, and yet has been distilled down into a...
(EN_WEAK_ADJECTIVE)
[grammar] ~284-~284: Use a hyphen to join words.
Context: ... could override it if we decided to full screen Ali's window: ``` ali/audio subsc...
(QB_NEW_EN_HYPHEN)
[style] ~297-~297: You have already used this phrasing in nearby sentences. Consider replacing it to add variety to your writing.
Context: ...cation SHOULD NOT use ordered when it wants to provide a live experience, preferring t...
(REP_WANT_TO_VB)
[grammar] ~361-~361: Ensure spelling is correct
Context: ...r encoding delays, ex. when video takes 300ms longer to encode than audio. However, i...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~633-~633: Use a hyphen to join words.
Context: ...nformation. Payload: An application specific payload. A generic library or r...
(QB_NEW_EN_HYPHEN)
🪛 markdownlint-cli2 (0.18.1)
draft-lcurley-moq-lite.md
123-123: Heading style
Expected: setext; Actual: atx
(MD003, heading-style)
240-240: Heading style
Expected: setext; Actual: atx
(MD003, heading-style)
248-248: Heading style
Expected: setext; Actual: atx
(MD003, heading-style)
256-256: Heading style
Expected: setext; Actual: atx
(MD003, heading-style)
270-270: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
270-270: Code block style
Expected: indented; Actual: fenced
(MD046, code-block-style)
277-277: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
277-277: Code block style
Expected: indented; Actual: fenced
(MD046, code-block-style)
285-285: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
285-285: Code block style
Expected: indented; Actual: fenced
(MD046, code-block-style)
292-292: Heading style
Expected: setext; Actual: atx
(MD003, heading-style)
304-304: Heading style
Expected: setext; Actual: atx
(MD003, heading-style)
321-321: Heading style
Expected: setext; Actual: atx
(MD003, heading-style)
364-364: Heading style
Expected: setext; Actual: atx
(MD003, heading-style)
563-563: Heading style
Expected: setext; Actual: atx
(MD003, heading-style)
567-567: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
567-567: Code block style
Expected: indented; Actual: fenced
(MD046, code-block-style)
567-567: Code fence style
Expected: backtick; Actual: tilde
(MD048, code-fence-style)
579-579: Heading style
Expected: setext; Actual: atx
(MD003, heading-style)
583-583: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
583-583: Code block style
Expected: indented; Actual: fenced
(MD046, code-block-style)
583-583: Code fence style
Expected: backtick; Actual: tilde
(MD048, code-fence-style)
637-637: Heading style
Expected: setext; Actual: atx
(MD003, heading-style)
639-639: Heading style
Expected: setext; Actual: atx
(MD003, heading-style)
🔇 Additional comments (4)
draft-lcurley-moq-lite.md (4)
123-128: LGTM.The Frame definition clearly establishes the dual purpose of Instant (protocol-level for expiration/metrics and optionally application-level for presentation), which aligns well with the later Instant section.
215-235: Clarify what triggers multiple SUBSCRIBE_OK messages and their semantics.Line 235 states "The publisher replies with any number of SUBSCRIBE_OK messages, or closes the stream." However, it's unclear when/why a publisher would send multiple SUBSCRIBE_OK messages beyond the initial acceptance. Does this correspond to dynamic priority updates (e.g., speaker changes in the conference example)?
The Appendix A changelog (line 643) states "SUBSCRIBE_OK may be sent multiple times" but doesn't explain the trigger or semantics. Consider adding guidance on when and why publishers send multiple SUBSCRIBE_OK messages (e.g., updating Publisher Priority dynamically).
615-630: Clarify the Instant Delta encoding starting point within a Group.The FRAME message now uses
Instant Delta(line 621) encoded as a delta from the previous frame. However, it's not explicitly stated: what is the baseline Instant for the first frame in a group? Should implementations assume:
- The delta is relative to the previous frame in the group (clear for frames 2+)?
- Or should the first frame's delta be relative to 0, or to the Group's start time?
Line 628 says "encoded as a delta from the previous frame in the same group" but doesn't clarify the first frame's baseline. Consider adding: "For the first frame in a group, the Instant Delta is relative to 0. Subsequent frames are relative to the previous frame's instant (base instant + cumulative deltas)."
637-643: LGTM.The changelog accurately documents the new moq-lite-03 fields and capabilities introduced in this draft.
No description provided.