diff --git a/RTC_TODO.md b/RTC_TODO.md new file mode 100644 index 00000000..835237ba --- /dev/null +++ b/RTC_TODO.md @@ -0,0 +1,76 @@ +# RTC TODO + +## Approach + +1. vibe code mass testing to get broad coverage fast +2. vibe code refactoring (naming, dead code, restructure) +3. hand craft production code (libdatachannel, peer management, SDP) +4. hand craft testing — thin it out to essentials only + +## Unit Tests + +| Function | Rating | Assertions | +|---|---|---| +| `create_peer_connection` | essential | returns valid peer connection id | +| `add_audio_track` | essential | returns valid track id | +| `add_video_track` | essential | returns valid track id | +| `negotiate_sdp` | essential | returns sdp answer from valid offer, returns None on timeout | +| `delete_peers` | essential | clears peer list | +| `create_rtc_stream` | essential | initializes empty peer list | +| `destroy_rtc_stream` | essential | cleans up peers, handles missing session | +| `add_rtc_viewer` | nice to have | returns None for unknown session | +| `resolve_binary_file` | skip | covered implicitly by `create_static_rtc_library` | +| `handle_whep_offer` | skip | thin wrapper, covered by integration tests | +| `send_to_peers` | skip | needs open tracks, covered by integration tests | +| `send_rtc_frame` | skip | thin delegation to `send_to_peers` | + +## Integration Tests + +- [ ] test full SDP offer/answer roundtrip between two peer connections +- [ ] test `send_to_peers` delivers frame data to a connected peer +- [ ] test peer cleanup after `delete_peers` prevents further sends +- [ ] test `add_rtc_viewer` followed by `send_rtc_frame` end-to-end +- [ ] test multiple viewers receive frames from same stream +- [ ] test viewer disconnect does not break remaining peers + +## Dead Code + +- [ ] remove `is_peer_connected` from `rtc.py`, never called +- [ ] remove `pre_check` from `rtc.py`, never called +- [ ] remove `get_rtc_stream` from `rtc_store.py`, never called +- [ ] remove `RtcOfferSet` from `types.py`, never used + +## Naming + +- [ ] rename `rtc_bindings.py` to `libdatachannel.py` — owns C library loading, struct definitions, ctypes registration +- [ ] rename `RTC_CONFIGURATION` to a proper class definition +- [ ] rename `RTC_PACKETIZER_INIT` to a proper class definition +- [ ] rename `init_ctypes` to something more specific like `register_argtypes` + +## Unused Library Config + +- [ ] `create_peer_connection` exposes 14 params but production only uses `disable_auto_negotiation`, `enable_ice_udp_mux`, `force_media_transport` — reduce to what is needed +- [ ] `RTC_PACKETIZER_INIT` defines `nalSeparator`, `obuPacketization`, `playoutDelayId`, `playoutDelayMin`, `playoutDelayMax`, `sequenceNumber`, `timestamp` — none are set outside the struct definition, they are H264/AV1 specific and unused for VP8/Opus +- [ ] `rtcSetLocalDescription` is used in tests but not registered in `rtc_bindings.py` + +## Refactor + +- [ ] extract shared media description builder from `rtc.py` and `tests/stream_helper.py` (see TODO in `tests/stream_helper.py`) +- [ ] replace `type()` calls for ctypes structs in `rtc_bindings.py` with proper class definitions +- [ ] move `resolve_binary_file`, `create_static_download_set`, `create_static_rtc_library` from `rtc.py` into `libdatachannel.py` — library init belongs with the bindings, `rtc.py` just consumes it +- [ ] move `rtc_store.py` state into a proper store pattern consistent with other `*_store.py` files +- [ ] replace `time.sleep` polling loops in `negotiate_sdp` and `create_sdp_offer` (test helper) with `rtcSetGatheringStateChangeCallback` — signal a `threading.Event` on ICE gathering complete, then `event.wait(timeout=5)` instead of spinning +- [ ] `create_static_download_set` has hardcoded linux URLs with a TODO to use dynamic `binary_name` + +## Violations + +- `rtc.py:25` — comment on `create_static_download_set` (no comments) +- `rtc.py:34` — comment on hash url (no comments) +- `rtc.py:42` — comment on source url (no comments) +- `rtc.py:205` — `send_to_peers` returns None explicitly on a void function (redundant) +- `rtc_bindings.py:3,23` — `type()` to create structs is a class workaround (no classes, but these should be plain structs) +- `tests/stream_helper.py:45` — `sdp` temp variable before return (no need for temporary variable in simple cases) + +## Security + +- `rtc.py:157` — `negotiate_sdp` passes unsanitized `sdp_offer` string directly to C library, no SDP format validation before hitting native code diff --git a/STREAM_API_TODO.md b/STREAM_API_TODO.md new file mode 100644 index 00000000..141531b5 --- /dev/null +++ b/STREAM_API_TODO.md @@ -0,0 +1,60 @@ +# Stream API TODO + +## Approach + +1. vibe code mass testing to get broad coverage fast +2. vibe code refactoring (naming, boilerplate extraction, validation) +3. hand craft production code (stream pipeline, encoder loop, IVF parsing) +4. hand craft testing — thin it out to essentials only + +## Unit Tests + +| Function | Rating | Assertions | +|---|---|---| +| `get_websocket_stream_mode` | essential | returns None for missing header, returns None for unknown protocol | +| `forward_rtc_frames` | essential | reads IVF header and forwards frame data, handles broken pipe | +| `run_encode_loop` | nice to have | drains deque and closes encoder | +| `receive_vision_frames` | skip | async generator, covered by integration tests | +| `submit_encoder_frame` | skip | thin glue between cv2 and subprocess stdin | +| `websocket_stream` | skip | routing only, covered by integration tests | +| `post_stream` | skip | covered by integration tests | + +## Integration Tests + +- [ ] test stream without session — expect rejection +- [ ] test stream with expired or invalid token +- [ ] test image stream without source selected +- [ ] test video stream without source selected +- [ ] test WHEP offer without active websocket stream +- [ ] test WHEP offer with malformed SDP body +- [ ] test WHEP offer with wrong content type +- [ ] test multiple WHEP viewers on same stream +- [ ] test websocket disconnect mid-stream triggers cleanup + +## Naming + +- [ ] rename `test_get_stream_mode` to `test_get_websocket_stream_mode` in `test_stream_helper.py` — does not match function name +- [ ] rename `make_scope` in `test_stream_helper.py` to `make_websocket_scope` — more descriptive +- [ ] `stream_helper.py` mixes pure helpers (`calculate_bitrate`) with async handlers (`handle_image_stream`) — consider splitting + +## Dead Code + +- [ ] `read_pipe_buffer` has a test but the test is disconnected from how it is actually used — the test reads from a closed pipe, production reads from a live subprocess stdout + +## Refactor + +- [ ] `handle_image_stream` and `handle_video_stream` share session setup boilerplate (subprotocol, access_token, session_id, source_paths, websocket accept) — extract common setup +- [ ] `forward_rtc_frames` assumes IVF container format with hardcoded header size 32 and frame header size 12 — document or make configurable +- [ ] `post_stream` does not validate content type is `application/sdp` before parsing body +- [ ] `calculate_bitrate` has a TODO about improving the calculation +- [ ] `handle_video_stream` has a hardcoded fallback `output_video_fps or 30` with a TODO to resolve from target video fps + +## Violations + +- `stream_helper.py:24` — comment on `calculate_bitrate` (no comments) +- `stream_helper.py:143` — comment on `output_video_fps` fallback (no comments) +- `test_stream_helper.py:30` — `test_get_stream_mode` does not match function name `get_websocket_stream_mode` (naming convention) + +## Security + +- `endpoints/stream.py:27` — `request.body().decode()` is not sanitized, raw user input forwarded to C library via RTC layer diff --git a/tests/stream_helper.py b/tests/stream_helper.py index b60bb9ff..c22c927d 100644 --- a/tests/stream_helper.py +++ b/tests/stream_helper.py @@ -10,6 +10,7 @@ from facefusion import rtc from facefusion.types import RtcSdpOffer +# TODO: reuse media description building from rtc.py def create_sdp_offer() -> Optional[RtcSdpOffer]: rtc_library = rtc.create_static_rtc_library() peer_connection = rtc.create_peer_connection(disable_auto_negotiation = True) @@ -43,7 +44,9 @@ def create_sdp_offer() -> Optional[RtcSdpOffer]: if rtc_library.rtcGetLocalDescription(peer_connection, buffer_string, buffer_size) > 0: sdp = buffer_string.value.decode() rtc_library.rtcDeletePeerConnection(peer_connection) + #TODO: use return buffer_string.value.decode() return sdp + time.sleep(0.05) rtc_library.rtcDeletePeerConnection(peer_connection) diff --git a/tests/test_api_stream.py b/tests/test_api_stream.py index ad2813e8..cd60ef4a 100644 --- a/tests/test_api_stream.py +++ b/tests/test_api_stream.py @@ -132,6 +132,7 @@ def test_stream_video(test_client : TestClient) -> None: ready_event = threading.Event() stop_event = threading.Event() + #TODO: use asyncio stream_thread = threading.Thread(target = open_websocket_stream, args = (test_client, [ 'access_token.' + access_token, 'video' ], source_content, ready_event, stop_event)) stream_thread.start() ready_event.wait()