fix screensharing when screen height/width is an odd pixel number #271
Conversation
📝 WalkthroughWalkthroughAdds logging for track add/remove events, enforces even frame dimensions by cropping odd-width/height frames before enqueueing, tweaks a Roboflow parameter/import, and adds unit tests for ensure_even_dimensions. Changes
Sequence Diagram(s)(No sequence diagrams generated: changes are logging, frame preprocessing, tests, and small refactors without new multi-component control flow.) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Disabled knowledge base sources:
📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ 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)
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
📜 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 (4)
agents-core/vision_agents/core/agents/agents.pyagents-core/vision_agents/core/utils/video_track.pyplugins/roboflow/example/roboflow_example.pyplugins/roboflow/vision_agents/plugins/roboflow/roboflow_local_processor.py
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
📄 CodeRabbit inference engine (.cursor/rules/python.mdc)
**/*.py: Never adjust sys.path in Python code
Never writeexcept Exception as e- use specific exception handling
Avoid using getattr, hasattr, delattr and setattr; prefer normal attribute access in Python
Docstrings should follow the Google style guide for docstrings
Files:
agents-core/vision_agents/core/agents/agents.pyplugins/roboflow/example/roboflow_example.pyagents-core/vision_agents/core/utils/video_track.pyplugins/roboflow/vision_agents/plugins/roboflow/roboflow_local_processor.py
🧠 Learnings (2)
📓 Common learnings
Learnt from: dangusev
Repo: GetStream/Vision-Agents PR: 249
File: agents-core/vision_agents/core/agents/agents.py:1032-1042
Timestamp: 2025-12-10T19:35:39.238Z
Learning: In `agents-core/vision_agents/core/agents/agents.py`, when using `video_track_override_path`, creating a new `VideoFileTrack` for each participant (each call to `_on_track_added`) is intentional to maintain proper track lifecycle semantics tied to each participant.
📚 Learning: 2025-12-10T19:35:39.238Z
Learnt from: dangusev
Repo: GetStream/Vision-Agents PR: 249
File: agents-core/vision_agents/core/agents/agents.py:1032-1042
Timestamp: 2025-12-10T19:35:39.238Z
Learning: In `agents-core/vision_agents/core/agents/agents.py`, when using `video_track_override_path`, creating a new `VideoFileTrack` for each participant (each call to `_on_track_added`) is intentional to maintain proper track lifecycle semantics tied to each participant.
Applied to files:
agents-core/vision_agents/core/agents/agents.pyagents-core/vision_agents/core/utils/video_track.pyplugins/roboflow/vision_agents/plugins/roboflow/roboflow_local_processor.py
🧬 Code graph analysis (3)
agents-core/vision_agents/core/agents/agents.py (3)
agents-core/vision_agents/core/edge/sfu_events.py (19)
track_type(579-583)track_type(1193-1197)track_type(2289-2293)user_id(489-493)user_id(856-860)user_id(901-905)user_id(1186-1190)user_id(2093-2097)user_id(2142-2146)participant(1496-1501)participant(1504-1507)participant(1545-1550)participant(1553-1556)participant(1625-1630)participant(1633-1636)participant(2100-2105)participant(2108-2111)participant(2156-2161)participant(2164-2167)agents-core/vision_agents/core/events/base.py (1)
user_id(45-48)conftest.py (1)
participant(187-189)
plugins/roboflow/example/roboflow_example.py (1)
plugins/roboflow/example/utils.py (1)
Debouncer(4-19)
plugins/roboflow/vision_agents/plugins/roboflow/roboflow_local_processor.py (1)
agents-core/vision_agents/core/utils/video_forwarder.py (1)
VideoForwarder(25-185)
⏰ 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 / Mypy
- GitHub Check: unit / Test "not integration"
- GitHub Check: unit / Mypy
- GitHub Check: unit / Test "not integration"
🔇 Additional comments (6)
agents-core/vision_agents/core/agents/agents.py (2)
974-978: LGTM: Enhanced video track observability.The logging addition provides valuable visibility into which source track is selected and its priority. The distinction between "SCREEN_SHARE" and "VIDEO" track types will aid in debugging track selection logic.
Minor note: Line 977 uses the module-level
loggerrather thanself.logger. While both work, consider usingself.loggerfor consistency with the rest of the agent class (e.g., lines 1010, 1022) and to include the agent_id context from the LoggerAdapter.
1007-1011: LGTM: Valuable track lifecycle logging.This logging provides essential visibility into video track additions, including the track type and contributing user. The fallback to "unknown" for missing participants is appropriate defensive coding.
agents-core/vision_agents/core/utils/video_track.py (2)
78-79: LGTM: Correct application in frame queuing.The
ensure_even_dimensionscall is properly placed before queuing frames, ensuring all buffered frames comply with H.264 encoding requirements. The comment clearly explains the rationale.
232-233: LGTM: Consistent application in file track.The
ensure_even_dimensionscall is correctly placed to ensure frames from video files also have even dimensions. This is especially important for the video override path mentioned in the learnings, whereVideoFileTrackis used for testing/debugging.plugins/roboflow/vision_agents/plugins/roboflow/roboflow_local_processor.py (2)
161-166: LGTM: Cleaner parameter naming.The renaming from
incoming_tracktotracksimplifies the method signature and improves readability. Since theVideoForwarderis called with a positional argument (line 182), this change doesn't affect functionality.
182-182: LGTM: Consistent with parameter renaming.The updated reference to
trackis consistent with the method signature change.
maxkahan
force-pushed
the
fix-screenshare-odd-dimensions
branch
from
9a4f048 to
60cdcf1
Compare
dangusev
left a comment
dangusev
left a comment
There was a problem hiding this comment.
One more thing:
Right now, ensure_even_dimensions scales the image instead of cropping it.
And rescaling is a more expensive operation.
Can you please change it to crop the frame and add tests?
Thanks.
|
@maxkahan what's the status of this PR? |
|
@dangusev @Nash0x7E2 made Daniil's edits, ready for re-review |
d3xvn
dismissed
dangusev’s stale review
Dismissing to merge since changes were complete.
Summary by CodeRabbit
Bug Fixes
Improvements
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.