Add support for frame level user timestamp#890
Conversation
chenosaurus
requested review from
cloudwebrtc,
Copilot,
ladvoc and
theomonnom
There was a problem hiding this comment.
Pull request overview
This PR adds comprehensive support for frame-level user timestamps in video tracks, enabling end-to-end timestamp propagation through the WebRTC pipeline. The feature embeds custom timestamps as 12-byte trailers (8 bytes timestamp + 4 bytes "LKTS" magic marker) on encoded frames, which are extracted on the receiver side and made available to applications for latency measurement and frame correlation.
Changes:
- Implements
UserTimestampTransformerfor embedding/extracting timestamps in RTP frames - Integrates timestamp handling with E2EE frame cryptor through chained transformers
- Adds API surface on video tracks, sources, and streams for timestamp management
- Updates examples with E2EE support and timestamp visualization
Reviewed changes
Copilot reviewed 35 out of 36 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| webrtc-sys/include/livekit/user_timestamp.h | Core transformer interface and handler class definitions |
| webrtc-sys/src/user_timestamp.cpp | Implementation of timestamp embedding/extraction logic with LRU-based map eviction |
| webrtc-sys/src/user_timestamp.rs | Rust FFI bindings for timestamp handler |
| webrtc-sys/src/video_track.{h,cpp,rs} | Extended video track source to accept and forward user timestamps |
| webrtc-sys/src/frame_cryptor.{h,cpp,rs} | Added chaining support to combine timestamp and encryption transformers |
| libwebrtc/src/native/user_timestamp.rs | High-level Rust API for timestamp handlers |
| libwebrtc/src/native/video_{source,stream,track}.rs | Integrated timestamp handlers into video pipeline |
| livekit/src/room/e2ee/manager.rs | Automatic timestamp handler setup for all video tracks |
| livekit/src/room/track/{local,remote}_video_track.rs | Public API for accessing timestamp handlers |
| examples/local_video/src/{publisher,subscriber}.rs | Added E2EE support and timestamp visualization |
| examples/local_video/README.md | Documentation for new timestamp and E2EE features |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
webrtc-sys/src/frame_cryptor.cpp
Outdated
| private: | ||
| rtc::scoped_refptr<webrtc::FrameTransformerInterface> first_; | ||
| rtc::scoped_refptr<webrtc::FrameTransformerInterface> second_; | ||
| rtc::scoped_refptr<webrtc::TransformedFrameCallback> callback_; |
There was a problem hiding this comment.
The ChainedFrameTransformer stores the callback_ but never uses it. When the first transformer completes and calls OnTransformedFrame, the frame is forwarded to the second transformer, which then calls its own registered callback directly. The stored callback_ field is redundant and could be removed to simplify the code.
Consider removing the callback_ member variable since it serves no purpose in the current implementation.
There was a problem hiding this comment.
removed the unused ref
webrtc-sys/src/user_timestamp.cpp
Outdated
| recv_map_[rtp_timestamp] = user_ts.value(); | ||
| recv_map_order_.push_back(rtp_timestamp); |
There was a problem hiding this comment.
The same duplicate entry issue exists for the receive map. If a frame with the same RTP timestamp is received multiple times (e.g., due to retransmission), the order queue will contain duplicates while the map has only one entry. This can lead to incorrect eviction behavior.
Consider checking if the RTP timestamp already exists before adding to the order queue, or handle potential duplicates in the eviction logic.
| RTC_LOG(LS_INFO) << "UserTimestampTransformer::TransformSend appended " | ||
| "trailer" | ||
| << " ts_us=" << ts_to_embed | ||
| << " rtp_ts=" << rtp_timestamp | ||
| << " ssrc=" << ssrc | ||
| << " capture_us=" | ||
| << (capture_time.has_value() ? capture_time->us() : -1) | ||
| << " orig_size=" << data.size() | ||
| << " new_size=" << new_data.size(); |
There was a problem hiding this comment.
Using LS_INFO log level for per-frame operations will generate extremely verbose logs in production. Since this logs on every frame (potentially 30-60 times per second), it could significantly impact performance and log file sizes.
Consider using LS_VERBOSE or adding a runtime configuration flag to control this level of logging detail.
webrtc-sys/src/user_timestamp.cpp
Outdated
| send_map_[key] = user_timestamp_us; | ||
| send_map_order_.push_back(key); |
There was a problem hiding this comment.
Potential issue: If the same key (truncated capture timestamp) is stored multiple times, it will be inserted into send_map_order_ repeatedly, but the map will only contain one entry. This means send_map_order_ can contain duplicate keys, causing incorrect eviction behavior where the wrong entries might be evicted or entries might remain in the order queue after being erased from the map.
Consider checking if the key already exists in the map before adding it to the order queue, or remove existing entries from the order queue before re-adding them.
| RTC_LOG(LS_INFO) << "UserTimestampTransformer" | ||
| << " user_ts=" << user_ts.value() | ||
| << " rtp_ts=" << frame->GetTimestamp() | ||
| << " recv_latency=" << recv_latency_ms << " ms"; |
There was a problem hiding this comment.
Using LS_INFO log level for per-frame operations will generate extremely verbose logs. Since this logs on every received frame, it could significantly impact performance and log file sizes in production.
Consider using LS_VERBOSE for per-frame logs and reserving LS_INFO for connection-level events.
| RTC_LOG(LS_INFO) << "UserTimestampTransformer::store_user_timestamp" | ||
| << " capture_ts_us=" << capture_timestamp_us | ||
| << " key_us=" << key | ||
| << " user_ts_us=" << user_timestamp_us | ||
| << " size=" << send_map_.size(); |
There was a problem hiding this comment.
Using LS_INFO log level for every call to store_user_timestamp will generate extremely verbose logs. Since this is called for every captured frame (potentially 30-60 times per second), it could significantly impact performance.
Consider using LS_VERBOSE or only logging at LS_INFO when the map reaches certain thresholds.
| RTC_LOG(LS_INFO) << "UserTimestampTransformer::Transform (disabled)" | ||
| << " direction=" | ||
| << (direction_ == Direction::kSend ? "send" : "recv") | ||
| << " ssrc=" << ssrc << " rtp_ts=" << rtp_timestamp; |
There was a problem hiding this comment.
Using LS_INFO log level for disabled transformer pass-through will log on every frame even when the feature is disabled. This could generate significant log noise in production.
Consider using LS_VERBOSE for this diagnostic message or removing it entirely since pass-through is the expected behavior when disabled.
| log::info!( | ||
| target: "user_timestamp", | ||
| "store: capture_ts_us={}, user_ts_us={}", | ||
| capture_timestamp_us, | ||
| user_timestamp_us | ||
| ); | ||
| self.sys_handle.store_user_timestamp(capture_timestamp_us, user_timestamp_us); |
There was a problem hiding this comment.
The log::info! call on every store_user_timestamp invocation will generate very verbose logs (potentially 30-60 times per second for video frames). This appears to be debug-level logging rather than info-level.
Consider using log::debug! or log::trace! instead of log::info! for per-frame operations.
| // Route user timestamp transformer logs to a dedicated target so they can | ||
| // be enabled independently from the very noisy general libwebrtc logs. | ||
| if msg.contains("UserTimestampTransformer") { | ||
| log::info!(target: "user_timestamp_rtp", "{}", msg); |
There was a problem hiding this comment.
The log routing logic uses log::info! for user timestamp transformer logs, which means these very verbose per-frame logs will be at info level. Since the transformer logs on every single frame, this could still be quite noisy even with a dedicated log target.
Consider routing these to log::debug! or log::trace! instead of log::info! so they can be filtered out more easily in production.
| log::info!(target: "user_timestamp_rtp", "{}", msg); | |
| log::trace!(target: "user_timestamp_rtp", "{}", msg); |
|
Is there a way to ensure that any client can still decode videos, even if they don’t implement user timestamps? |
@theomonnom if an old SDK is used to receive a video track that has timestamps attached, it will not be able to decode. We had discussed this and the consensus was that this is something that a customer would need to ensure is enabled on all of their SDKs if they want to use it. W/ the trailer attached, the video does not decode. If we wanted to be able to allow clients that doesn't have this feature to be able to decode video, we'd have to do something like check the sdk version and strip the trailer at the SFU before sending downstream to client. |
Stripping the trailer is a cheap operation tho no? |
2 participants
Uh oh!
There was an error while loading. Please reload this page.