Fix screensharing #119
Fix screensharing #119
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThis PR enhances video track lifecycle management by adding Changes
Sequence Diagram(s)sequenceDiagram
participant App as Application
participant Agent as Agent
participant Tracks as Track Manager
participant Forwarder as VideoForwarder
participant Processor as Frame Processor
App->>Agent: on_track(video track)
activate Agent
Agent->>Tracks: Store in _active_video_tracks
Agent->>Agent: _process_track()
Note over Agent: Subscribe & wrap if screenshare
Agent->>Forwarder: Create shared RAW forwarder
Agent->>Agent: _current_video_track_id = track_id
deactivate Agent
loop Frame Processing
Tracks->>Forwarder: Forward frame
Forwarder->>Processor: Process RAW frame
Processor-->>Forwarder: Return processed frame
end
App->>Agent: on_track_removed(video track)
activate Agent
Agent->>Tracks: Cancel per-track task
Agent->>Tracks: Remove from _active_video_tracks
Agent->>Agent: _switch_to_next_available_track()
Note over Agent: Select next available video/screenshare
Agent->>Agent: _current_video_track_id = new_track_id
deactivate Agent
App->>Agent: on_track(same video track re-added)
activate Agent
Agent->>Agent: Reuse existing forwarder
Note over Agent: Realtime mode: switch forwarder
deactivate Agent
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Disabled knowledge base sources:
📒 Files selected for processing (3)
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 |
maxkahan
force-pushed
the
fix-screensharing
branch
from
32378b6 to
d488a0d
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
agents-core/vision_agents/core/agents/agents.py (2)
660-679: Nil‑safe check and fast‑path switchUse explicit None check to avoid treating a valid enum 0 as falsy. Fast‑path logic looks good.
- if not track_id or not track_type: + if not track_id or track_type is None: return
866-899: Graceful cancellation in _process_track loopHandle asyncio.CancelledError to exit cleanly when tracks are removed.
- while True: - try: + try: + while True: + try: @@ - except asyncio.TimeoutError: + except asyncio.TimeoutError: @@ - await asyncio.sleep(backoff_delay) + await asyncio.sleep(backoff_delay) + except asyncio.CancelledError: + return
🧹 Nitpick comments (6)
plugins/getstream/vision_agents/plugins/getstream/stream_edge_transport.py (1)
109-116: Consider synchronizing _track_map accessEventManager dispatches handlers as tasks; _on_track_published and _on_track_removed could race on _track_map. A simple asyncio.Lock around map writes avoids flakiness under bursty events. Optional but prudent.
tests/test_utils.py (1)
446-481: Nice coverage; add single‑odd cases and format assertionCurrent tests cover even and both-odd. Add width-odd/height-even and width-even/height-odd cases, and assert pixel format preserved (yuv420p). Keeps regressions tight.
agents-core/vision_agents/core/utils/video_utils.py (1)
7-11: Docstring to Google style; pin pixel format in reformatImprove docstring per guidelines and explicitly pass the source format to avoid any implicit format changes.
- """ - Ensure frame has even dimensions for H.264 yuv420p encoding. - - Crops by 1 pixel if width or height is odd. - """ + """Ensure even frame dimensions for H.264 yuv420p (4:2:0). + + Crops by 1 pixel on width and/or height when odd, preserving PTS/time_base. + + Args: + frame: Input av.VideoFrame. + + Returns: + av.VideoFrame: Original if already even; otherwise a new cropped frame. + """ @@ - cropped = frame.reformat(width=new_width, height=new_height) + cropped = frame.reformat(width=new_width, height=new_height, format=frame.format.name)Also applies to: 21-24
agents-core/vision_agents/core/agents/agents.py (3)
737-764: Switching logic: ensure shared_forwarder matches subscriberYou reuse a stored forwarder (from prior subscription) with a freshly added subscriber track. If Realtime._watch_video_track consumes frames exclusively from shared_forwarder, you're fine; if it also interacts with the passed track, mismatch could occur. Verify provider behavior; consider passing None/placeholder for track when using a shared forwarder only.
777-787: Even‑dimensions wrapper OK; comment says RAW but forwarder sees processed framesYou wrap screenshare frames before creating the “raw” forwarder, so it’s actually forwarding cropped frames. Adjust the comment to avoid confusion or create the forwarder from the unwrapped track if true raw is desired.
Also applies to: 788-803
841-856: Rename typo: hasImageProcessers → has_image_processorsMinor clarity/readability.
- hasImageProcessers = len(self.image_processors) > 0 + has_image_processors = len(self.image_processors) > 0 @@ - if hasImageProcessers: + if has_image_processors: @@ - if not hasImageProcessers: + if not has_image_processors:Also applies to: 876-879
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
agents-core/vision_agents/core/agents/agents.py(8 hunks)agents-core/vision_agents/core/utils/video_utils.py(1 hunks)plugins/getstream/tests/test_getstream_plugin.py(1 hunks)plugins/getstream/vision_agents/plugins/getstream/stream_edge_transport.py(1 hunks)tests/test_utils.py(2 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.py
📄 CodeRabbit inference engine (.cursor/rules/python.mdc)
**/*.py: Do not modify sys.path in Python code
Docstrings must follow the Google style guide
Files:
plugins/getstream/vision_agents/plugins/getstream/stream_edge_transport.pyagents-core/vision_agents/core/utils/video_utils.pytests/test_utils.pyplugins/getstream/tests/test_getstream_plugin.pyagents-core/vision_agents/core/agents/agents.py
tests/**/*.py
📄 CodeRabbit inference engine (.cursor/rules/python.mdc)
tests/**/*.py: Never use mocking utilities (e.g., unittest.mock, pytest-mock) in test files
Write tests using pytest (avoid unittest.TestCase or other frameworks)
Mark integration tests with @pytest.mark.integration
Do not use @pytest.mark.asyncio; async support is automatic
Files:
tests/test_utils.py
🧬 Code graph analysis (4)
plugins/getstream/vision_agents/plugins/getstream/stream_edge_transport.py (3)
agents-core/vision_agents/core/edge/sfu_events.py (8)
track_type(579-583)track_type(1193-1197)track_type(2254-2258)participant(1495-1500)participant(1537-1542)participant(1610-1615)participant(2078-2083)participant(2127-2132)agents-core/vision_agents/core/events/manager.py (1)
send(426-468)agents-core/vision_agents/core/edge/events.py (1)
TrackAddedEvent(17-22)
tests/test_utils.py (1)
agents-core/vision_agents/core/utils/video_utils.py (1)
ensure_even_dimensions(6-26)
plugins/getstream/tests/test_getstream_plugin.py (2)
agents-core/vision_agents/core/events/manager.py (1)
EventManager(56-546)agents-core/vision_agents/core/edge/events.py (2)
TrackAddedEvent(17-22)TrackRemovedEvent(26-31)
agents-core/vision_agents/core/agents/agents.py (5)
agents-core/vision_agents/core/edge/events.py (1)
TrackRemovedEvent(26-31)agents-core/vision_agents/core/utils/video_forwarder.py (1)
VideoForwarder(13-188)agents-core/vision_agents/core/utils/video_utils.py (1)
ensure_even_dimensions(6-26)agents-core/vision_agents/core/llm/realtime.py (2)
Realtime(23-201)_watch_video_track(68-70)plugins/getstream/vision_agents/plugins/getstream/stream_edge_transport.py (1)
add_track_subscriber(312-315)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: unit / Ruff & mypy
- GitHub Check: unit / Test "not integration"
- GitHub Check: unit / Ruff & mypy
plugins/getstream/vision_agents/plugins/getstream/stream_edge_transport.py
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
agents-core/vision_agents/core/agents/agents.py (1)
685-707: Task cancellation doesn't await cleanup.Line 697 cancels the task but doesn't await its completion. While the task handles cleanup via
CancelledError(lines 902-925), the cleanup is not guaranteed to finish before this handler returns. This creates a window where_active_video_tracks(line 701) still contains the track_id but the task is cancelling, contributing to the race condition flagged in the previous comment.Consider awaiting cancellation to ensure deterministic cleanup ordering:
if track_id in self._track_tasks: - self._track_tasks[track_id].cancel() + task = self._track_tasks[track_id] + task.cancel() + try: + await task + except asyncio.CancelledError: + pass self._track_tasks.pop(track_id)
🧹 Nitpick comments (2)
agents-core/vision_agents/core/agents/agents.py (2)
781-789: Consider extracting the even-dimensions wrapper.The inline
_EvenDimensionsTrackclass (lines 782-789) is defined inside the method, reducing reusability. While functionally correct, extracting it to module level would improve testability and potential reuse.Extract to module level below the imports:
class EvenDimensionsVideoTrack(VideoStreamTrack): """Wraps a video track to ensure frames have even dimensions for H.264 encoding.""" def __init__(self, source: VideoStreamTrack): super().__init__() self.source = source async def recv(self): frame = await self.source.recv() return ensure_even_dimensions(frame)Then use it in
_process_track:if track_type == TrackType.TRACK_TYPE_SCREEN_SHARE: - class _EvenDimensionsTrack(VideoStreamTrack): - def __init__(self, src): - super().__init__() - self.src = src - async def recv(self): - return ensure_even_dimensions(await self.src.recv()) - - track = _EvenDimensionsTrack(track) # type: ignore[arg-type] + track = EvenDimensionsVideoTrack(track)
865-901:consecutive_errorscounter is incremented but never used.Line 867 initializes
consecutive_errors, and line 892 increments it when empty frames are received, but no threshold check exists to trigger circuit-breaking or logging. Either implement a threshold check or remove the counter.If empty frames indicate a problem worth tracking:
else: self.logger.warning("🎥VDP: Received empty frame") consecutive_errors += 1 + if consecutive_errors >= 10: + self.logger.error( + f"🎥VDP: Too many consecutive empty frames ({consecutive_errors}), stopping" + ) + breakOtherwise, remove the unused variable:
-consecutive_errors = 0else: self.logger.warning("🎥VDP: Received empty frame") - consecutive_errors += 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
agents-core/vision_agents/core/agents/agents.py(5 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
📄 CodeRabbit inference engine (.cursor/rules/python.mdc)
**/*.py: Do not modify sys.path in Python code
Docstrings must follow the Google style guide
Files:
agents-core/vision_agents/core/agents/agents.py
🧬 Code graph analysis (1)
agents-core/vision_agents/core/agents/agents.py (7)
agents-core/vision_agents/core/edge/events.py (2)
TrackAddedEvent(17-22)TrackRemovedEvent(26-31)agents-core/vision_agents/core/utils/video_forwarder.py (4)
VideoForwarder(13-188)start(31-39)next_frame(83-101)stop(41-58)agents-core/vision_agents/core/utils/video_utils.py (1)
ensure_even_dimensions(6-26)agents-core/vision_agents/core/llm/realtime.py (2)
Realtime(23-201)_watch_video_track(68-70)plugins/gemini/vision_agents/plugins/gemini/gemini_realtime.py (2)
Realtime(52-676)_watch_video_track(385-424)plugins/openai/vision_agents/plugins/openai/openai_realtime.py (2)
Realtime(37-460)_watch_video_track(263-267)plugins/getstream/vision_agents/plugins/getstream/stream_edge_transport.py (1)
add_track_subscriber(312-315)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: unit / Test "not integration"
- GitHub Check: unit / Ruff & mypy
- GitHub Check: unit / Test "not integration"
- GitHub Check: unit / Ruff & mypy
🔇 Additional comments (5)
agents-core/vision_agents/core/agents/agents.py (5)
17-17: LGTM!The new imports (
TrackRemovedEvent,VideoForwarder,ensure_even_dimensions) are necessary for the track lifecycle and screensharing fixes.Also applies to: 36-37
164-166: LGTM!The new state variables properly track video track metadata and the currently active track. Type annotations and documentation are clear.
810-842: LGTM!The realtime mode handling correctly distinguishes between processed frames (when a video publisher exists) and raw frames, ensuring the appropriate forwarder is shared with the LLM provider. The redundant
isinstancechecks (lines 829, 838) provide defense-in-depth given the property-basedrealtime_modecheck.
902-925: CancelledError handling properly addresses the previous review.The cleanup block correctly catches
asyncio.CancelledError, stops both forwarders with proper error handling, and returns cleanly. This resolves the concern raised in the past review comment about missing cancellation guards.Based on previous review feedback.
667-679: Race condition verified; providers lack defensive checks for stopped forwarders.The concern is valid. The race window exists between cancelling the track task (line 697) without awaiting and removing it from
_active_video_tracks(line 701). Ifon_trackis re-added within this window, the code retrieves a forwarder that may be in the process of stopping.VideoForwarder has no guards against reuse after
stop()completes—_startedbecomesFalsebut there are no checks before reusing the instance. The Realtime providers (Gemini, OpenAI) pass the forwarder to downstream consumers without validating its state, risking frame delivery failures if the forwarder is already stopped.To fix: Either (1) await the task cancellation in
on_track_removedbefore removing from_active_video_tracks, or (2) add a null/validity check inon_track_re-additionand create a new forwarder if the stored one is no longer started, or (3) have Realtime providers validate forwarder state before use.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
plugins/getstream/tests/test_getstream_plugin.py (1)
18-69: Well-structured regression test with proper event flow validation.The test correctly simulates the track lifecycle (add → remove → add) and validates that all three events flow through the system. The use of
await event_manager.wait()instead of sleeps makes the test deterministic, addressing previous feedback.Consider adding cleanup to prevent potential test pollution:
# Before the fix: The agent would never receive this third event assert len(events) == 3, "Republishing track should emit TrackAddedEvent" assert isinstance(events[2], TrackAddedEvent) assert events[2].track_id == track_id + + # Cleanup + event_manager.unsubscribe(collect_track_events)
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
plugins/getstream/tests/test_getstream_plugin.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
📄 CodeRabbit inference engine (.cursor/rules/python.mdc)
**/*.py: Do not modify sys.path in Python code
Docstrings must follow the Google style guide
Files:
plugins/getstream/tests/test_getstream_plugin.py
🧬 Code graph analysis (1)
plugins/getstream/tests/test_getstream_plugin.py (2)
agents-core/vision_agents/core/events/manager.py (2)
EventManager(56-546)wait(470-484)agents-core/vision_agents/core/edge/events.py (2)
TrackAddedEvent(17-22)TrackRemovedEvent(26-31)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: unit / Ruff & mypy
- GitHub Check: unit / Test "not integration"
- GitHub Check: unit / Ruff & mypy
- GitHub Check: unit / Test "not integration"
🔇 Additional comments (3)
plugins/getstream/tests/test_getstream_plugin.py (3)
1-4: LGTM! Previous feedback addressed.The imports are clean and properly use the
TrackTypeenum instead of magic numbers, addressing the prior review feedback.
7-16: Excellent documentation of the regression test.The docstring clearly explains the bug, the fix, and the purpose of the test, making it easy for future maintainers to understand the context.
28-28: No changes required—Python 3.10+ is the project minimum.The project explicitly requires Python 3.10+, making the
|union operator syntax valid and appropriate. The code is correct as written.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
plugins/getstream/tests/test_getstream_plugin.py (1)
7-72: Deterministic republish regression test — niceGood use of EventManager.wait() and TrackType enum. Consider also asserting events[i].track_type == track_type for completeness. Please confirm the repo is configured to run bare async tests (no @pytest.mark.asyncio).
agents-core/vision_agents/core/agents/agents.py (5)
737-764: Snapshot dict before iterating to avoid RuntimeError on concurrent mutationevent handlers may mutate _active_video_tracks while iterating. Iterate over a snapshot.
- for track_id, (track_type, participant, forwarder) in self._active_video_tracks.items(): + for track_id, (track_type, participant, forwarder) in list(self._active_video_tracks.items()):
164-166: Type safety and initialization for per-track stateInitialize self._video_forwarders in init and prefer a small dataclass for track metadata for clarity.
self._active_video_tracks: Dict[str, tuple[int, Any, Any]] = {} self._current_video_track_id: Optional[str] = None + self._video_forwarders: list[VideoForwarder] = []Optionally:
@dataclass class VideoTrackInfo: track_type: int participant: Any forwarder: VideoForwarder | None
803-806: Remove lazy hasattr init; rely on initNow that _video_forwarders is initialized in init, drop the hasattr guard.
- # Track forwarders for cleanup - if not hasattr(self, "_video_forwarders"): - self._video_forwarders = [] - self._video_forwarders.append(raw_forwarder) + # Track forwarders for cleanup + self._video_forwarders.append(raw_forwarder)
667-679: Guard condition for track_typeAvoid falsy check that would drop 0-valued enums (e.g., TRACK_TYPE_UNSPECIFIED). Use explicit None check.
- if not track_id or not track_type: + if not track_id or track_type is None: return
765-926: General exception path should also clean forwardersYou handle asyncio.CancelledError, but other exceptions will leak running forwarders until close(). Add a generic except/finally to stop any created forwarders before exiting.
- except asyncio.CancelledError: + except asyncio.CancelledError: # Task was cancelled (e.g., track removed) @@ - return + return + except Exception as e: + self.logger.exception(f"Error in _process_track({track_id}): {e}") + # Best-effort cleanup, same as on cancellation + for fwd in (raw_forwarder, processed_forwarder): + if fwd is not None and hasattr(self, "_video_forwarders"): + try: + await fwd.stop() + if fwd in self._video_forwarders: + self._video_forwarders.remove(fwd) + except Exception: + self.logger.error("Error stopping forwarder after exception", exc_info=True) + return
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
agents-core/vision_agents/core/agents/agents.py(5 hunks)plugins/getstream/tests/test_getstream_plugin.py(1 hunks)plugins/getstream/vision_agents/plugins/getstream/stream_edge_transport.py(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
📄 CodeRabbit inference engine (.cursor/rules/python.mdc)
**/*.py: Do not modify sys.path in Python code
Docstrings must follow the Google style guide
Files:
plugins/getstream/vision_agents/plugins/getstream/stream_edge_transport.pyagents-core/vision_agents/core/agents/agents.pyplugins/getstream/tests/test_getstream_plugin.py
🧬 Code graph analysis (3)
plugins/getstream/vision_agents/plugins/getstream/stream_edge_transport.py (4)
agents-core/vision_agents/core/events/base.py (1)
user_id(43-46)agents-core/vision_agents/core/edge/events.py (1)
TrackAddedEvent(17-22)agents-core/vision_agents/core/agents/agents.py (1)
on_audio_received(647-656)agents-core/vision_agents/core/edge/types.py (1)
PcmData(37-183)
agents-core/vision_agents/core/agents/agents.py (5)
agents-core/vision_agents/core/edge/events.py (2)
TrackAddedEvent(17-22)TrackRemovedEvent(26-31)agents-core/vision_agents/core/utils/video_forwarder.py (4)
VideoForwarder(13-188)start(31-39)next_frame(83-101)stop(41-58)agents-core/vision_agents/core/utils/video_utils.py (1)
ensure_even_dimensions(6-26)agents-core/vision_agents/core/llm/realtime.py (2)
Realtime(23-201)_watch_video_track(68-70)plugins/getstream/vision_agents/plugins/getstream/stream_edge_transport.py (1)
add_track_subscriber(308-311)
plugins/getstream/tests/test_getstream_plugin.py (2)
agents-core/vision_agents/core/events/manager.py (3)
EventManager(56-546)wait(470-484)unsubscribe(272-297)agents-core/vision_agents/core/edge/events.py (2)
TrackAddedEvent(17-22)TrackRemovedEvent(26-31)
🔇 Additional comments (2)
plugins/getstream/vision_agents/plugins/getstream/stream_edge_transport.py (2)
283-283: Type-widened audio callback matches consumersAllowing pcm: PcmData | None aligns with Agent.on_audio_received guard (skips when pcm or participant is falsy). No issues.
112-126: Republish emits TrackAddedEvent — verified cleanRe-publish correctly emits TrackAddedEvent for non-agent tracks to unblock agent switching. Verification confirms user_metadata has been fully removed from the TrackAddedEvent class and all call sites pass only supported fields (plugin_name, track_id, track_type, user). LGTM.
plugins/getstream/vision_agents/plugins/getstream/stream_edge_transport.py
Show resolved
Hide resolved
maxkahan
force-pushed
the
fix-screensharing
branch
from
f086393 to
ca62d37
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
agents-core/vision_agents/core/agents/agents.py (2)
690-712: Stop the VideoForwarder when removing track to prevent resource leaks.When a track is removed, the code cancels the task and drops metadata but doesn't stop the still-running
raw_forwarder. The forwarder continues pulling frames from the input track until explicitly stopped.Apply this fix:
# Clean up track metadata +track_info = self._active_video_tracks.get(track_id) +if track_info: + # Stop the forwarder to prevent resource leaks + _, _, forwarder = track_info + if forwarder: + try: + await forwarder.stop() + if hasattr(self, "_video_forwarders") and forwarder in self._video_forwarders: + self._video_forwarders.remove(forwarder) + except Exception as e: + self.logger.error(f"Error stopping video forwarder for {track_id}: {e}") + self._active_video_tracks.pop(track_id, None)Based on learnings (past review comments).
742-768: Snapshot dict items before iteration to prevent RuntimeError.Line 750 iterates over
self._active_video_tracks.items()directly, but concurrent calls toon_trackoron_track_removedcan modify this dict during iteration, raisingRuntimeError. Async task switching can cause this even with the GIL.Apply this fix:
-for track_id, (track_type, participant, forwarder) in self._active_video_tracks.items(): +for track_id, (track_type, participant, forwarder) in list(self._active_video_tracks.items()):Based on learnings (past review comments).
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (6)
agents-core/vision_agents/core/agents/agents.py(5 hunks)agents-core/vision_agents/core/utils/video_utils.py(1 hunks)examples/other_examples/openai_realtime_webrtc/openai_realtime_example.py(1 hunks)plugins/getstream/tests/test_getstream_plugin.py(1 hunks)plugins/getstream/vision_agents/plugins/getstream/stream_edge_transport.py(4 hunks)tests/test_utils.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- agents-core/vision_agents/core/utils/video_utils.py
🧰 Additional context used
📓 Path-based instructions (2)
tests/**/*.py
📄 CodeRabbit inference engine (.cursor/rules/python.mdc)
tests/**/*.py: Never use mocking utilities (e.g., unittest.mock, pytest-mock) in test files
Write tests using pytest (avoid unittest.TestCase or other frameworks)
Mark integration tests with @pytest.mark.integration
Do not use @pytest.mark.asyncio; async support is automatic
Files:
tests/test_utils.py
**/*.py
📄 CodeRabbit inference engine (.cursor/rules/python.mdc)
**/*.py: Do not modify sys.path in Python code
Docstrings must follow the Google style guide
Files:
tests/test_utils.pyplugins/getstream/tests/test_getstream_plugin.pyexamples/other_examples/openai_realtime_webrtc/openai_realtime_example.pyagents-core/vision_agents/core/agents/agents.pyplugins/getstream/vision_agents/plugins/getstream/stream_edge_transport.py
🧬 Code graph analysis (4)
tests/test_utils.py (1)
agents-core/vision_agents/core/utils/video_utils.py (1)
ensure_even_dimensions(6-26)
plugins/getstream/tests/test_getstream_plugin.py (2)
agents-core/vision_agents/core/events/manager.py (2)
wait(470-484)unsubscribe(272-297)agents-core/vision_agents/core/edge/events.py (2)
TrackAddedEvent(17-22)TrackRemovedEvent(26-31)
agents-core/vision_agents/core/agents/agents.py (8)
agents-core/vision_agents/core/edge/events.py (1)
TrackRemovedEvent(26-31)agents-core/vision_agents/core/utils/video_forwarder.py (4)
VideoForwarder(13-188)start(31-39)next_frame(83-101)stop(41-58)agents-core/vision_agents/core/utils/video_utils.py (1)
ensure_even_dimensions(6-26)agents-core/vision_agents/core/llm/realtime.py (1)
_watch_video_track(68-70)plugins/gemini/vision_agents/plugins/gemini/gemini_realtime.py (1)
_watch_video_track(385-424)plugins/openai/vision_agents/plugins/openai/openai_realtime.py (1)
_watch_video_track(263-267)plugins/getstream/vision_agents/plugins/getstream/stream_edge_transport.py (1)
add_track_subscriber(310-313)agents-core/vision_agents/core/events/manager.py (1)
subscribe(299-368)
plugins/getstream/vision_agents/plugins/getstream/stream_edge_transport.py (4)
agents-core/vision_agents/core/events/manager.py (1)
send(426-468)agents-core/vision_agents/core/edge/events.py (1)
TrackAddedEvent(17-22)agents-core/vision_agents/core/agents/agents.py (1)
on_audio_received(652-661)agents-core/vision_agents/core/edge/types.py (1)
PcmData(37-183)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: unit / Test "not integration"
- GitHub Check: unit / Ruff & mypy
- GitHub Check: unit / Test "not integration"
- GitHub Check: unit / Ruff & mypy
🔇 Additional comments (16)
examples/other_examples/openai_realtime_webrtc/openai_realtime_example.py (1)
19-19: LGTM! Enhanced logging verbosity.Changing the log level to INFO provides better visibility into the agent's runtime behavior, which is helpful for debugging and monitoring in examples.
tests/test_utils.py (1)
446-481: LGTM! Comprehensive test coverage for ensure_even_dimensions.The test suite properly validates:
- Even dimensions pass through unchanged (1920×1080)
- Odd dimensions are cropped correctly (1921×1081 → 1920×1080)
- Frame timing metadata (pts, time_base) survives the crop
This aligns with the H.264 yuv420p encoding requirement for even dimensions mentioned in the PR objectives.
plugins/getstream/tests/test_getstream_plugin.py (1)
7-72: LGTM! Well-structured regression test for track republishing.The test properly validates the critical fix: when a user stops and restarts screensharing, the second
TrackAddedEventis now correctly emitted (line 67 assertion). The test flow is clear:
- Emit initial TrackAddedEvent → verify received
- Emit TrackRemovedEvent → verify received
- Emit TrackAddedEvent again → verify the republish event is received (the bug fix)
The implementation correctly follows coding guidelines: no
@pytest.mark.asynciodecorator (async support is automatic), usesevent_manager.wait()for deterministic timing instead of sleeps, and usesTrackType.TRACK_TYPE_SCREEN_SHAREenum instead of magic numbers.As per coding guidelines
plugins/getstream/vision_agents/plugins/getstream/stream_edge_transport.py (5)
109-112: LGTM! Agent track filtering prevents self-subscription.Skipping the agent's own tracks early avoids unnecessary subscription logic and potential feedback loops. The debug log provides visibility for troubleshooting.
117-130: LGTM! Core fix for track republishing.When a track is re-published (e.g., user stops and restarts screensharing), this code path now correctly:
- Marks the track as published in the map
- Retrieves the stored
track_id- Emits
TrackAddedEventso the agent can switch back to the trackThis directly addresses the bug described in the PR objectives where "republished tracks were not being shared with the AI."
168-176: LGTM! TrackAddedEvent emission for pending track resolution.Correctly emits the event after successfully resolving a pending track from the WebRTC layer. The payload matches the
TrackAddedEventschema (nouser_metadatafield).
285-285: LGTM! Signature allows optional PCM data.The
pcm: PcmData | Nonetype correctly reflects scenarios where audio is received but doesn't require direct processing. The maintainer confirmed this is intentional for logging received audio without necessarily acting on it.
400-403: LGTM! H.264 encoder aligns with even-dimension cropping.Switching from VP8 to H.264 pairs well with the new
ensure_even_dimensionsutility introduced in this PR, since H.264 with yuv420p format requires even dimensions. The demo parameters (1920×1080) are already even, ensuring compatibility.agents-core/vision_agents/core/agents/agents.py (8)
17-17: LGTM! Clean imports for new functionality.The new imports support track removal events, video forwarding infrastructure, and the screenshare dimension fix.
Also applies to: 36-37
164-166: Good initialization of track metadata structures.The instance variables for tracking active video tracks and current track ID are properly initialized in
__init__with clear type hints and documentation.
786-794: Excellent fix for screenshare dimension issues!The wrapper class that applies
ensure_even_dimensionsto screenshare tracks elegantly solves the non-even pixel bug mentioned in the PR objectives. The implementation is clean and only applies the fix where needed.
849-849: Good fix: Now following Python naming conventions.The variable now correctly uses
has_image_processors(snake_case) instead of the previous camelCasehasImageProcessers.
818-868: Realtime mode forwarder logic is well-structured.The conditional logic properly handles two scenarios:
- When a video publisher exists: forwards processed frames via a separate forwarder
- Otherwise: forwards raw frames directly
The early return when no image processors are present avoids unnecessary work.
907-930: Excellent CancelledError cleanup implementation.The exception handler properly stops and removes both raw and processed forwarders when the task is cancelled. The defensive checks prevent errors if forwarders weren't created, and error handling ensures cleanup continues even if stopping one forwarder fails.
874-906: Robust frame processing loop with proper error handling.The loop implementation includes:
- Timeout with exponential backoff to handle temporary frame starvation
- Error counting to track consecutive issues
- Reset on success to avoid penalizing transient errors
- Proper logging at appropriate levels
672-684: Verify GetStream SDK behavior for repeatedadd_track_subscribercalls.The re-add logic (line 680) calls
add_track_subscriber(track_id)again for a track already in_active_video_tracks, reusing the existing forwarder. However, without access to GetStream's SDK documentation or source code, it's unclear whether:
- Calling
add_track_subscribertwice on the sametrack_idis idempotent (safe)- It creates duplicate subscriptions or resource leaks
- A new subscription replaces the old one
The codebase consistently uses this pattern (initial subscription at line 780 and re-add at line 680), but no tests validate its safety. Recommend either:
- Checking GetStream SDK documentation/source for
subscriber_pc.add_track_subscriberidempotency guarantees- Adding a clarifying comment explaining why this pattern is safe
- Testing with GetStream to confirm behavior
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
agents-core/vision_agents/core/agents/agents.py (2)
743-770: Snapshot items before iteration to prevent concurrent modification.Line 751 iterates over
self._active_video_tracks.items()directly, but concurrent calls toon_trackoron_track_removedcould modify this dictionary during iteration, raisingRuntimeError. Take a snapshot first.Based on learnings (past review comments).
Apply this diff:
- for track_id, (track_type, participant, forwarder) in self._active_video_tracks.items(): + for track_id, (track_type, participant, forwarder) in list(self._active_video_tracks.items()):
691-714: Stop forwarder directly to prevent leaks when _process_track exits early.While task cancellation triggers cleanup in the
CancelledErrorhandler (lines 906-929), if_process_trackreturned early (e.g., at line 867 when there are no image processors), the forwarder remains running because the task is already complete. Explicitly stop the forwarder here to ensure cleanup in all cases.Apply this diff:
# Clean up track metadata - self._active_video_tracks.pop(track_id, None) + track_info = self._active_video_tracks.pop(track_id, None) + + # Stop the forwarder if present + if track_info and len(track_info) > 2: + forwarder = track_info[2] + if forwarder is not None: + try: + await forwarder.stop() + if forwarder in self._video_forwarders: + self._video_forwarders.remove(forwarder) + except Exception as e: + self.logger.error(f"Error stopping forwarder for {track_id}: {e}") # If this was the active track, switch to any other available track
🧹 Nitpick comments (1)
agents-core/vision_agents/core/agents/agents.py (1)
786-795: Consider extracting the wrapper class for clarity.The inline
_EvenDimensionsTrackclass definition correctly solves the screensharing bug by ensuring even dimensions. However, defining a class inside a method is unconventional and impacts readability.Consider extracting to a module-level helper or using a wrapper function:
Option 1: Module-level class
Add near the top of the file after imports:
class _EvenDimensionsTrack(VideoStreamTrack): """Wrapper that ensures video frames have even dimensions for H.264 encoding.""" def __init__(self, src): super().__init__() self.src = src async def recv(self): return ensure_even_dimensions(await self.src.recv())Then at line 795:
- track = _EvenDimensionsTrack(track) # type: ignore[arg-type] + track = _EvenDimensionsTrack(track) # type: ignore[arg-type]Option 2: Keep inline (current approach is acceptable if you prefer locality)
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
agents-core/vision_agents/core/agents/agents.py(5 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
📄 CodeRabbit inference engine (.cursor/rules/python.mdc)
**/*.py: Do not modify sys.path in Python code
Docstrings must follow the Google style guide
Files:
agents-core/vision_agents/core/agents/agents.py
🧬 Code graph analysis (1)
agents-core/vision_agents/core/agents/agents.py (7)
agents-core/vision_agents/core/edge/events.py (3)
AudioReceivedEvent(9-13)TrackAddedEvent(17-22)TrackRemovedEvent(26-31)agents-core/vision_agents/core/utils/video_forwarder.py (4)
VideoForwarder(13-188)start(31-39)next_frame(83-101)stop(41-58)agents-core/vision_agents/core/utils/video_utils.py (1)
ensure_even_dimensions(6-26)agents-core/vision_agents/core/llm/realtime.py (2)
Realtime(23-201)_watch_video_track(68-70)plugins/gemini/vision_agents/plugins/gemini/gemini_realtime.py (2)
Realtime(52-676)_watch_video_track(385-424)plugins/openai/vision_agents/plugins/openai/openai_realtime.py (2)
Realtime(37-460)_watch_video_track(263-267)plugins/getstream/vision_agents/plugins/getstream/stream_edge_transport.py (1)
add_track_subscriber(310-313)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: unit / Ruff & mypy
- GitHub Check: unit / Test "not integration"
- GitHub Check: unit / Ruff & mypy
- GitHub Check: unit / Test "not integration"
🔇 Additional comments (6)
agents-core/vision_agents/core/agents/agents.py (6)
17-17: LGTM! Import additions support the new functionality.The new imports for
TrackRemovedEvent,VideoForwarder, andensure_even_dimensionsare necessary for handling track removal events, managing video frame forwarding, and fixing the screensharing pixel dimension issue.Also applies to: 36-37
164-167: Good initialization of video track state.The instance variables are properly initialized in
__init__, addressing the past review concern about lazy initialization. The metadata structure clearly tracks the forwarder alongside track type and participant information, which will be essential for cleanup operations.
673-686: Excellent fix for republished track handling.This logic correctly addresses the issue where republished tracks were not being shared with the AI. By reusing the existing forwarder and re-subscribing to the track, you avoid creating duplicate tasks and maintain continuity in the video processing pipeline. The early return at line 685 is crucial to prevent creating a new processing task.
797-812: Well-structured VideoForwarder creation and metadata management.The shared
VideoForwarderprevents multiple consumers from competing ontrack.recv(), and storing the forwarder in the metadata tuple enables proper cleanup. The pattern of creating, starting, tracking, and storing metadata is clean and maintainable.
817-847: Appropriate handling of processed vs. raw video in realtime mode.The logic correctly distinguishes between scenarios where a video publisher exists (YOLO-processed frames) versus raw frames. Creating separate forwarders for processed video prevents interference with raw video processing, and the shared forwarder pattern ensures efficient frame delivery to the Realtime provider.
906-929: Excellent CancelledError cleanup implementation.The cleanup handler properly stops and removes both
raw_forwarderandprocessed_forwarder, addressing the past review concern about noisy tracebacks. The defensive checks (is not None,hasattr, membership tests) prevent errors during cleanup, and exception logging helps diagnose issues without crashing.
3 participants
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores