Conversation
There was a problem hiding this comment.
Note
This report is out of date. Scroll down for Devin Review's latest report on this PR.
Devin Review found 1 potential issue.
View 1 additional finding in Devin Review. (Configure)
| ))), | ||
| } | ||
| } | ||
| EncodedVideoCodec::H265 => Ok(Some(GStreamerSampleFormat::H265AnnexB)), |
There was a problem hiding this comment.
🔴 Length-prefixed H.265 never starts
When caps advertise hvc1 or hev1, sample_format_from_caps_structure selects H265AnnexB. Length-prefixed samples lack Annex-B NALs, so keyframes remain deltas and capture never starts.
Learn more
H.265 caps distinguish Annex-B byte streams from ISO length-prefixed streams through stream-format. The byte-stream layout uses start codes, while hvc1 and hev1 use NAL length prefixes and out-of-band decoder configuration. This branch maps all three layouts to H265AnnexB. The H.265 path then calls access_unit_from_annex_b, which finds no Annex-B NALs in a length-prefixed sample and marks it as a delta. The encoded pump drops every frame until it sees a keyframe, so no video is published.
Example: A named appsink negotiates video/x-h265,stream-format=hvc1,alignment=au. Its IDR sample begins with a four-byte NAL length instead of 00 00 00 01. The source labels that IDR as a delta and the pump discards it and all following frames.
Recommended fix: Inspect H.265 stream-format. Accept byte-stream as Annex-B, reject hvc1 and hev1 until a proper length-prefixed converter exists, or implement conversion using the caps decoder configuration and NAL length size.
Was this helpful? React with 👍 or 👎 to provide feedback.
143a548 to
0cccd0a
Compare
Changeset ✓This PR includes a changeset covering all affected packages:
|
e634c5e to
49ceb9e
Compare
| /// When the configuration declares no resolution, this blocks until the | ||
| /// first sample arrives (bounded by a timeout) to read the stream | ||
| /// settings. | ||
| pub fn new_blocking(config: GStreamerVideoSourceConfig) -> Result<Self, SourceError> { |
There was a problem hiding this comment.
Curiosity: is new_blocking a rust convention similar to new? First I'm seeing it
| enum GstreamerBitrateUnit { | ||
| GSTREAMER_BITRATE_UNIT_BPS = 0; | ||
| GSTREAMER_BITRATE_UNIT_KBPS = 1; | ||
| } |
There was a problem hiding this comment.
Mentioned this elsewhere but just for consistency: is there a more general bitrate enum we have available to minimize API surface? No biggie if not, or if it's intentional to have a specific one here
49ceb9e to
7a2544c
Compare
There was a problem hiding this comment.
Note
This report is out of date. Scroll down for Devin Review's latest report on this PR.
Devin Review found 1 new potential issue.
2 flags not posted on this PR by your GitHub settings — view them in Devin Review. (Configure)
| pub fn encoded_caps_string(codec: EncodedVideoCodec) -> &'static str { | ||
| match codec { | ||
| EncodedVideoCodec::H264 => "video/x-h264,stream-format=byte-stream,alignment=au", | ||
| EncodedVideoCodec::H265 => "video/x-h265,stream-format=byte-stream,alignment=au", | ||
| EncodedVideoCodec::VP8 => "video/x-vp8", | ||
| EncodedVideoCodec::VP9 => "video/x-vp9,profile=(string)0", | ||
| EncodedVideoCodec::AV1 => "video/x-av1,stream-format=obu-stream,alignment=tu", | ||
| } | ||
| } |
There was a problem hiding this comment.
🟡 Public helpers leak GStreamer internals
encoded_caps_string, encoded_caps, parser_name, ENCODED_APPSINK_NAME, and pipeline are pub with no caller outside the module, and encoded_caps/pipeline expose raw gstreamer types in their signatures. The crate rule requires new sources to keep the surface minimal and hide implementation details, as the sibling pattern source does.
Learn more
The livekit-capture/AGENTS.md conventions require new source modules to keep their public API surface minimal and hide implementation details, defaulting new items to private or pub(crate). In livekit-capture/src/sources/gstreamer.rs several items are pub but have no consumers outside the module and leak GStreamer types: encoded_caps_string, encoded_caps (returns gst::Caps), parser_name, ENCODED_APPSINK_NAME, and the pipeline() accessor (returns &gst::Pipeline). Compare with sources/pattern.rs, which exposes only its config struct, error enum, source struct, and constructors. Consider changing these helpers/const to pub(crate) or private, and reconsider whether pipeline() needs to be public. Note GStreamerPipelineError must remain public because it appears in the public GStreamerVideoSourceError::Layout variant.
Was this helpful? React with 👍 or 👎 to provide feedback.
7a2544c to
14ef876
Compare
There was a problem hiding this comment.
Note
This report is out of date. Scroll down for Devin Review's latest report on this PR.
Devin Review found 3 new potential issues.
2 flags not posted on this PR by your GitHub settings — view them in Devin Review. (Configure)
| if let Some(timestamp) = buffer.pts().or_else(|| buffer.dts()) { | ||
| let timestamp_us = clock_time_to_timestamp_us(0, timestamp); | ||
| self.next_fallback_timestamp_us = timestamp_us.saturating_add(self.frame_interval_us); | ||
| return timestamp_us; |
There was a problem hiding this comment.
🔴 Zero-based timestamps jump backwards
When the first buffer has PTS zero, timestamp_us passes zero into the RTC source. It becomes wall-clock time, while later timestamps remain near zero.
Learn more
GStreamerVideoSource forwards pipeline-relative PTS/DTS values directly, usually beginning at zero. NativeVideoSource::capture_encoded_frame treats exactly zero as missing and substitutes Unix wall-clock time, but leaves later relative timestamps unchanged. Add a stable nonzero time origin for the GStreamer source and offset every PTS/DTS and fallback timestamp by it. Preserve monotonicity when buffers mix present and absent timestamps, and add tests covering a normal 0, 33_333, 66_666 sequence through the RTC timestamp behavior.
Was this helpful? React with 👍 or 👎 to provide feedback.
| let structure = | ||
| gst::Structure::builder("GstForceKeyUnit").field("all-headers", true).build(); | ||
| let _ = self.appsink.send_event(gst::event::CustomUpstream::new(structure)); |
There was a problem hiding this comment.
🟡 Keyframe requests omit mandatory fields
On PLI or FIR, request_keyframe omits the standard event's running-time and count fields. Encoders can reject it, delaying decoder recovery.
Prompt for agents
GStreamerVideoSource::request_keyframe manually builds GstForceKeyUnit with only all-headers. Standard upstream force-key-unit events include running-time, all-headers, and count; encoder implementations commonly parse that schema. Build the complete standard event, preferably through the version-matched GStreamer video event helper, and test that an upstream encoder receives and accepts a keyframe request.
Was this helpful? React with 👍 or 👎 to provide feedback.
| } | ||
| sample_format | ||
| } | ||
| None => sample_format_for_codec(requested_codec.unwrap_or(EncodedVideoCodec::H264)), |
There was a problem hiding this comment.
🟡 Late codec discovery defaults to H.264
When a named appsink exposes no codec before playback, ensure_encoded_appsink guesses H.264. Dynamically negotiated non-H.264 streams then fail to publish.
Learn more
For an existing lk_appsink whose sink pad caps do not identify a codec before the pipeline starts, ensure_encoded_appsink defaults to H.264 when config.codec is absent. This contradicts the configuration contract that omitted codecs are inferred from negotiated caps. Defer sample-format selection until the first negotiated sample, or require an explicit codec when pre-playback caps are inconclusive. Ensure the first sample's actual codec and stream format are validated before storing its caps as the baseline, and test dynamic non-H.264 negotiation.
Was this helpful? React with 👍 or 👎 to provide feedback.
14ef876 to
42fa4b9
Compare
42fa4b9 to
82d4c80
Compare
There was a problem hiding this comment.
Devin Review found 3 new potential issues.
4 flags not posted on this PR by your GitHub settings — view them in Devin Review. (Configure)
| let appsink = appsink | ||
| .downcast::<gst_app::AppSink>() | ||
| .map_err(|_| GStreamerPipelineError::NotAnAppSink)?; | ||
| return Ok((appsink, sample_format)); |
There was a problem hiding this comment.
🔴 Idle named appsinks grow unbounded
When a named appsink keeps default limits, ensure_encoded_appsink leaves its queue unbounded while the pipeline starts immediately. Delayed or omitted pump startup accumulates encoded buffers until memory exhaustion.
Learn more
GStreamer's appsink queue has no buffer limit by default. This branch accepts a caller-provided appsink unchanged, then new_blocking starts the pipeline immediately. No samples are consumed until discovery or the pump begins. A configured resolution skips discovery, and FFI clients start pumping through a separate request, so the queue can remain without a consumer indefinitely. The auto-created appsink avoids this by setting max-buffers=8 and drop=true in ensure_encoded_appsink.
Example: A 10 Mbps pipeline uses appsink name=lk_appsink with a declared resolution. Source creation returns immediately, but the client waits ten minutes before StartCaptureRequest. The appsink retains roughly 750 MB of encoded data instead of keeping only recent frames.
Recommended fix: Apply a bounded queue policy to named appsinks before returning, matching the auto-created sink. Set an explicit maximum and enable dropping, or reject an unbounded named appsink with a configuration error. Also cover source creation without pump startup in a lifecycle test.
Was this helpful? React with 👍 or 👎 to provide feedback.
| Some("avc") | Some("avc3") => Ok(Some(GStreamerSampleFormat::H264Avc { | ||
| nal_length_size: h264_avc_nal_length_size_from_caps(structure), | ||
| })), |
There was a problem hiding this comment.
🔴 AVC parameter sets disappear
With stream-format=avc, H264Avc retains only the NAL length size and discards codec_data. AVC carries SPS/PPS there, so emitted keyframes can be undecodable.
Learn more
The AVC configuration record in caps contains both the NAL length-prefix size and out-of-band parameter sets. This branch extracts only the prefix size. Later, access_unit_from_sample_payload converts only the sample NALs to Annex-B. In the avc layout, samples commonly omit SPS and PPS because those NALs live in codec_data. An IDR is still labeled as a keyframe, but a decoder joining on that frame lacks its sequence and picture parameters.
Example: h264parse negotiates stream-format=avc and places SPS/PPS in codec_data. Its IDR sample contains only a length-prefixed IDR NAL. The source emits an Annex-B IDR without SPS/PPS, while a late subscriber needs all three to decode.
Recommended fix: Parse the AVC decoder configuration record, retain its SPS/PPS NALs in the sample format state, and prepend them to key access units. Alternatively, constrain named appsinks to stream-format=byte-stream,alignment=au and reject AVC caps.
Was this helpful? React with 👍 or 👎 to provide feedback.
| let src_pad = pipeline | ||
| .find_unlinked_pad(gst::PadDirection::Src) | ||
| .ok_or(GStreamerPipelineError::MissingAppSink)?; | ||
| let inferred_codec = codec_from_pad_caps(&src_pad) | ||
| .ok_or_else(|| GStreamerPipelineError::UnsupportedPadCaps(src_pad.name().to_string()))?; |
There was a problem hiding this comment.
🟡 Non-video pads block attachment
With one encoded-video pad plus another unlinked source pad, find_unlinked_pad can select the non-video pad. Creation returns UnsupportedPadCaps despite the valid video output.
Learn more
The automatic attachment contract requires one unlinked encoded-video source pad. It does not require that every other source pad be linked. find_unlinked_pad returns one unlinked source pad without filtering by caps. The following codec check rejects that pad immediately rather than searching for the eligible video pad.
Example: A pipeline contains an unlinked VP8 encoder output and an unlinked audio encoder output. If GStreamer returns the audio pad first, source creation reports unsupported pad caps even though the VP8 pad is usable.
Recommended fix: Enumerate unlinked source pads, retain those whose caps map to a supported encoded video codec, and require exactly one eligible pad. Report ambiguity only when multiple encoded-video pads remain.
Was this helpful? React with 👍 or 👎 to provide feedback.
82d4c80 to
1d1145a
Compare
1d1145a to
0e85173
Compare
a52041f to
86690f6
Compare
86690f6 to
05ef3fe
Compare
05ef3fe to
cf9aa7d
Compare
Add a capture source that ingests encoded video from a GStreamer pipeline.
Closes BOT-551