Conversation
ead5974 to
2f13802
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR fixes two issues: a memory leak in MediaMixer and the inability to configure frame rate when attaching video devices. The key changes refactor device initialization to occur at attachment time rather than lazily, allowing configuration blocks to execute before devices are added to the capture session.
Key Changes:
- Moved device initialization from lazy creation to attachment time, enabling proper configuration
- Changed
isMultiTrackAudioMixingEnabledfrom computed to stored property to fix memory leak - Added test coverage for memory leak fixes and video configuration functionality
Reviewed Changes
Copilot reviewed 9 out of 10 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| HaishinKit/Sources/Mixer/MediaMixer.swift | Removed initialization task and changed isMultiTrackAudioMixingEnabled to stored property; simplified attachVideo to delegate configuration to VideoCaptureUnit |
| HaishinKit/Sources/Mixer/VideoCaptureUnit.swift | Refactored attachVideo to create VideoDeviceUnit instances at attachment time and apply configuration before session attachment |
| HaishinKit/Sources/Mixer/VideoDeviceUnit.swift | Changed initialization to require device parameter; moved device setup from attachDevice to initializer; renamed data output class |
| HaishinKit/Sources/Mixer/AudioCaptureUnit.swift | Refactored attachAudio to create AudioDeviceUnit at attachment time; added isMultiTrackAudioMixingEnabled parameter to initializer |
| HaishinKit/Sources/Mixer/AudioDeviceUnit.swift | Changed initializer to require device parameter and perform full initialization; removed attachDevice method |
| HaishinKit/Sources/Mixer/DeviceUnit.swift | Updated protocol to require device parameter in initializer |
| HaishinKit/Tests/Mixer/MediaMixerTests.swift | Added test for video configuration and uncommented memory leak test |
| HaishinKit/Tests/Mixer/VideoDeviceUnitTests.swift | Added new test file for VideoDeviceUnit memory leak verification |
| HaishinKit/Tests/Mixer/AudioDeviceUnitTests.swift | Added new test file for AudioDeviceUnit memory leak verification |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| return | ||
| } | ||
| try? configuration?(capture) | ||
| session.detachCapture(self.devices[track]) |
There was a problem hiding this comment.
Potential crash when detaching a nil device. If devices[track] is nil (e.g., on first attachment), calling detachCapture with nil may cause issues depending on the implementation. Consider checking if the device exists before detaching: if let device = self.devices[track] { session.detachCapture(device) }
| session.detachCapture(self.devices[track]) | |
| if let device = self.devices[track] { | |
| session.detachCapture(device) | |
| } |
| try session.configuration { _ in | ||
| for capture in devices.values where capture.device == device { | ||
| try? capture.attachDevice(nil, session: session, audioUnit: self) | ||
| session.detachCapture(self.devices[track]) |
There was a problem hiding this comment.
Potential crash when detaching a nil device. If devices[track] is nil (e.g., on first attachment), calling detachCapture with nil may cause issues depending on the implementation. Consider checking if the device exists before detaching: if let device = self.devices[track] { session.detachCapture(device) }
| session.detachCapture(self.devices[track]) | |
| if let device = self.devices[track] { | |
| session.detachCapture(device) | |
| } |
Description & motivation
MediaMixer.Type of change
Screenshots: