Fix pre-encoded frame segfault on macOS - #1291
Conversation
No changeset foundThis PR modifies versioned packages but doesn't include a changeset. The following packages require a version bump:
A package must be bumped when its own files change, and whenever a package it depends on is bumped (so downstream consumers get a matching release). Click here to create a changeset for the missing packages The link pre-populates a changeset file with If this change doesn't require a version bump, add the |
ladvoc
changed the title
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| if (encoder && | ||
| BackendFromFormat(format) != VideoEncoderBackend::PreEncoded) { | ||
| encoder = std::make_unique<EncodedFrameGuardEncoder>(std::move(encoder)); | ||
| } |
There was a problem hiding this comment.
🟡 Change ships without the required release note entry
This pull request modifies encoder behavior but does not include a changeset entry (webrtc-sys/src/video_encoder_factory.cpp:696-699), which the repository requires for every change so that affected packages get version-bumped.
Impact: The fix may be released without a version bump or documented note, so downstream consumers won't see it recorded in the release.
Missing changeset per AGENTS.md requirements
AGENTS.md states under "Documenting changes": "Every PR needs a changeset" and "Changeset must list any crates which need to be bumped stemming from the change." This PR only touches webrtc-sys/src/video_encoder_factory.cpp and adds no file under /.changeset, so the native fix (which affects the webrtc-sys crate and its dependents) is undocumented and will not trigger a version bump.
Prompt for agents
AGENTS.md requires every PR to include a changeset (under /.changeset) that lists the crates needing a version bump. This PR fixes a pre-encoded frame segfault in webrtc-sys/src/video_encoder_factory.cpp but adds no changeset. Add a changeset file (e.g. via `knope document-change` or manually in /.changeset) describing the fix and listing the affected crate(s) that should be bumped (at minimum webrtc-sys, plus any downstream crates such as livekit that need to be re-released with the fix).
Was this helpful? React with 👍 or 👎 to provide feedback.
| int32_t Encode( | ||
| const webrtc::VideoFrame& frame, | ||
| const std::vector<webrtc::VideoFrameType>* frame_types) override { | ||
| if (livekit::EncodedVideoFrameBuffer::FromNative( | ||
| frame.video_frame_buffer().get())) { | ||
| static std::atomic<bool> logged{false}; | ||
| if (!logged.exchange(true)) { | ||
| RTC_LOG(LS_WARNING) | ||
| << "Dropping pre-encoded access unit sent to a non pass-through " | ||
| "encoder; waiting for the sender to switch onto the " | ||
| "pass-through backend"; | ||
| } | ||
| return WEBRTC_VIDEO_CODEC_OK; | ||
| } | ||
| return encoder_->Encode(frame, frame_types); | ||
| } |
There was a problem hiding this comment.
It feels a bit heavy to re-implement an entire encoder to just redo one of its methods, and then only register that for pre-encoded. I am not familiar with this C++ code very much, but that's my initial read.
2 participants
Capturing pre-encoded frames shortly after publishing a track segfaults on macOS: the sender starts on the real encoder (VideoToolbox) because the switch to the passthrough encoder is asynchronous, and any access unit captured before the switch carries a kNative EncodedVideoFrameBuffer that the ObjC encoder blind-casts and retains as a garbage pointer (Linux survives via the defended ToI420() path). Fixed by having non-passthrough encoders drop pre-encoded frames instead of forwarding them until the switch completes.
This was discovered while developing the gstreamer source for livekit-capture.