VoipManager / VoipGroupManager review — to-do

Line-by-line review of Telegram.Native.Calls/VoipManager.{h,cpp} and Telegram.Native.Calls/VoipGroupManager.{h,cpp}, cross-checked against the C# call sites in Telegram/Services/Calls/ and against tgcalls' own expectations.

Line numbers are as of c533f04c0, i.e. before the P0 commits below; anything still open has shifted by a few lines since.

Legend: [live] = confirmed reachable from current app code · [latent] = correct today only by convention or because no caller exercises it.

Build: msbuild Telegram.Native.Calls\Telegram.Native.Calls.vcxproj /p:Configuration=Debug /p:Platform=x64 /m with VS 18 Community. Takes a few minutes and links clean; the C4715 and C4702 warnings it prints are pre-existing.

P0 — memory safety and security — done

All five landed on develop as 4eda2af98..c4bbb77f6, one fix per commit, built x64 Debug.

Unchecked get_self on a VoipCaptureBase — VoipManager.cpp:215-219 [live] → 4eda2af98

Start does get_self<implementation::VoipVideoCapture>(descriptor.VideoCapture()) with no try_as. get_self reinterprets the ABI pointer, so a VoipScreenCapture reads m_impl at the wrong offset. Reachable because VoipCall._camera holds either kind (VoipCall.cs:832) and is passed straight into the descriptor (VoipCall.cs:680) — start screen sharing before the call reaches Ready.

Fixed: the try_as chain became GetVideoCaptureImpl in VoipScreenCapture.h, used by all four call sites (both managers, Start + SetVideoCapture).
E2E: a null delegate silently transmits plaintext — VoipGroupManager.cpp:409-435 [live] → d200580d0

OnE2EEncryptDecrypt returns the input unchanged when m_encryptData is null, and hands raw ciphertext to the media stack when m_decryptData is null. Two windows: SetEncryptDecrypt runs after construction (VoipGroupCall.cs:297, :748), and SetEncryptDecrypt(null, null) runs before Stop() (VoipGroupCall.cs:787).

Fixed: returns an empty vector, which is what FrameTransformer::Transform (GroupInstanceCustomImpl.cpp:1454) treats as "drop this frame" — and what the managed EncryptData/DecryptData already return when TDLib fails (VoipGroupCall.cs:414).

The window is now closed too, see P2 below — and it was teardown-only, not construction. Every construction site installs the delegates synchronously before anything can move media, so the "delegates are set after construction" half of this finding was wrong.
persistentState out-of-bounds write — VoipManager.cpp:72-79 [latent] → fef656e87

Empty vector indexed by [i]. Nothing in C# sets PersistentState today, so it never fires — but it is heap corruption the moment someone wires it up. Also re-fetches descriptor.PersistentState() twice per byte.

Fixed with vector_to_unmanaged. Note nothing reads the state back — getPersistentState is still commented out at VoipManager.cpp:363 — so the property could just as well be deleted.
CLoopbackCapture held by value — VoipGroupManager.h:81 → c4bbb77f6

It is a ref-counted WRL RuntimeClass, and its METHODASYNCCALLBACK sub-objects forward AddRef/Release to it while MF work items hold those refs. Storage died with VoipGroupManager regardless of the refcount.

Fixed: ComPtr<CLoopbackCapture> via Make<>, created only on the screencast path. That also removed the StopCaptureAsync every non-screencast manager was making.
Loopback start failure crashed on stop — LoopbackCapture.cpp → d4f761270

Originally filed as "Stop() calls MFShutdown() process-wide". That part was wrong — MFStartup is called from InitializeLoopbackCapture (LoopbackCapture.cpp:65) and the two are paired 1:1, which is the contract MF expects.

What was real underneath it: OnStopCapture dereferenced m_AudioClient unconditionally, while StopCaptureAsync accepts the error state that a failed activation leaves behind and where no client was ever created — a null deref on exactly the machines where process loopback is unsupported. And the MFStartup was left dangling whenever the start failed, since StopCaptureAsync is the only thing that paired it; the destructor now does.

Still open, deliberately not touched: StopCaptureAsync blocks the calling thread on m_hCaptureStopped.wait() with no timeout, and Stop() runs on the UI thread. → P2.

P1 — user-visible bugs

P2 — lifetime and threading

Close the E2E delegate window — carried over from P0 → 5356ebabb

There was no window at construction. Checked every site: 297, 352 and 748 all call SetEncryptDecrypt synchronously, on the same thread, before the SetConnectionMode / EmitJoinPayload that starts media (:564, :765) — and :145, the one construction that doesn't set them, is the non-conference call, where IsConference means the e2eEncryptDecrypt callback is never wired at all (VoipGroupManager.cpp:69-84).

The real window was teardown: SetEncryptDecrypt(null, null) ran before Stop() in both paths (:787 and :1116), so tgcalls was still live with no delegate installed. Fixed by reordering — the clear only exists to break the native to managed cycle, which works just as well after the instance is gone. Not compiled; C# side, statement reorder.
DECIDED, kept as is — DecryptData ignores the data channel — VoipGroupCall.cs:421

It always passes new GroupCallDataChannelMain(), while EncryptData distinguishes screen sharing from main. Work out whether decrypting a remote screencast needs the other channel, or whether the receiving side genuinely only ever sees main.
StopCaptureAsync blocks the caller with no timeout — LoopbackCapture.cpp:230

m_hCaptureStopped.wait() was unbounded and VoipGroupManager::Stop() runs on the UI thread, so a work queue that never drained hung the app. Gone with the rewrite below: Stop now takes the lock the sample callback holds, so it waits for one packet read at most and never for a work item that may never run.
Rewrite CLoopbackCapture without WIL, C++/WinRT friendly (Fela)

Now VoipLoopbackCapture (VoipLoopbackCapture.{h,cpp}), matching how the rest of the project names things. What changed:
- WRL RuntimeClass → winrt::implements, which brings agility with it, so FtmBase is gone too. wil::com_ptr_nothrow → com_ptr, wil::unique_event → handle, wil::critical_section → slim_mutex, and the RETURN_IF_FAILED macros → plain if (auto result = …; FAILED(result)).
- The METHODASYNCCALLBACK macro and its four offsetof-derived callback objects are gone. Only the sample pump genuinely needs the MMCSS work queue, so start, stop and finish are now direct calls and there is one IMFAsyncCallback left.
- No more m_hCaptureStopped handshake: a slim_mutex held across the sample callback is what Stop synchronises against.
- The handler is a constructor argument rather than a settable sink, so it cannot be missing when the first packet lands.
Three real bugs fell out of the rewrite, all in the packet loop:
- The byte count came from GetNextPacketSize but the copy ran after GetBuffer, which is allowed to hand back a different frame count — an over-read whenever they differ.
- AUDCLNT_BUFFERFLAGS_SILENT was ignored. A silent buffer's contents are undefined, so silence was being sent as whatever happened to be in memory. Now zeroed.
- m_samples was invoked unguarded.

AUTOCONVERTPCM is passed as the periodicity argument — VoipLoopbackCapture.cpp, in OnActivated

IAudioClient::Initialize takes it in StreamFlags, but it sat in hnsPeriodicity, which the documentation says shared mode requires to be 0. Inherited verbatim from the Microsoft sample.

Measured before changing — a standalone harness activated the same process-loopback client and called Initialize three ways:

Variant	Result
AUTOCONVERTPCM as `hnsPeriodicity` (what shipped)	`S_OK`, 480-frame buffer
AUTOCONVERTPCM in `StreamFlags`, periodicity 0	`S_OK`, 480-frame buffer
no AUTOCONVERTPCM at all, periodicity 0	`S_OK`, 480-frame buffer

So shared mode ignores periodicity rather than validating it — the misplacement was harmless and screen audio has been working for real, not by luck. The control run also shows the flag does nothing here: process loopback converts to whatever format is asked for regardless. Moved into StreamFlags with periodicity 0 as a correctness tidy-up, with the measurement recorded in the comment so nobody re-derives it.

Incidental, and relevant to the clock-buffer item below: the buffer comes back as 480 frames, exactly one 10ms WebRTC quantum, despite a 20ms request.

Lock held across managed callbacks, and taken again by add/remove — VoipManager.cpp:415-458 vs :463-571; VoipGroupManager.cpp:306-435 vs :447-539

winrt::event synchronises itself, so m_lock only served to serialise callbacks against (un)subscription — at the price of a lock-order inversion with the C# side: the UI thread takes _managerLock then unsubscribes (VoipCall.cs:803-814 → wanted m_lock), while a tgcalls worker held m_lock inside managed code that takes C# locks (_stateLock, VoipCall.cs:507). No handler closed the cycle, but any handler touching _managerLock would have.

Gone from all 41 sites. VoipManager has no lock at all now; VoipGroupManager keeps one for the only state that is genuinely shared — the encrypt/decrypt delegates, written from the UI thread and read per frame from a media thread.

The bigger win is what came off that lock: the managed EncryptData blocks on a TDLib round trip (VoipGroupCall.cs:407-414), and it used to do that while holding the same mutex as every audio-level update and every subscribe. The delegates are now copied out under the lock and invoked outside it.

A comment on the event fields records why there is no lock, so it does not come back.
m_impl is not guarded — reachable in VoipGroupManager, not in VoipManager

Every if (m_impl) is TOCTOU against Stop()'s reset(). Traced properly rather than assumed, and the two managers turn out to differ:
- VoipManager is safe. Every managed call site, Dispose included, runs inside lock (_managerLock) (VoipCall.cs:365, :804). The contract holds; it just is not written down anywhere.
- VoipGroupManager is not. VoipGroupCall takes no such lock, and Dispose is reached from Update on TDLib's update thread (VoipCoordinator.cs:483 → VoipGroupCall.cs:1414), while the UI thread calls AddIncomingVideoOutput, SetRequestedVideoChannels and SetVolume unguarded. Stop() resetting m_impl against any of those is a use-after-free, not merely a torn read.
Correcting my own earlier note: guarding m_impl would not put a lock back on the path that reaches managed code. The callbacks never touch m_impl — only inbound calls do, and those go to tgcalls, not to C#. That reasoning was wrong.

Fixed the first way: VoipGroupCall now takes the _managerLock it already declared — the field existed but guarded nothing except a block of commented-out code copied from VoipCall. Held around every call that reaches the tgcalls instance, and around the teardown in Dispose and EndScreenSharing.

Deliberately left outside the lock: the IsMuted and IsNoiseSuppressionEnabled getters, which read a native field rather than touching m_impl, and the null checks that only decide whether to start something.

Two places needed more than a wrapper. The EmitJoinPayload continuations in Rejoin and RejoinScreenSharing run after awaits, so they take the lock again where they call in — C# cannot hold one across an await. And UpdateParticipant dereferenced _manager unconditionally on TDLib's update thread, which was a null reference waiting to happen quite apart from the race.

The two alternatives, if this ever proves too coarse: a dedicated native mutex around m_impl (watch the ordering — Stop() holding it while stopping the loopback would deadlock against AddExternalAudioSamples), or an atomic shared_ptr swap so a late caller keeps the instance alive instead of blocking.
No destructor on either manager

Destruction without Stop() destroyed the tgcalls instance without calling stop(), and left the loopback capture running. Both destructors call Stop(), which is idempotent.
VoipManager::Start has no re-entrancy guard — calling it twice destroyed the previous Instance without stop().

Now a no-op when one is already running, rather than a stop-and-restart: the instance in flight is serving the live call, and the second descriptor would be for the same one. Completes the Start/Stop lifecycle work the destructors began.
DECIDED, confirmed correct — requestMediaChannelDescriptions gating on IsConference — VoipGroupManager.cpp:69-83

The C# handler is subscribed unconditionally (VoipGroupCall.cs:151, :296, :351). For non-conference calls tgcalls falls back to synthesizing a description with userId = 0 (GroupInstanceCustomImpl.cpp:3453-3458). Confirm that's intended before changing anything.

P3 — performance on hot paths

OnE2EEncryptDecrypt does ~2 COM calls per byte, per packet — VoipGroupManager.cpp:409-435

The return path rebuilt the result with a push_back loop over an IIterator, on every frame of a conference call. Now one GetMany into a pre-sized buffer. The lock it all used to run under is gone too, see P2.

Still there, and not fixable from this side: the input is copied into a single_threaded_vector because the delegate takes an IVector<byte>, and the managed side then calls ToArray() on it (VoipGroupCall.cs:407) — two copies before TDLib even sees the frame. Removing them means changing the delegate signature in the IDL to pass a buffer the managed side writes into. → below.
The E2E delegate signature forces two copies per frame — VoipGroupManager.idl:15-16

Reshaped to UInt8[] in and out, since every byte ends up base64 encoded into TDLib JSON and an IVector was the wrong shape at both ends. The projection copies once each way and the managed side hands the array straight to TDLib — two COM objects, two RCWs and the ToArray() are gone. ReceiveSignalingData and the signaling callback got the same treatment; the latter stopped being an event at the same time, since it only ever had one listener, which deleted SignalingDataEmittedEventArgs outright.

It also exposed a live bug in the managed half — see the transform semaphore below.
The frame transform waited on a shared semaphore — VoipGroupCall.cs:403-432

Found while reshaping the delegates above. new Semaphore(0, 1) was shared across every invocation, but tgcalls builds one frame transformer per simulcast layer and one per incoming channel, each on its own thread — so a second Release() before the first Wait() throws SemaphoreFullException on TDLib's callback thread, and a waiter can be woken by another frame's answer and return a null result, dropping a good frame.

My own P2 change is what exposed it. Holding m_lock across the managed delegate used to serialise every encrypt and decrypt through one mutex, which accidentally made the shared semaphore safe. Taking that lock off the callback path was right, but I did not check what depended on the serialisation.

Each transform now waits on its own ManualResetEventSlim, with a 5s timeout so a stuck TDLib drops the frame rather than wedging a media thread. Not disposed on purpose — a late answer would still Set() it.
OnAudioLevelsUpdated allocates per update — VoipGroupManager.cpp:313-329

One Append COM call per participant, ~10x/s for the whole call. Now filled into a reserved std::vector and handed over once via single_threaded_vector(std::move(vec)) — which is what the /*std::move(levels)*/ comment had been asking for.

Correcting my own earlier note: there was no boxing per element. VoipGroupParticipant is an IDL struct, so the IVector holds it by value.

The remaining IVector allocation per update is fixed by the event signature.
BroadcastPartTaskImpl::done copies the payload twice — VoipGroupManager.h:187-191. Done alongside the NotReady fix in P1.
ReceiveSignalingData push_backs per byte with no reserve — VoipManager.cpp:367-379. One GetMany into a pre-sized buffer.
SetRequestedVideoChannels builds the vector even when m_impl is null — VoipGroupManager.cpp:273-302. Early-out at the top, plus a reserve. It still walks SourceGroups()/SourceIds() through COM iterators, which is fine: it runs when the video layout changes, not per frame.
Protocol()'s comparator takes std::string by value — VoipManager.h:34. Now const&. The lexicographic sort it performed was a live bug, not a latent one — see P4.
~~RemoveSsrcs iterator copy~~ — VoipGroupManager.cpp:173-179. Won't fix. GetMany needs matching element types and this converts int32_t to uint32_t, so it would trade the COM calls for a second allocation. It runs when a participant leaves, with a handful of ssrcs — not a hot path, and the current form is the clearer one.

How the loopback audio reaches WebRTC

Checked while rewriting the capture, since "we just inject bytes" sounded like the wrong shape. It is not — the screencast path is already on the intended hook:

A screencast instance never opens the microphone. createAudioDeviceModule branches on VideoContentType::Screencast and builds a FakeAudioDeviceModule over an ExternalAudioRecorder (GroupInstanceCustomImpl.cpp:4392-4397). The loopback capture is that instance's audio device.
addExternalAudioSamples converts int16 → float and appends to _externalAudioSamples (:3828), and ExternalAudioRecorder::Record (:958) pulls 480 samples — 10ms of 48kHz mono — off the front each time WebRTC asks. That is why the capture format is fixed at 1×48000×16.
The AudioCapturePostProcessor that mixes external samples into the microphone (:842-859) is the other consumer, for the main instance. We never feed it.

So there is no better hook to move to; a custom AudioDeviceModule via createAudioDeviceModule would just be reimplementing FakeAudioDeviceModule. What is worth attention is the coupling, not the mechanism:

The buffer between the two clocks is unmanaged — GroupInstanceCustomImpl.cpp:3838

WASAPI pushes on its capture clock, WebRTC pulls 10ms at a time on its own. Nothing reconciles them: the buffer grows to 2 seconds and then drops from the front, so a consumer that falls behind gets a permanent 2s lag plus a discontinuity, not a resynchronisation. It is also the reason screen audio can drift out of sync with video over a long share. Worth measuring the steady-state depth before deciding anything.
DEFERRED to an upstream pass — Screencast audio is mono-only by construction — options.num_channels = 1

Fine for speech, lossy for music or a game. Widening it means the descriptor, the capture format and ExternalAudioRecorder all have to agree, so it is a tgcalls change as much as ours.

P4 — hygiene, worth doing while we're in here

EmitJoinPayload (VoipGroupManager.cpp:151-163) invoked completion synchronously on the null-impl path and asynchronously otherwise. It no longer completes at all when there is no instance: an empty payload only got the caller as far as a join the server would reject.
const auto RegisterTag = tgcalls::Register<...>() at namespace scope in a header (VoipManager.h:22-24) — internal linkage, one registration per including TU. Moved into VoipManager.cpp, which is where a thing with internal linkage belongs.
The screencast group manager writes tgcalls_screencast.txt rather than sharing tgcalls_group.txt with the main one. Both still grow without bound; that needs a rotation policy, not a rename.
Dead #ifndef _WIN32 branch inside the designated-initializer list — VoipManager.cpp:51-57. It could never have compiled; deleted.
~~No remove counterpart to AddIncomingVideoOutput~~ — not a leak. Same mistake as the P1 video-sink item: the group VideoSinkImpl::OnFrame prunes expired weak_ptrs exactly like the 1:1 one (GroupInstanceCustomImpl.cpp:662-665), so releasing the sink is the removal. Left open only as a reminder of the shape.
DECIDED, kept as is — enableAEC/enableNS/enableAGC hard-coded true in 1:1 calls (VoipManager.cpp:46-48) while group calls honour the IsNoiseSuppressionEnabled setting. Needs your call — it is a product decision, not a defect.
Protocol() sorted versions lexicographically. Filed as latent; it was live. I had assumed tgcalls' majors were single-digit. They are not — the registered set is 2.7.7 5.0.0 7.0.0 8.0.0 9.0.0 10.0.0 11.0.0, and a string sort orders that 9 8 7 5 2.7.7 11 10, putting the two newest protocols at the end of a list the comment says the server reads newest first. Now compared component-wise as numbers, checked against the real set plus the strict-weak-ordering properties.

What is left

Every defect is fixed. Fela ruled on the open questions on 2026-08-11; what follows is what he decided and the three things still genuinely open.

Decided — accepted as they are, not defects:

The data channel that EncryptData/DecryptData discard. Native picks ScreenSharing vs Main and the managed side sends null / Main regardless.
enableAEC/enableNS/enableAGC hard-coded in 1:1 calls while group calls honour the setting.
RequestMediaChannelDescriptionTaskImpl carrying only audio.
requestMediaChannelDescriptions gated on IsConference — confirmed correct.
The 5s transform timeout stays. Encryption is near-instantaneous, which is why tgcalls made it synchronous in the first place; the timeout is there for a stuck TDLib, not a slow one.

Deferred to an upstream pass: mono-only screencast audio.

Still open:

The unmanaged buffer between the WASAPI and WebRTC clocks — worth logging the steady-state depth over a long share before designing anything.

Left deliberately, with the reasoning recorded above: m_impl being unguarded (P2), RemoveSsrcs (P3), unbounded log growth (P4).

Every item that was a defect and did not need a decision, a device or a measurement is now fixed. Two of them were only found by re-reading commits already made — the responseTimestamp gap and this version sort — so the pass over one's own work earned its keep.