WebRTC: add implementation of ChatSessionRequest capability to start IM fix#146
WebRTC: add implementation of ChatSessionRequest capability to start IM fix#146Misterblue wants to merge 3 commits intoOpenSim-NGC:developfrom
Conversation
…bles IMs while using WebRTC voice module.
There was a problem hiding this comment.
Pull request overview
This PR adds implementation of the ChatSessionRequest capability to the WebRTC voice module to enable instant messaging functionality. The implementation handles P2P voice session start requests and other related methods, addressing an issue where IMs were not working while using the WebRTC voice module. The PR also includes code refactoring to extract common LLSD parsing logic into a reusable helper method.
Changes:
- Added ChatSessionRequest capability handler for P2P voice session initialization
- Extracted duplicate LLSD request body parsing logic into a reusable BodyToMap helper method
- Updated documentation comments to use proper XML doc format
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| addon-modules/os-webrtc-janus/WebRtcVoiceRegionModule/WebRtcVoiceRegionModule.cs | Main implementation: registers ChatSessionRequest capability, adds handler method with switch-case for different request types, extracts BodyToMap helper, includes various formatting fixes |
| addon-modules/os-webrtc-janus/WebRtcVoice/VoiceViewerSession.cs | Code formatting improvements (whitespace cleanup, property formatting) |
| addon-modules/os-webrtc-janus/WebRtcVoice/IWebRtcVoiceService.cs | Converts comment to XML documentation format |
| addon-modules/os-webrtc-janus/WebRtcVoice/IVoiceViewerSession.cs | Adds XML documentation comment |
| addon-modules/os-webrtc-janus/VERSION | Version bump from 2.1.1 to 2.1.2 |
| private OSDMap BodyToMap(IOSHttpRequest request, string pCaller) | ||
| { | ||
| OSDMap? map = null; | ||
| using (Stream inputStream = request.InputStream) | ||
| { | ||
| if (inputStream.Length > 0) | ||
| { | ||
| OSD tmp = OSDParser.DeserializeLLSDXml(inputStream); | ||
| if (_MessageDetails) m_log.DebugFormat("{0} BodyToMap: Request: {1}", pCaller, tmp.ToString()); | ||
| map = tmp as OSDMap; | ||
| } | ||
| } | ||
| return map; |
There was a problem hiding this comment.
The BodyToMap helper method returns a nullable OSDMap (OSDMap?), but the calling code checks using "is null" which is appropriate. However, there's an inconsistency in error handling - when the request body is empty or invalid, this method returns null and the caller returns NotFound (404). Consider whether BadRequest (400) would be more appropriate for invalid/empty request bodies, as NotFound typically indicates a missing resource rather than a malformed request.
There was a problem hiding this comment.
Two of the routines return HttpStatusCode.NoContent so the one request that returns NotFound would be better to return HttpStatusCode.NoContent.
There was a problem hiding this comment.
Changes made in subsequent commits
| { | ||
| response.StatusCode = (int)HttpStatusCode.NotFound; | ||
| return; | ||
| } | ||
|
|
||
| if (!reqmap.TryGetUUID("session-id", out UUID sessionID)) | ||
| { |
There was a problem hiding this comment.
Missing error logging when request validation fails. When method or session-id are missing from the request (lines 340-350), the handler returns NotFound without logging any diagnostic information. This makes debugging client integration issues difficult. Consider adding debug or warning log messages similar to line 318 to help diagnose why requests are being rejected.
| { | |
| response.StatusCode = (int)HttpStatusCode.NotFound; | |
| return; | |
| } | |
| if (!reqmap.TryGetUUID("session-id", out UUID sessionID)) | |
| { | |
| { | |
| m_log.Warn($"[WebRTC] ChatSessionRequest: missing required 'method' field in request for agent {agentID}"); | |
| response.StatusCode = (int)HttpStatusCode.NotFound; | |
| return; | |
| } | |
| if (!reqmap.TryGetUUID("session-id", out UUID sessionID)) | |
| { | |
| m_log.Warn($"[WebRTC] ChatSessionRequest: missing required 'session-id' field in request for agent {agentID}"); |
There was a problem hiding this comment.
changes made in subsequent commits
| return; | ||
| } | ||
|
|
||
| m_log.Debug($"[WebRTC] ChatSessionRequest"); |
There was a problem hiding this comment.
The comment on line 338 uses the generic "WebRTC" prefix instead of the more specific logHeader variable used elsewhere in the file. For consistency with the rest of the module's logging (e.g., lines 318, 412-414), this should use the logHeader variable and include the method context like other log statements in this file.
| m_log.Debug($"[WebRTC] ChatSessionRequest"); | |
| m_log.DebugFormat("{0}: ChatSessionRequest", logHeader); |
There was a problem hiding this comment.
Fixed in subsequent commits.
| queue.ChatterBoxSessionStartReply( | ||
| newSessionID, | ||
| OSD.FromString(sp.Name), | ||
| 2, | ||
| false, | ||
| true, | ||
| sessionID, | ||
| true, | ||
| string.Empty, | ||
| agentID); |
There was a problem hiding this comment.
The IEventQueue interface returned from RequestModuleInterface can be null if the EventQueue module is not loaded or available. Calling queue.ChatterBoxSessionStartReply without checking for null could result in a NullReferenceException. Add a null check before using the queue object, similar to how other parts of the codebase handle this pattern (e.g., using the null-conditional operator or an explicit null check with early return).
| queue.ChatterBoxSessionStartReply( | |
| newSessionID, | |
| OSD.FromString(sp.Name), | |
| 2, | |
| false, | |
| true, | |
| sessionID, | |
| true, | |
| string.Empty, | |
| agentID); | |
| if (queue != null) | |
| { | |
| queue.ChatterBoxSessionStartReply( | |
| newSessionID, | |
| OSD.FromString(sp.Name), | |
| 2, | |
| false, | |
| true, | |
| sessionID, | |
| true, | |
| string.Empty, | |
| agentID); | |
| } | |
| else | |
| { | |
| m_log.WarnFormat("{0}: EventQueue module not available; unable to send ChatterBoxSessionStartReply for agent {1} in scene {2}", logHeader, agentID, scene.RegionInfo.RegionName); | |
| } |
| IEventQueue queue = scene.RequestModuleInterface<IEventQueue>(); | ||
| queue.ChatterBoxSessionStartReply( | ||
| newSessionID, | ||
| OSD.FromString(sp.Name), |
There was a problem hiding this comment.
The second parameter to ChatterBoxSessionStartReply should be a string (sessionName), but OSD.FromString(sp.Name) is being passed. This is incorrect - OSD.FromString returns an OSD object, not a string. Simply pass sp.Name directly as the sessionName parameter instead of wrapping it with OSD.FromString.
| OSD.FromString(sp.Name), | |
| sp.Name, |
…nRequest. Correct parameters sent to ChatterBoxSessionStartReply. Return consistant error codes when request bodies are empty/unparsable.
…parsing. Check for null error if RequestModuleInterface<IEventQueue> fails (neven happens).
1 participant
Add implementation of the ChatSessionRequest capability which enables IMs while using the webrtc voice module. This is an initial solution as there are other functions of the capability that are not yet implemented.