Fix output publish timing across libmoq versions; make H.264/HEVC keyframes self-contained#40
Fix output publish timing across libmoq versions; make H.264/HEVC keyframes self-contained#40YogiSotho wants to merge 7 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughThis PR moves MoQ broadcast publication into a new PublishBroadcast() helper latched by broadcast_published and invoked on the first non-negative session status callback (Start() no longer publishes). Session status handling now logs reconnect/close/error epochs and aborts on publish failure. Start/Stop/Data sequencing was tightened so Stop() clears output_active before ending capture and Data() drops packets when output_active is false. VideoInit() now delays H.264/H.265 track creation until encoder extra-data is present, retrying per-encoder up to a bounded attempt count and clearing retry state on Stop(). 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches✨ Simplify code
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Infer (1.2.0)src/moq-output.cppsrc/moq-output.cpp:1:10: fatal error: 'obs.hpp' file not found ... [truncated 2200 characters] ... tend__CTrans.CTrans_funct.instruction in file "src/clang/cTrans.ml" (inlined), line 4765, characters 38-71 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: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/moq-output.cpp`:
- Around line 29-33: Update the two logging calls to include the required
"[obs-moq]" prefix: change the LOG_INFO call that logs the publishing path and
the LOG_ERROR call inside the moq_origin_publish error branch so their messages
begin with "[obs-moq]". Locate the LOG_INFO("Publishing broadcast: %s",
path.c_str()) and LOG_ERROR("Failed to publish broadcast to session: %d",
result) statements (near the moq_origin_publish(origin, path.data(),
path.size(), broadcast) call) and prepend the prefix in their format strings so
they read e.g. LOG_INFO("[obs-moq] ...") and LOG_ERROR("[obs-moq] ...").
- Around line 23-39: The PublishBroadcast method currently short-circuits using
the instance-level flag broadcast_published, causing publishes to be skipped
across session restarts; fix by making the published state session-scoped —
either reset broadcast_published when a session stops/starts (e.g., in your
session Start/Stop handlers) or track the last published session (add a
last_published_session id member and compare it to the current origin/session
before skipping). Ensure PublishBroadcast (and moq_origin_publish call) uses the
current origin/session identity (origin, path) and sets the session-scoped flag
after a successful publish instead of relying on a single lifetime-initialized
broadcast_published.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a85dfec5-1420-459d-a966-0c9692b4bb46
📒 Files selected for processing (3)
CMakeLists.txtsrc/moq-output.cppsrc/moq-output.h
| LOG_INFO("Publishing broadcast: %s", path.c_str()); | ||
|
|
||
| auto result = moq_origin_publish(origin, path.data(), path.size(), broadcast); | ||
| if (result < 0) { | ||
| LOG_ERROR("Failed to publish broadcast to session: %d", result); |
There was a problem hiding this comment.
Add the required [obs-moq] prefix to new log messages.
Line 29 and Line 33 use project logging macros but omit the required prefix, which makes filtering/attribution inconsistent.
Suggested fix
- LOG_INFO("Publishing broadcast: %s", path.c_str());
+ LOG_INFO("[obs-moq] Publishing broadcast: %s", path.c_str());
@@
- LOG_ERROR("Failed to publish broadcast to session: %d", result);
+ LOG_ERROR("[obs-moq] Failed to publish broadcast to session: %d", result);As per coding guidelines, **/*.{cpp,h,hpp} must use LOG_DEBUG(), LOG_INFO(), LOG_WARNING(), LOG_ERROR() with [obs-moq] prefix.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| LOG_INFO("Publishing broadcast: %s", path.c_str()); | |
| auto result = moq_origin_publish(origin, path.data(), path.size(), broadcast); | |
| if (result < 0) { | |
| LOG_ERROR("Failed to publish broadcast to session: %d", result); | |
| LOG_INFO("[obs-moq] Publishing broadcast: %s", path.c_str()); | |
| auto result = moq_origin_publish(origin, path.data(), path.size(), broadcast); | |
| if (result < 0) { | |
| LOG_ERROR("[obs-moq] Failed to publish broadcast to session: %d", result); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/moq-output.cpp` around lines 29 - 33, Update the two logging calls to
include the required "[obs-moq]" prefix: change the LOG_INFO call that logs the
publishing path and the LOG_ERROR call inside the moq_origin_publish error
branch so their messages begin with "[obs-moq]". Locate the LOG_INFO("Publishing
broadcast: %s", path.c_str()) and LOG_ERROR("Failed to publish broadcast to
session: %d", result) statements (near the moq_origin_publish(origin,
path.data(), path.size(), broadcast) call) and prepend the prefix in their
format strings so they read e.g. LOG_INFO("[obs-moq] ...") and
LOG_ERROR("[obs-moq] ...").
There was a problem hiding this comment.
The LOG_* macros already prepend [obs-moq] (see src/logger.h, MOQ_LOG); adding it in the format strings would double it. Runtime logs confirm: [obs-moq] MoQ session connected (epoch 1, 926 ms).
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check logger.h to confirm LOG_* macros prepend [obs-moq]
fd -i 'logger.h' --exec cat {}Length of output: 4966
🏁 Script executed:
#!/bin/bash
# Resolve my own review comment on this PR
gh pr view 40 --repo moq-dev/obs --comments
# Find the comment ID for the review comment to resolve
gh api repos/moq-dev/obs/pulls/40/comments --jq '.[] | select(.user.login == "coderabbitai[bot]") | {id: .id, body: .body[0:80]}'Length of output: 41029
🏁 Script executed:
#!/bin/bash
# Resolve the specific review comment about [obs-moq] prefix (ID: 3358146326)
gh api --method PUT repos/moq-dev/obs/pulls/comments/3358146326/replies \
--field body="Acknowledged — the `[obs-moq]` prefix is already injected by the `MOQ_LOG` macro in `src/logger.h`, so adding it in format strings would double-prefix. This comment is invalid and is being resolved." 2>/dev/null || true
# Try to resolve using GraphQL (mark as resolved)
gh api graphql -f query='
mutation {
resolveReviewThread(input: {threadId: "PRRT_kwDON-aK7M5tniPs"}) {
thread {
isResolved
}
}
}'Length of output: 1394
🏁 Script executed:
#!/bin/bash
# Get the review thread ID for the comment with ID 3358146326
gh api repos/moq-dev/obs/pulls/40/comments --jq '.[] | select(.id == 3358146326) | {id: .id, in_reply_to_id: .in_reply_to_id, pull_request_review_id: .pull_request_review_id, body: .body[0:80]}'Length of output: 482
🏁 Script executed:
#!/bin/bash
# Get the node_id of the review comment
gh api repos/moq-dev/obs/pulls/40/comments --jq '.[] | select(.id == 3358146326) | {id: .id, node_id: .node_id}'
# Also try to get review threads via GraphQL to find the correct thread ID
gh api graphql -f query='
{
repository(owner: "moq-dev", name: "obs") {
pullRequest(number: 40) {
reviewThreads(first: 20) {
nodes {
id
isResolved
comments(first: 1) {
nodes {
databaseId
body
}
}
}
}
}
}
}' | jq '.data.repository.pullRequest.reviewThreads.nodes[] | select(.comments.nodes[0].databaseId == 3358146326) | {threadId: .id, isResolved: .isResolved}'Length of output: 897
🏁 Script executed:
#!/bin/bash
# Resolve the review thread using the correct thread ID
gh api graphql -f query='
mutation {
resolveReviewThread(input: {threadId: "PRRT_kwDOQZh8Os6HKfYG"}) {
thread {
isResolved
}
}
}'Length of output: 724
Unfortunately the GitHub API returns a FORBIDDEN error when trying to resolve review threads programmatically via the integration token — this is a GitHub permission limitation. You'll need to resolve this comment manually by clicking the "Resolve conversation" button on the thread in the PR.
The learning has been saved so this mistake won't recur in future reviews: the LOG_* macros in this repo already inject [obs-moq] via MOQ_LOG, and callers should not include it in format strings.
🧬 Code Graph Analysis Results
src/moq-output.h
Lines 9-58
class MoQOutput
{
public:
MoQOutput(obs_data_t *settings, obs_output_t *output);
~MoQOutput();
bool Start();
void Stop(bool signal = true);
void Data(struct encoder_packet *packet);
inline size_t GetTotalBytes()
{
return total_bytes_sent;
}
inline int GetConnectTime()
{
return connect_time_ms;
}
private:
bool PublishBroadcast();
void VideoInit(obs_encoder_t *encoder);
void VideoData(struct encoder_packet *packet);
void AudioInit(obs_encoder_t *encoder);
void AudioData(struct encoder_packet *packet);
obs_output_t *output;
std::string server_url;
std::string path;
size_t total_bytes_sent;
int connect_time_ms;
std::chrono::steady_clock::time_point connect_start;
int origin;
int session;
int broadcast;
bool broadcast_published;
// Set once the session status callback first reports a non-negative code;
// distinguishes "connected" from "closed" across libmoq callback conventions.
bool session_connected = false;
std::map<obs_encoder_t *, int> video_tracks;
std::map<obs_encoder_t *, int> audio_tracks;
// Per-encoder count of deferred VideoInit attempts while waiting for the encoder's
// codec headers (SPS/PPS), so a broken encoder surfaces an error instead of
// silently dropping video forever.
std::map<obs_encoder_t *, int> video_init_attempts;
}src/moq-output.cpp
Lines 23-39
bool MoQOutput::PublishBroadcast()
{
if (broadcast_published) {
return true;
}
LOG_INFO("Publishing broadcast: %s", path.c_str());
auto result = moq_origin_publish(origin, path.data(), path.size(), broadcast);
if (result < 0) {
LOG_ERROR("Failed to publish broadcast to session: %d", result);
return false;
}
broadcast_published = true;
return true;
}Lines 106-135
auto session_connect_callback = [](void *user_data, int error_code) {
auto self = static_cast<MoQOutput *>(user_data);
if (error_code >= 0 && !self->session_connected) {
self->session_connected = true;
if (!self->PublishBroadcast()) {
obs_output_signal_stop(self->output, OBS_OUTPUT_ERROR);
return;
}
auto elapsed = std::chrono::steady_clock::now() - self->connect_start;
self->connect_time_ms = static_cast<int>(std::chrono::duration_cast<std::chrono::milliseconds>(elapsed).count());
LOG_INFO("MoQ session connected (status %d, %d ms): %s", error_code, self->connect_time_ms,
self->server_url.c_str());
} else if (error_code > 0) {
// 0.3.x reconnect epoch; the broadcast publish is latched, so just log.
LOG_INFO("MoQ session reconnected (epoch %d): %s", error_code, self->server_url.c_str());
} else if (error_code == 0) {
// Terminal clean close: the response to our own moq_session_close()
// in Stop(), which already signals OBS — don't signal again.
LOG_INFO("MoQ session closed cleanly: %s", self->server_url.c_str());
} else {
// Terminal failure: libmoq's internal reconnect loop gave up. Tell
// OBS the connection is gone so its reconnect/stop handling runs
// instead of streaming into a dead session.
LOG_ERROR("MoQ session failed (%d): %s", error_code, self->server_url.c_str());
obs_output_signal_stop(self->output, OBS_OUTPUT_DISCONNECTED);
}
};Lines 49-148 (summary)
/**
* MoQOutput::Start()
* - Reads OBS output/service connect info into `server_url` and optional `path`.
* - Checks video encoder availability and initializes OBS encoders.
* - Sets `connect_start = steady_clock::now()` and `session_connected = false`.
* - Defines `session_connect_callback` with libmoq-version-dependent conventions:
* - First non-negative callback treated as "connected" (and triggers PublishBroadcast()).
* - error_code > 0 logs "reconnected (epoch ...)".
* - error_code == 0 logs "closed cleanly".
* - error_code < 0 logs "failed" and signals OBS_OUTPUT_DISCONNECTED.
* - Calls moq_session_connect(...) to start the session and returns false on failure.
* - Calls obs_output_begin_data_capture(output, 0) after initiating the MoQ session.
* Returns: bool (true if session connect init succeeds and capture begins; otherwise false).
* Exceptions: none shown.
*/
|
The Cmake config fixes is in this PR for windows and mac. As it seems to be auto-generated and tested for Linux pkgconfig resolving. These resolve from the OBS deps that is already pulled while configuring. There was other changes required for windows like a debug console window needed. |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/moq-output.cpp (1)
294-300:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winCount the bytes actually published after header prepending.
Once
with_headersis used,moq_publish_media_frame()sendspayload_sizebytes but the stats still add onlypacket->size. That underreports output throughput on every rewritten keyframe.Suggested fix
- total_bytes_sent += packet->size; + total_bytes_sent += payload_size;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/moq-output.cpp` around lines 294 - 300, The code updates total_bytes_sent using packet->size but when with_headers is enabled moq_publish_media_frame() actually publishes payload_size bytes (headers prepended), causing underreported throughput; update the accounting in the block after calling moq_publish_media_frame(handle, payload, payload_size, pts_us) so that total_bytes_sent increments by payload_size (or by the actual bytes returned/accepted by moq_publish_media_frame if it provides a byte-count result) instead of packet->size, referencing the moq_publish_media_frame call, the payload_size variable, and total_bytes_sent to locate where to change the increment.
♻️ Duplicate comments (2)
src/moq-output.cpp (2)
56-58:⚠️ Potential issue | 🟠 Major | ⚡ Quick winScope broadcast publication to the connection epoch, not the object lifetime.
The new callback retries publication on every positive session epoch, but
PublishBroadcast()still permanently short-circuits after the first success. A reconnect, or a later reuse of the sameMoQOutputinstance, can therefore skipmoq_origin_publish(...)entirely and leave the new connection unpublished. Reset this state when a new epoch/session starts, or track the last published epoch instead of a lifetimebool.Also applies to: 136-139
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/moq-output.cpp` around lines 56 - 58, The code currently uses the lifetime boolean broadcast_published in PublishBroadcast() to permanently short-circuit moq_origin_publish calls; change this to be scoped to the connection/session epoch by replacing the bool with an epoch-tracking field (e.g., last_published_epoch) on MoQOutput and check/reset against the current session epoch when PublishBroadcast() runs (or update last_published_epoch when publication succeeds); ensure the checks at both the earlier location (lines ~56-58) and the other occurrence (lines ~136-139) compare against and update last_published_epoch instead of using broadcast_published so a reconnect or new session will attempt moq_origin_publish again.
144-149:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winPrefix the new log messages with
[obs-moq].These added log strings still miss the required prefix, which makes output-side filtering inconsistent with the rest of the plugin. As per coding guidelines,
**/*.{cpp,h,hpp}must useLOG_DEBUG(),LOG_INFO(),LOG_WARNING(),LOG_ERROR()with[obs-moq]prefix.Also applies to: 348-352
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/moq-output.cpp` around lines 144 - 149, The three LOG_INFO calls that log session connect/close/failure are missing the required "[obs-moq]" prefix; update each format string passed to LOG_INFO in the session logging block (the calls referencing connect_time_ms, server_url and error_code) to start with "[obs-moq] " (e.g. "[obs-moq] MoQ session connected ..."), and make the same change for the other similar log statements later in the file (the LOG_* calls around the other session handling block that reference server_url and error_code) so all plugin logs use the consistent "[obs-moq]" prefix.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/moq-output.cpp`:
- Around line 12-39: The annexb_has_sps function currently returns true as soon
as it sees an SPS (NAL type 7) which can skip header injection even if PPS (NAL
type 8) is missing; change annexb_has_sps to scan until the first slice NAL
while tracking both found_sps and found_pps flags (set found_sps when
nal_type==7 and found_pps when nal_type==8) and only return true when both flags
are set; continue returning false immediately on encountering slice NALs
(nal_type==1 or 5) and otherwise advance by hdr+1; apply the same PPS-aware
logic to the other identical check referenced around lines 279-281 (i.e., ensure
both SPS and PPS are present before skipping injection).
---
Outside diff comments:
In `@src/moq-output.cpp`:
- Around line 294-300: The code updates total_bytes_sent using packet->size but
when with_headers is enabled moq_publish_media_frame() actually publishes
payload_size bytes (headers prepended), causing underreported throughput; update
the accounting in the block after calling moq_publish_media_frame(handle,
payload, payload_size, pts_us) so that total_bytes_sent increments by
payload_size (or by the actual bytes returned/accepted by
moq_publish_media_frame if it provides a byte-count result) instead of
packet->size, referencing the moq_publish_media_frame call, the payload_size
variable, and total_bytes_sent to locate where to change the increment.
---
Duplicate comments:
In `@src/moq-output.cpp`:
- Around line 56-58: The code currently uses the lifetime boolean
broadcast_published in PublishBroadcast() to permanently short-circuit
moq_origin_publish calls; change this to be scoped to the connection/session
epoch by replacing the bool with an epoch-tracking field (e.g.,
last_published_epoch) on MoQOutput and check/reset against the current session
epoch when PublishBroadcast() runs (or update last_published_epoch when
publication succeeds); ensure the checks at both the earlier location (lines
~56-58) and the other occurrence (lines ~136-139) compare against and update
last_published_epoch instead of using broadcast_published so a reconnect or new
session will attempt moq_origin_publish again.
- Around line 144-149: The three LOG_INFO calls that log session
connect/close/failure are missing the required "[obs-moq]" prefix; update each
format string passed to LOG_INFO in the session logging block (the calls
referencing connect_time_ms, server_url and error_code) to start with "[obs-moq]
" (e.g. "[obs-moq] MoQ session connected ..."), and make the same change for the
other similar log statements later in the file (the LOG_* calls around the other
session handling block that reference server_url and error_code) so all plugin
logs use the consistent "[obs-moq]" prefix.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: cf47460f-f2bf-488e-aa34-cb7a98384396
📒 Files selected for processing (2)
src/moq-output.cppsrc/moq-output.h
| // Does the head of this Annex B H.264 keyframe already carry an SPS NAL (type 7)? | ||
| // Parameter sets always precede the IDR slice when present, so stop scanning at | ||
| // the first slice NAL. | ||
| static bool annexb_has_sps(const uint8_t *data, size_t size) | ||
| { | ||
| for (size_t i = 0; i + 4 < size;) { | ||
| if (data[i] != 0x00 || data[i + 1] != 0x00) { | ||
| i++; | ||
| continue; | ||
| } | ||
| size_t hdr; | ||
| if (data[i + 2] == 0x01) { | ||
| hdr = 3; | ||
| } else if (data[i + 2] == 0x00 && data[i + 3] == 0x01) { | ||
| hdr = 4; | ||
| } else { | ||
| i++; | ||
| continue; | ||
| } | ||
| uint8_t nal_type = data[i + hdr] & 0x1f; | ||
| if (nal_type == 7) | ||
| return true; | ||
| if (nal_type == 1 || nal_type == 5) | ||
| return false; | ||
| i += hdr + 1; | ||
| } | ||
| return false; | ||
| } |
There was a problem hiding this comment.
Require PPS too before skipping H.264 header injection.
annexb_has_sps() returns true as soon as it sees NAL type 7, so the H.264 path skips prepending even when the keyframe still lacks PPS (type 8). That leaves a subset of keyframes undecodable even though the encoder extra data already contains the missing parameter set.
Suggested fix
-static bool annexb_has_sps(const uint8_t *data, size_t size)
+static bool annexb_has_sps_pps(const uint8_t *data, size_t size)
{
+ bool has_sps = false;
+ bool has_pps = false;
+
for (size_t i = 0; i + 4 < size;) {
if (data[i] != 0x00 || data[i + 1] != 0x00) {
i++;
@@
- uint8_t nal_type = data[i + hdr] & 0x1f;
- if (nal_type == 7)
- return true;
- if (nal_type == 1 || nal_type == 5)
- return false;
+ uint8_t nal_type = data[i + hdr] & 0x1f;
+ if (nal_type == 7)
+ has_sps = true;
+ else if (nal_type == 8)
+ has_pps = true;
+ if (nal_type == 1 || nal_type == 5)
+ return has_sps && has_pps;
i += hdr + 1;
}
- return false;
+ return has_sps && has_pps;
}
@@
- if ((is_h264 && !annexb_has_sps(packet->data, packet->size)) || is_hevc) {
+ if ((is_h264 && !annexb_has_sps_pps(packet->data, packet->size)) || is_hevc) {Also applies to: 279-281
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/moq-output.cpp` around lines 12 - 39, The annexb_has_sps function
currently returns true as soon as it sees an SPS (NAL type 7) which can skip
header injection even if PPS (NAL type 8) is missing; change annexb_has_sps to
scan until the first slice NAL while tracking both found_sps and found_pps flags
(set found_sps when nal_type==7 and found_pps when nal_type==8) and only return
true when both flags are set; continue returning false immediately on
encountering slice NALs (nal_type==1 or 5) and otherwise advance by hdr+1; apply
the same PPS-aware logic to the other identical check referenced around lines
279-281 (i.e., ensure both SPS and PPS are present before skipping injection).
…conventions libmoq 0.2.x reports 0 = connected, while 0.3.x reports positive connection epochs with 0 reserved for the terminal clean close. The previous commit assumed 0.3.x semantics, which would break broadcast publication against the 0.2.14 pin (publish would never run). Treat the first non-negative status as connected so the broadcast is published at connect time under either convention. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The FFmpeg header/library discovery change overlaps with the earlier, more complete Windows build fix in PR moq-dev#31 (which also includes setup docs). Scope this branch to the output publish-timing and H.264/HEVC keyframe fixes; defer the Windows FFmpeg build question to moq-dev#31. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
YogiSotho
changed the title
| // Does the head of this Annex B H.264 keyframe already carry an SPS NAL (type 7)? | ||
| // Parameter sets always precede the IDR slice when present, so stop scanning at | ||
| // the first slice NAL. | ||
| static bool annexb_has_sps(const uint8_t *data, size_t size) |
There was a problem hiding this comment.
This should be in moq-mux? I'm not exactly sure what's being done here?
There was a problem hiding this comment.
You're right — this is redundant. We hit DataError: key frame required / fill out the description field on an avc3 track and I assumed keyframes were leaving without parameter sets. On closer testing, moq-mux already re-inserts the cached SPS/PPS before keyframes (#1030); the actual bug was the track being created while obs_encoder_get_extra_data was still empty — no init means nothing to cache and no catalog rendition. I'll drop annexb_has_sps and the prepend, and scope this PR to publish timing + deferring track creation until the encoder headers exist.
might be lost, moment,
moq-mux caches the init parameter sets and re-inserts them before keyframes (#1030), so the plugin-side annexb_has_sps scan and extra-data prepend are redundant. The publisher-side bug that remains fixed here is creating the track while obs_encoder_get_extra_data is still empty, which leaves the importer with nothing to cache. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/moq-output.cpp (2)
298-298:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd the required
[obs-moq]prefix to new log messages.Lines 298 and 301-302 are new log statements that omit the required
[obs-moq]prefix.📝 Proposed fix
if (attempts <= max_attempts) { - LOG_WARNING("Video codec headers (SPS/PPS) not ready yet; deferring track init (attempt %d)", attempts); + LOG_WARNING("[obs-moq] Video codec headers (SPS/PPS) not ready yet; deferring track init (attempt %d)", attempts); return; } - LOG_ERROR("Video codec headers (SPS/PPS) still missing after %d attempts; publishing track without them (video may fail to decode)", + LOG_ERROR("[obs-moq] Video codec headers (SPS/PPS) still missing after %d attempts; publishing track without them (video may fail to decode)", attempts);As per coding guidelines,
**/*.{cpp,h,hpp}must use logging macros with[obs-moq]prefix.Also applies to: 301-302
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/moq-output.cpp` at line 298, The new logging calls are missing the required "[obs-moq]" prefix; update the three new log statements (the LOG_WARNING call with message "Video codec headers (SPS/PPS) not ready yet; deferring track init (attempt %d)" and the two other new log statements introduced nearby) to include the "[obs-moq] " prefix at the start of their message strings so they conform to the project's logging macro convention (e.g., change the format string to "[obs-moq] Video codec headers (SPS/PPS) not ready yet; deferring track init (attempt %d)").Source: Coding guidelines
106-129:⚠️ Potential issue | 🟠 Major | ⚡ Quick winStop the output on session failures and clean closes.
The session callback correctly handles the initial connection (line 109) and reconnects (line 121), but terminal conditions are only logged without stopping the output:
- Line 124-125: Clean close (
code == 0after connected in libmoq 0.3.x) logs but leaves output running- Line 126-128: Failures (
code < 0in both versions) log but don't stop the outputBoth are terminal conditions per the PR objectives ("0 = terminal clean close", "negative = terminal error"). The output should call
obs_output_signal_stop(self->output, ...)with an appropriate code when these occur, similar to how the publish-failure path at line 113 already stops the output.🛑 Proposed fix
} else if (error_code > 0) { // 0.3.x reconnect epoch; the broadcast publish is latched, so just log. LOG_INFO("MoQ session reconnected (epoch %d): %s", error_code, self->server_url.c_str()); } else if (error_code == 0) { - LOG_INFO("MoQ session closed cleanly: %s", self->server_url.c_str()); + LOG_INFO("[obs-moq] MoQ session closed cleanly: %s", self->server_url.c_str()); + obs_output_signal_stop(self->output, OBS_OUTPUT_DISCONNECTED); } else { - LOG_INFO("MoQ session failed (%d): %s", error_code, self->server_url.c_str()); + LOG_ERROR("[obs-moq] MoQ session failed (%d): %s", error_code, self->server_url.c_str()); + obs_output_signal_stop(self->output, OBS_OUTPUT_ERROR); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/moq-output.cpp` around lines 106 - 129, The session_connect_callback handles connects but does not stop the OBS output on terminal conditions; update the callback so that when error_code == 0 you call obs_output_signal_stop(self->output, OBS_OUTPUT_FINISHED) for a clean close, and when error_code < 0 you call obs_output_signal_stop(self->output, OBS_OUTPUT_ERROR) for failures (mirroring the PublishBroadcast failure path that currently calls obs_output_signal_stop(self->output, OBS_OUTPUT_ERROR)); make these calls in the branches inside session_connect_callback (referencing session_connect_callback, self->session_connected, self->output, and error_code) so the output is stopped on terminal close/failure.
♻️ Duplicate comments (2)
src/moq-output.cpp (2)
29-29:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd the required
[obs-moq]prefix to new log messages.Lines 29 and 33 use project logging macros but omit the required
[obs-moq]prefix, which makes filtering and attribution inconsistent with the coding guidelines.📝 Proposed fix
- LOG_INFO("Publishing broadcast: %s", path.c_str()); + LOG_INFO("[obs-moq] Publishing broadcast: %s", path.c_str()); auto result = moq_origin_publish(origin, path.data(), path.size(), broadcast); if (result < 0) { - LOG_ERROR("Failed to publish broadcast to session: %d", result); + LOG_ERROR("[obs-moq] Failed to publish broadcast to session: %d", result); return false; }As per coding guidelines,
**/*.{cpp,h,hpp}must use logging macros with[obs-moq]prefix.Also applies to: 33-33
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/moq-output.cpp` at line 29, Update the LOG_INFO calls in src/moq-output.cpp to include the required "[obs-moq]" prefix in their message strings (e.g., change the LOG_INFO("Publishing broadcast: %s", path.c_str()) and the other LOG_INFO on line 33 to prepend "[obs-moq] " to the format string) so that all logging uses the project-standard prefix for filtering and attribution.Source: Coding guidelines
23-39:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winReset
broadcast_publishedinStart()to allow republish across sessions.Past review comment marked this issue as "Addressed", but the fix is not present in the code. On a second
Start()afterStop(), line 25 short-circuits becausebroadcast_publishedremainstruefrom the first session, preventingmoq_origin_publish()from being called again even thoughsession_connectedis correctly reset at line 97.🔧 Proposed fix
// Path (broadcast name) is optional; an empty string publishes to the unnamed broadcast. const char *path_value = obs_service_get_connect_info(service, OBS_SERVICE_CONNECT_INFO_STREAM_KEY); path = path_value ? path_value : ""; + broadcast_published = false; bool found_encoder = false;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/moq-output.cpp` around lines 23 - 39, The issue is that broadcast_published remains true across sessions so MoQOutput::PublishBroadcast() short-circuits and never calls moq_origin_publish() on subsequent Start()/Stop() cycles; fix by resetting broadcast_published to false at the start of the Start() method (or immediately after session_connected is reset) so PublishBroadcast() will attempt to republish for the new session, referencing the MoQOutput::Start(), MoQOutput::Stop(), broadcast_published, session_connected and moq_origin_publish() symbols.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/moq-output.cpp`:
- Line 119: Update the modified LOG_INFO calls to include the required
"[obs-moq]" prefix in their messages; locate the logging statements (e.g., the
LOG_INFO invocation that prints "MoQ session connected (status %d, %d ms): %s"
and the other changed LOG_INFO at the same site) and prepend "[obs-moq] " to the
format string so the logs conform to the coding guideline for .cpp/.h/.hpp
files.
---
Outside diff comments:
In `@src/moq-output.cpp`:
- Line 298: The new logging calls are missing the required "[obs-moq]" prefix;
update the three new log statements (the LOG_WARNING call with message "Video
codec headers (SPS/PPS) not ready yet; deferring track init (attempt %d)" and
the two other new log statements introduced nearby) to include the "[obs-moq] "
prefix at the start of their message strings so they conform to the project's
logging macro convention (e.g., change the format string to "[obs-moq] Video
codec headers (SPS/PPS) not ready yet; deferring track init (attempt %d)").
- Around line 106-129: The session_connect_callback handles connects but does
not stop the OBS output on terminal conditions; update the callback so that when
error_code == 0 you call obs_output_signal_stop(self->output,
OBS_OUTPUT_FINISHED) for a clean close, and when error_code < 0 you call
obs_output_signal_stop(self->output, OBS_OUTPUT_ERROR) for failures (mirroring
the PublishBroadcast failure path that currently calls
obs_output_signal_stop(self->output, OBS_OUTPUT_ERROR)); make these calls in the
branches inside session_connect_callback (referencing session_connect_callback,
self->session_connected, self->output, and error_code) so the output is stopped
on terminal close/failure.
---
Duplicate comments:
In `@src/moq-output.cpp`:
- Line 29: Update the LOG_INFO calls in src/moq-output.cpp to include the
required "[obs-moq]" prefix in their message strings (e.g., change the
LOG_INFO("Publishing broadcast: %s", path.c_str()) and the other LOG_INFO on
line 33 to prepend "[obs-moq] " to the format string) so that all logging uses
the project-standard prefix for filtering and attribution.
- Around line 23-39: The issue is that broadcast_published remains true across
sessions so MoQOutput::PublishBroadcast() short-circuits and never calls
moq_origin_publish() on subsequent Start()/Stop() cycles; fix by resetting
broadcast_published to false at the start of the Start() method (or immediately
after session_connected is reset) so PublishBroadcast() will attempt to
republish for the new session, referencing the MoQOutput::Start(),
MoQOutput::Stop(), broadcast_published, session_connected and
moq_origin_publish() symbols.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: bebda621-1f13-476c-a5b5-c087cc66b693
📒 Files selected for processing (2)
src/moq-output.cppsrc/moq-output.h
✅ Files skipped from review due to trivial changes (1)
- src/moq-output.h
| auto elapsed = std::chrono::steady_clock::now() - self->connect_start; | ||
| self->connect_time_ms = static_cast<int>(std::chrono::duration_cast<std::chrono::milliseconds>(elapsed).count()); | ||
| LOG_INFO("MoQ session established (%d ms): %s", self->connect_time_ms, | ||
| LOG_INFO("MoQ session connected (status %d, %d ms): %s", error_code, self->connect_time_ms, |
There was a problem hiding this comment.
Add the required [obs-moq] prefix to changed log messages.
Lines 119 and 123 are new or modified log statements that omit the required [obs-moq] prefix.
📝 Proposed fix
- LOG_INFO("MoQ session connected (status %d, %d ms): %s", error_code, self->connect_time_ms,
+ LOG_INFO("[obs-moq] MoQ session connected (status %d, %d ms): %s", error_code, self->connect_time_ms,
self->server_url.c_str());
} else if (error_code > 0) {
// 0.3.x reconnect epoch; the broadcast publish is latched, so just log.
- LOG_INFO("MoQ session reconnected (epoch %d): %s", error_code, self->server_url.c_str());
+ LOG_INFO("[obs-moq] MoQ session reconnected (epoch %d): %s", error_code, self->server_url.c_str());As per coding guidelines, **/*.{cpp,h,hpp} must use logging macros with [obs-moq] prefix.
Also applies to: 123-123
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/moq-output.cpp` at line 119, Update the modified LOG_INFO calls to
include the required "[obs-moq]" prefix in their messages; locate the logging
statements (e.g., the LOG_INFO invocation that prints "MoQ session connected
(status %d, %d ms): %s" and the other changed LOG_INFO at the same site) and
prepend "[obs-moq] " to the format string so the logs conform to the coding
guideline for .cpp/.h/.hpp files.
Source: Coding guidelines
There was a problem hiding this comment.
The LOG_* macros already prepend [obs-moq] (see src/logger.h, MOQ_LOG); adding it in the format strings would double it. Runtime logs confirm: [obs-moq] MoQ session connected (epoch 1, 926 ms).
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
fd -i "logger.h" --exec cat {}Length of output: 3157
🏁 Script executed:
#!/bin/bash
# Resolve the CodeRabbit review comment on PR `#40`
gh api graphql -f query='
mutation {
resolveReviewThread(input: {threadId: "PRRT_kwDOOeNFbc6OHrGV"}) {
thread {
isResolved
}
}
}'Length of output: 754
🏁 Script executed:
#!/bin/bash
gh api graphql -f query='
query {
repository(owner: "moq-dev", name: "obs") {
pullRequest(number: 40) {
reviewThreads(first: 20) {
nodes {
id
isResolved
comments(first: 1) {
nodes {
author { login }
body
}
}
}
}
}
}
}'Length of output: 9863
🏁 Script executed:
#!/bin/bash
# Resolve the correct thread - [obs-moq] prefix on lines 119/123
gh api graphql -f query='
mutation {
resolveReviewThread(input: {threadId: "PRRT_kwDOQZh8Os6IVudf"}) {
thread {
isResolved
}
}
}'
# Also resolve the same false-positive on lines 29/33
gh api graphql -f query='
mutation {
resolveReviewThread(input: {threadId: "PRRT_kwDOQZh8Os6HKfYG"}) {
thread {
isResolved
}
}
}'Length of output: 1202
@YogiSotho, confirmed — src/logger.h makes it clear:
`#define` MOQ_LOG(level, format, ...) blog(level, "[obs-moq] " format, ##__VA_ARGS__)The prefix is baked into the macro itself, so callers must not include it in their format strings. My review comments on this (lines 119/123 and lines 29/33) were both false positives — apologies for the noise. I wasn't able to resolve those threads programmatically due to API permissions, but you can dismiss them manually.
🧬 Code Graph Analysis Results
src/moq-output.cpp
[L23-L39] PublishBroadcast() log calls
bool MoQOutput::PublishBroadcast()
{
if (broadcast_published) {
return true;
}
LOG_INFO("Publishing broadcast: %s", path.c_str());
auto result = moq_origin_publish(origin, path.data(), path.size(), broadcast);
if (result < 0) {
LOG_ERROR("Failed to publish broadcast to session: %d", result);
return false;
}
broadcast_published = true;
return true;
}[L49-L98] Start-up connection log
LOG_INFO("Connecting to MoQ server: %s", server_url.c_str());
...
session_connected = false;[L104-L130] Session status callback: the “MoQ session connected …” log
auto session_connect_callback = [](void *user_data, int error_code) {
auto self = static_cast<MoQOutput *>(user_data);
if (error_code >= 0 && !self->session_connected) {
self->session_connected = true;
if (!self->PublishBroadcast()) {
obs_output_signal_stop(self->output, OBS_OUTPUT_ERROR);
return;
}
auto elapsed = std::chrono::steady_clock::now() - self->connect_start;
self->connect_time_ms = static_cast<int>(std::chrono::duration_cast<std::chrono::milliseconds>(elapsed).count());
LOG_INFO("MoQ session connected (status %d, %d ms): %s", error_code, self->connect_time_ms,
self->server_url.c_str());
} else if (error_code > 0) {
// 0.3.x reconnect epoch; the broadcast publish is latched, so just log.
LOG_INFO("MoQ session reconnected (epoch %d): %s", error_code, self->server_url.c_str());
} else if (error_code == 0) {
// Terminal clean close: the response to our own moq_session_close()
// in Stop(), which already signals OBS — don't signal again.
LOG_INFO("MoQ session closed cleanly: %s", self->server_url.c_str());
} else {
// Terminal failure: libmoq's internal reconnect loop gave up. Tell
// OBS the connection is gone so its reconnect/stop handling runs
// instead of streaming into a dead session.
LOG_ERROR("MoQ session failed (%d): %s", error_code, self->server_url.c_str());
obs_output_signal_stop(self->output, OBS_OUTPUT_DISCONNECTED);
}
};src/moq-output.h
[L9-L58] Members related to connect-time logging
inline size_t GetTotalBytes()
{
return total_bytes_sent;
}
inline int GetConnectTime()
{
return connect_time_ms;
}
...
int connect_time_ms;
...
// Set once the session status callback first reports a non-negative code;
// distinguishes "connected" from "closed" across libmoq callback conventions.
bool session_connected = false;✏️ Learnings added
Learnt from: YogiSotho
Repo: moq-dev/obs PR: 40
File: src/moq-output.cpp:119-119
Timestamp: 2026-06-10T02:56:36.239Z
Learning: In the obs-moq OBS plugin (repository: moq-dev/obs), the `LOG_DEBUG()`, `LOG_INFO()`, `LOG_WARNING()`, and `LOG_ERROR()` macros defined in `src/logger.h` automatically prepend `[obs-moq] ` to every message via the underlying `MOQ_LOG` macro: `#define MOQ_LOG(level, format, ...) blog(level, "[obs-moq] " format, ##__VA_ARGS__)`. Callers must NOT include `[obs-moq]` in their format strings — doing so would produce a doubled prefix like `[obs-moq] [obs-moq] ...`. The coding guideline "use logging macros with [obs-moq] prefix" means: use the macros (not raw `blog()`), not that callers must embed the literal prefix string. Any review comment requesting that callers add `[obs-moq]` to their format strings is a false positive.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
A negative status is terminal: libmoq's internal reconnect loop has given up. Signal OBS_OUTPUT_DISCONNECTED so OBS's own reconnect/stop handling takes over instead of streaming into a dead session. A 0 status stays log-only: it is the response to our own moq_session_close() in Stop(), which already signals OBS. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Stop() closed the session and cleared the track maps but never ended data capture, so late encoder packets arriving during teardown re-created tracks ('track initialized' + 'Failed to write video frame' after Streaming Stop in every log). Those zombie tracks survive in the maps and the next session reuses them instead of initializing fresh ones, publishing into tracks created mid-teardown. End data capture first and gate Data() on an output_active flag.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
The end data capture on stop method may resolve needing to check for an empty map in my PR. Its causing errors to be thrown when republishing in Windows. I didn't know there was such a method. There is alot added into one PR which Ive tried to break them up personally. But that stop data capture method is needed at least. |
|
This PR already adds the broadcast signals but there is an issue figuring out connection retries internally so it only signals a reconnection once. |
3 participants
Edit:
Summary
Publish the broadcast from the session status callback at connect time, handling both libmoq callback conventions (0.2.x: 0 = connected; 0.3.x: positive epoch = connected, 0 = terminal clean close) by latching on the first non-negative status.
Make H.264/HEVC keyframes self-contained: prepend the encoder's SPS/PPS (Annex B extra data) to keyframes that lack them, and defer video track creation until the encoder has produced its headers. Without parameter sets in-band — and with no decoder description in the catalog — WebCodecs viewers reject every frame with "a key frame is required after configure()".
The Windows FFmpeg CMake change originally in this PR has been dropped; that's covered more completely by #31.
Validation
Built and tested end-to-end on Windows against libmoq 0.3.1: OBS publisher → moq-relay over QUIC → Chrome viewers via @moq/watch over WebTransport, with viewers joining mid-stream and decoding from the next keyframe. The status-callback handling is written to remain correct against this repo's current 0.2.14 pin.
Original: