Add multi-stream device support and improve ACX topology, ASIO compatibility, and audio performance - #34
Add multi-stream device support and improve ACX topology, ASIO compatibility, and audio performance#34Akihiro Tokumasu (ymh-akihiro-tokumasu) wants to merge 77 commits into
Conversation
- Simplify and centralize access to the Control Interface - Change array access in USBAudioConfiguration to range-based iteration
…lti-stream support - Consolidate audio transfer logic from C functions in Device.cpp into per-clock AudioIsochronousEngine classes - Move descriptor management to per-clock USBAudioStreamInterfaceGroup classes
…nd device interfaces - Correct handling of Clock Selector Units for multi-stream support - Fix BSOD when accessing circuit pins - Correct ACX render/capture behavior and device splitting logic - Fix ASIO device selection and sample rate handling issues - Update INF AddInterface for multi-stream support - Correct handling of device interface strings
…y to user space - Select MixingEngineThread based on Interface Descriptor structure - Allow user-space components to determine ASIO availability
…nd resolve conflicts
- Refactor shared Render/Capture Circuit routines into CircuitCommon.cpp - Add request handler for KSPROPERTY_TYPE_BASICSUPPORT - Add Selector Unit control via USBAudioConfiguration - Fix SAL annotation issues - Fix ASIOGetClockSources regression caused by multi-stream support changes
… topology - Add parsing logic to generate ACX Circuit elements based on USB Audio 2.0 topology - Update comments for CS_AC_MIXER_UNIT_DESCRIPTOR definition
- Refine and extend ACX Pin generation - Rename Sink/Source handling to Forward/Reverse in descriptor parsing to avoid confusion with ACX semantics - Add conversion from USB Audio 2.0 speaker positions to WAVEFORMATEXTENSIBLE - Add bmControls definitions for CS_AC_INPUT_TERMINAL_DESCRIPTOR and CS_AC_OUTPUT_TERMINAL_DESCRIPTOR
- Begin implementing Codec_CreateAgcElement, Codec_CreateMuxElement, and Codec_CreateSuperMixElement - Add partial support for Super Mix Element - Extend parsing for Extension Unit Descriptor - Update to use EVT_ACX_OBJECT_PROCESS_REQUEST definition
…criptors - Update Volume and Mute elements to retrieve configuration from descriptors - Fix issue in Super Mix Element information retrieval - Add descriptor-based configuration for Mux and AGC elements - Add connection logic for Pins and Elements
…agement - Add KSDATARANGE_AUDIO configuration to Circuits - Prepare shared ACXVOLUME and ACXMUTE management in common Circuit context for Render/Capture
…or notification callbacks - Implement Codec_AllocateElements invocation during Circuit generation for devices with multiple Clock Sources - Refactor VolumeChangeLevelNotification, MuteChangeStateNotification, and ConnectorChangeStateNotification callbacks to be callable from both Render and Capture paths
…t connections - Fix missing ACX audio streaming on devices with multiple clock sources - Align AGC, Mute, and Volume element creation order in Feature Unit with the inbox driver - Remove channel count limitation in Feature Unit - Fix output connection issues for Super Mix and Mux elements
…lector Unit behavior
…g issues - Add support for branching from a single unit to multiple units - Fix handling when AC Input Terminal Descriptor bNrChannels is zero - Refactor member naming
… avoid unnecessary stack usage
…pin mapping - Replace fixed limits for ACXPIN, ACXELEMENT, and ACX_CONNECTION with dynamic allocation - Convert Terminal Type to Pin Category mapping into a lookup table
…lti-stream and input-only devices - Distinguish streams in Control Panel settings for multi-stream devices - Enable Control Panel interaction for devices with input-only streams
- Use bmChannelConfig from Terminal Descriptor to generate Jack channel mapping - Set Jack GeoLocation to AcxGeoLocNotApplicable when Connector Control is unavailable
…ine ACX buffer constraints - Stop applying default configuration on device connection for devices using Feature Unit control - Limit Control Panel interaction for devices with multiple clock sources - Refine ACX buffer constraints
Apply the suggested fix from Copilot code review. The unconditional assignment to *outSample overwrote the saturated values, causing the clipping logic to be bypassed. Add an else branch so that saturated samples are preserved. Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Address additional findings reported by Copilot code review. - Preserve the existing USB audio data format when a duplicate format is detected. - Correct VariableArray element tracking for non-sequential indices. - Rename ConverSpeakerPositions() to ConvertSpeakerPositions(). - Update outdated comments describing sample rate handling in PrepareHardware().
Pete Brown (Psychlist1972)
left a comment
There was a problem hiding this comment.
Attribution: the findings in this review and in the individual line comments that follow were
produced by GitHub Copilot (Claude Opus 5), reviewing the diff against merge base2e647f4c
at head7af4dda. I have read them and agree they are worth acting on, but the analysis and the
line references are the model's. Please push back on anything it got wrong - a couple of items are
explicitly flagged as needing your confirmation.
Thanks for this. I understand why it had to be one change - the object model, descriptor parser, ACX
topology and transfer engine are too interdependent to unpick after the fact, and I am not going to
ask you to split it now. Taking it as it is.
I had Copilot do a focused pass on the defect classes that hurt most in a kernel driver: races,
TOCTOU, lock ordering, use-after-free, teardown lifetime, and error paths that silently report
success. It did not review all 20k lines - see "Not covered" below.
Separately: once this merges I want a thorough review of the whole codebase, not just the diff. That
is a follow-up effort, out of scope here, but it is coming. So if any of the pre-existing items below
feel like they belong to that effort rather than to this PR, that is a reasonable call - say so and
we will track them instead.
Summary
7 defects introduced by this PR. I would like these fixed on this branch before merge. They are
all small, localized changes. Details are in individual line comments.
AudioIsochronousEngine::SetAsioBufferreturnsSTATUS_SUCCESSwhen the allocation fails- Out-of-bounds read in the new Mixer Unit descriptor parser (untrusted USB input)
VariableArray::Setleaves the object in a state where the nextSetdoes a null write- Engine can be deleted while ACX circuit/pin contexts still hold un-refcounted pointers to it
- A second
EvtDevicePrepareHardwareproduces a device with no engines and no configuration GetCaptureStreamEngine/GetRenderStreamEngineread state that is written under a lockm_asioBufferOwneris declared but never assigned, so ASIO buffer ownership is not enforced
6 pre-existing issues that this PR moves code around, so it is a natural moment to fix them. I am
not asking for all of these here - happy to take most as tracked issues. But the first two are the
ones I care about:
- user-supplied pointers locked with
MmProbeAndLockPages(..., KernelMode, ...) TransferObject::CancelRequestcan time out and the caller frees the object anyway- double-fetch of the user-writable ASIO header between validation and use
- unvalidated
length - offsetsubtraction on user input - capture buffer locked
IoReadAccessthen written - assorted: missing null check before
KeSetEvent,Outpipe not aborted, ARM64 alignment
What was checked and found sound
Worth saying explicitly, because a lot of this is new and it holds up:
- No ABBA. The three
WDFWAITLOCKs are consistently taken in the order Stream -> Asio ->
StreamEngine on every path examined (D0Exit,StartAsioStream,FileCleanup,StartIsoStream,
MixingEngineThreadMain,StreamPrepareHardware). Nothing takes them in reverse, and nothing
takes a wait lock while holding a spin lock. MixingEngineThreadMainlock balance is correct. Everybreakinside a locked region is an
inner-loop break; the two outer-loop breaks both sit in unlocked windows. This was checked
statement by statement - it is easy to get wrong and you did not.WorkerThread::Terminateis a real join -KeWaitForSingleObjecton the thread object with no
timeout beforeObDereferenceObject.- The
outputReadyEventreference handling inMixingEngineThreadMainis correct -
ObReferenceObjectunder the lock, published, then unpublished and dereferenced afterWait(). UnsetBuffernow releasing both event references is a genuine fix for a pre-existing leak.- The
samplesFirsthoist inCopyToAsioFromInputDatafixes a real wrap-around overflow. m_numOfInputDevicesis assigned only after its array allocation succeeds, so the unguarded array
index inGetCaptureStreamEnginecannot hit a null array. Still worth annotating, but not a live
bug.
Not covered
CircuitCommon.cpp (2,596 new lines), most of USBAudioConfiguration.cpp, CaptureCircuit.cpp,
RenderCircuit.cpp, StreamEngine.cpp, InterruptDataMessage.cpp, ContiguousMemory.cpp, and the
user-mode uac2-asio DLL. The descriptor parser in particular wants a dedicated pass with a
malicious-descriptor fuzzer - finding #2 was in the first section of that file that was opened.
Validation ask
Before merge, please run under Driver Verifier with Force IRQL Checking, Pool Tracking and
Low Resources Simulation enabled, plus a surprise-removal-during-ASIO-streaming loop. That
combination targets findings #3 and the two pre-existing lifetime issues directly, and low-resources
simulation is the only practical way to exercise the allocation-failure paths in #1 and #3.
…ails Fix an issue where STATUS_SUCCESS was returned when AsioBufferObject creation failed. Co-authored-by: Pete Brown <psychlist1972@outlook.com>
- Add MAX_CAPACITY validation to prevent invalid growth requests - Preserve original container state when allocation fails - Document parentObject lifetime requirements - Clarify Allocate() success-only state updates
Add reference counting for AudioIsochronousEngine pointers stored in circuit, pin, and request contexts. Each context now takes a reference when storing the engine pointer and releases it during cleanup or request completion. This prevents the engine from being destroyed while outstanding contexts still reference it.
Allow AddRef and Release to be called at DISPATCH_LEVEL by moving them from paged to nonpaged code and updating their IRQL annotations.
|
Akihiro Tokumasu (@ymh-akihiro-tokumasu) I've converted the PR to draft. As soon as you have completed the open items, please change it to ready and I'll re-review. Thanks! Pete |
Copy play and record buffer headers into local variables before validation and use the local copies for all subsequent checks and calculations. Also add bounds checks for buffer offsets, lock the record buffer with modify access, and guard notification event signaling against null event objects.
- Introduce RequestState to replace m_isRequested - Detach AudioIsochronousEngine and TransferObject from the request context before cancellation - Serialize ownership transfer between cancellation and completion paths using interlocked operations - Prevent cancellation timeout paths from dereferencing objects no longer owned through ISOCHRONOUS_REQUEST_CONTEXT - Remove StreamObject from ISOCHRONOUS_REQUEST_CONTEXT - Add cleanup coverage in the StreamObject destruction path - Abort Out pipe when stopping isochronous transfers for consistency
- Support devices that perform sample rate switching on the hardware side - Add verified devices to USBAudio2-ACX.inf
Summary
This Pull Request adds support for USB audio devices that expose multiple Audio Streaming Interfaces, such as the Creative Sound Blaster G3.
To support multi-stream devices, a new Audio Engine has been implemented. As part of this work, the driver's internal class structure, USB Audio Descriptor Parser, stream management, and ACX Circuit generation have been reviewed and reorganized.
Main changes
Known limitations
Devices with multiple independent asynchronous clocks may still have inherent ASIO compatibility limitations, because ASIO may not be able to correctly synchronize all streams across independent clock domains.
Some behavioral differences have also been observed in Feature Unit and ACX topology handling. These may be related to ACX constraints or device behaviors outside the assumptions of the current design. These items will be raised as separate issues after this Pull Request and investigated individually.
Validation
The changes have been tested with multiple USB Audio 2.0 devices and DAWs, including Render, Capture, ASIO, Windows Audio Engine, and multi-stream operation.
Performance with small ASIO buffer sizes has also been confirmed to improve compared with the previous
mainbranch implementation.Next steps
This Pull Request consolidates the larger changes developed on the
multi-streams-supportbranch.After this Pull Request, future fixes, commits, validation, and Pull Requests will be handled in smaller and shorter development cycles.
Thank you for reviewing this contribution.