Forward client audio #6439
Forward client audio #6439
Conversation
Cabbache
force-pushed
the
forward-client-microphone
branch
from
9d10730 to
df655e4
Compare
Cabbache
changed the title
app/src/microphone.c
Outdated
|
|
||
| int read_mic(void *data) { | ||
| sc_socket mic_socket = *(sc_socket*)data; | ||
| const char *input_format_name = "alsa"; |
There was a problem hiding this comment.
I guess this is only available on Linux? Can SDL be used instead?
There was a problem hiding this comment.
@yume-chan it should work on other platforms if you change alsa to something appropriate? We might want input_format_name to be passed as an argument.
It's possible to list the available input devices with
const AVInputFormat *fmt = NULL;
fmt = av_input_audio_device_next(NULL);
while (fmt) {
printf("%s: %s\n",
fmt->name,
fmt->long_name ? fmt->long_name : "(no description)"
);
fmt = av_input_audio_device_next(fmt);
}This is what I see on linux:
alsa: ALSA audio input
jack: JACK Audio Connection Kit
lavfi: Libavfilter virtual input device
oss: OSS (Open Sound System) capture
pulse: Pulse audio input
|
Looking forward to this feature, would be really good in phone call experience while working for instance. |
Cabbache
force-pushed
the
forward-client-microphone
branch
2 times, most recently
from
7b052b2 to
8e1da04
Compare
|
It is working and somewhat stable on high latency. Tested from a laptop running linux and an android 16 device. The new arguments are like this: This is not enabled by default so you have to explicitly add the argument, e.g |
Cabbache
force-pushed
the
forward-client-microphone
branch
from
8e1da04 to
f565df6
Compare
|
@Cabbache i am getting these errors the moment i answer a phone call. Though i can use any recorder app on android to record using pc mic and it also does not work with |
|
@JunaidQrysh Hmm I haven't tested this thoroughly, let me see if I can replicate it. |
Cabbache
force-pushed
the
forward-client-microphone
branch
from
de939a0 to
fe310e2
Compare
|
@JunaidQrysh I also see similar errors, but the audio still worked. edit: I've tested only with |
|
@Cabbache the audio and client mic should work in a voice call. I get those errors when i make a voice call using |
|
@Cabbache i have found that |
Changes SummaryThis PR implements client-side microphone audio forwarding to Android devices, allowing users to inject audio from their computer's microphone (or audio files) into the Android device. The feature includes a new audio decoder (Opus format), audio injector using Android's AudioPolicy APIs, command-line options for audio source selection, and a utility to list available audio sources. Type: feature Components Affected: Client-side audio capture (C), Server-side audio injection (Java), Command-line interface, Socket management for microphone stream, Audio decoding pipeline, Options/configuration system Files Changed
Architecture Impact
Risk Areas: Android AudioPolicy API use via reflection: Fragile across Android versions, relies on private APIs that may change or be restricted on newer Android versions, Audio format/codec assumptions: Hardcoded Opus format and stereo 48kHz configuration may not work universally; limited flexibility for different audio formats, Buffer management: PipedInputStream/PipedOutputStream with 500KB buffer - potential blocking/deadlock if producer/consumer rates mismatch, Error handling in audio decoder: Limited error recovery in AudioDecoder.start() and audio injection pipeline - exceptions logged but may silently fail, Platform-specific audio capture: Uses different FFmpeg input formats (WASAPI/dshow on Windows, avfoundation on macOS, ALSA on Linux) - testing burden across platforms, Memory/resource lifecycle: Multiple concurrent streams (video, audio, microphone) - potential for resource exhaustion with long-running sessions, Socket connection ordering: Complex socket management logic in server.c for client_audio_socket - subtle bugs possible in connection establishment or cleanup Suggestions
Full review in progress... | Powered by diffray |
| try { | ||
| decoder.stop(); | ||
| decoder.release(); | ||
| } catch (Exception e) {} |
There was a problem hiding this comment.
🟠 HIGH - Empty catch block without exception logging
Agent: spring
Category: bug
Description:
The catch block at line 130 is empty - it catches Exception but does nothing with it. This silently swallows critical decoder cleanup errors without any diagnostic information.
Suggestion:
Log the exception using Ln.e() before the cleanup logic: catch (Exception e) { Ln.e("Error stopping decoder", e); }
Why this matters: Swallowed errors hide bugs and make debugging difficult.
Confidence: 85%
Rule: java_avoid_empty_catch_blocks
Review ID: 7dceb3ee-2843-4df2-91e1-e1a6f1855ffd
Rate it 👍 or 👎 to improve future reviews | Powered by diffray
| ByteBuffer outputBuffer = decoder.getOutputBuffer(outputIndex); | ||
| if (bufferInfo.size > 0) { | ||
| byte[] pcmData = new byte[bufferInfo.size]; | ||
| outputBuffer.position(bufferInfo.offset); | ||
| outputBuffer.limit(bufferInfo.offset + bufferInfo.size); | ||
| outputBuffer.get(pcmData); | ||
| pcmOutput.write(pcmData); | ||
| pcmOutput.flush(); | ||
| } |
There was a problem hiding this comment.
🟠 HIGH - Potential null dereference of outputBuffer
Agent: bugs
Category: bug
Description:
decoder.getOutputBuffer(outputIndex) can return null. The code uses outputBuffer without null checking, even though size is checked.
Suggestion:
Add a null check: if (outputBuffer == null) { decoder.releaseOutputBuffer(...); continue; } before using outputBuffer.
Why this matters: NullPointerException is the most common runtime exception in Java.
Confidence: 75%
Rule: bug_null_pointer_java
Review ID: 7dceb3ee-2843-4df2-91e1-e1a6f1855ffd
Rate it 👍 or 👎 to improve future reviews | Powered by diffray
| return audioFd; | ||
| } |
There was a problem hiding this comment.
🟠 HIGH - Potential null dereference in getFirstSocket
Agent: bugs
Category: bug
Description:
getFirstSocket() result is immediately used to get a FileDescriptor without null checking. If getFirstSocket() returns null, calling getFileDescriptor() on null will cause a NullPointerException.
Suggestion:
Check if the result is not null: LocalSocket socket = getFirstSocket(); if (socket == null) throw new IOException("No sockets available"); FileDescriptor fd = socket.getFileDescriptor();
Why this matters: NullPointerException is the most common runtime exception in Java.
Confidence: 80%
Rule: bug_null_pointer_java
Review ID: 7dceb3ee-2843-4df2-91e1-e1a6f1855ffd
Rate it 👍 or 👎 to improve future reviews | Powered by diffray
| ByteBuffer inputBuffer = decoder.getInputBuffer(inputIndex); | ||
| inputBuffer.clear(); | ||
| inputBuffer.put(opusData); |
There was a problem hiding this comment.
🟠 HIGH - Potential null dereference of inputBuffer
Agent: bugs
Category: bug
Description:
decoder.getInputBuffer(inputIndex) can return null, but the code calls inputBuffer.clear() and inputBuffer.put() without null checking first.
Suggestion:
Add a null check: if (inputBuffer == null) { continue; } before calling methods on inputBuffer.
Why this matters: NullPointerException is the most common runtime exception in Java.
Confidence: 75%
Rule: bug_null_pointer_java
Review ID: 7dceb3ee-2843-4df2-91e1-e1a6f1855ffd
Rate it 👍 or 👎 to improve future reviews | Powered by diffray
| new Thread(() -> { | ||
| byte[] audioBuffer = new byte[4096]; | ||
| while (true) { | ||
| try { | ||
| int bytesRead = pis.read(audioBuffer); | ||
| if (bytesRead <= 0) { | ||
| break; | ||
| } | ||
| audioTrack.write(audioBuffer, 0, bytesRead); | ||
| } catch (Exception e) { | ||
| Ln.e("Audio injection error", e); | ||
| break; | ||
| } | ||
| } | ||
| }, "client-audio-injector").start(); |
There was a problem hiding this comment.
🟡 MEDIUM - PipedInputStream not closed in new thread
Agent: bugs
Category: bug
Description:
The PipedInputStream 'pis' passed to the audio injector thread is never explicitly closed. The thread reads from it in an infinite loop, but there's no guarantee it will be closed if the thread exits.
Suggestion:
Add a try-finally block to close pis after the while loop exits, or wrap the read loop in try-with-resources if the stream should be owned by this thread.
Why this matters: Memory leaks degrade performance and can crash the application.
Confidence: 70%
Rule: java_resource_leak
Review ID: 7dceb3ee-2843-4df2-91e1-e1a6f1855ffd
Rate it 👍 or 👎 to improve future reviews | Powered by diffray
| return clientAudioSocket; | ||
| } | ||
|
|
||
| public void shutdown() throws IOException { |
There was a problem hiding this comment.
🟡 MEDIUM - Incomplete shutdown() for clientAudioSocket
Agent: refactoring
Category: bug
Description:
The shutdown() method shuts down video, audio, and control sockets but NOT clientAudioSocket. Additionally, close() method (lines 167-177) also omits clientAudioSocket. This leaves the socket in an inconsistent state.
Suggestion:
Add shutdown for clientAudioSocket: if (clientAudioSocket != null) { clientAudioSocket.shutdownInput(); clientAudioSocket.shutdownOutput(); } and also add clientAudioSocket.close() in the close() method.
Confidence: 95%
Rule: quality_law_of_demeter
Review ID: 7dceb3ee-2843-4df2-91e1-e1a6f1855ffd
Rate it 👍 or 👎 to improve future reviews | Powered by diffray
| return clientAudioSocket; | ||
| } | ||
|
|
||
| public void shutdown() throws IOException { |
There was a problem hiding this comment.
🟠 HIGH - Missing error handling for socket shutdown operations
Agent: spring
Category: bug
Description:
The shutdown() method calls shutdownInput() and shutdownOutput() on sockets without try-catch. These throw IOException which propagates and may leave sockets in inconsistent state.
Suggestion:
Either wrap each socket shutdown in individual try-catch to ensure all sockets are shutdown even if one fails, or document that caller must handle partial cleanup.
Confidence: 75%
Rule: java_add_try_catch_blocks_for_external_servic
Review ID: 7dceb3ee-2843-4df2-91e1-e1a6f1855ffd
Rate it 👍 or 👎 to improve future reviews | Powered by diffray
| private LocalSocket getFirstSocket() { | ||
| if (videoSocket != null) { | ||
| return videoSocket; | ||
| } | ||
| if (audioSocket != null) { | ||
| return audioSocket; | ||
| } | ||
| return controlSocket; | ||
| if (controlSocket != null) { | ||
| return controlSocket; | ||
| } | ||
| return clientAudioSocket; | ||
| } |
There was a problem hiding this comment.
🟡 MEDIUM - Missing null check in getFirstSocket()
Agent: refactoring
Category: quality
Description:
The getFirstSocket() method returns clientAudioSocket without null check if all other sockets are null. Line 187 calls getFirstSocket().getFileDescriptor() which would NPE if all sockets are null.
Suggestion:
Either throw IllegalStateException if all sockets are null, or add defensive null check in sendDeviceMeta(). Document the contract clearly.
Confidence: 70%
Rule: quality_law_of_demeter
Review ID: 7dceb3ee-2843-4df2-91e1-e1a6f1855ffd
Rate it 👍 or 👎 to improve future reviews | Powered by diffray
| int packetSize = ((sizeBuffer[0] & 0xFF) << 24) | | ||
| ((sizeBuffer[1] & 0xFF) << 16) | | ||
| ((sizeBuffer[2] & 0xFF) << 8) | | ||
| (sizeBuffer[3] & 0xFF); | ||
|
|
||
| if (packetSize > 0 && packetSize <= 100000) { | ||
| byte[] opusData = new byte[packetSize]; | ||
| int actualRead = bis.read(opusData); | ||
| if (actualRead == packetSize) { | ||
| ByteBuffer inputBuffer = decoder.getInputBuffer(inputIndex); | ||
| inputBuffer.clear(); | ||
| inputBuffer.put(opusData); | ||
| decoder.queueInputBuffer(inputIndex, 0, packetSize, presentationTime, 0); | ||
| // Increment presentation time (20ms per frame at 48kHz = 960 samples) | ||
| presentationTime += 20000; // microseconds | ||
| } | ||
| } |
There was a problem hiding this comment.
🟡 MEDIUM - Large memory allocation from untrusted network data
Agent: security
Category: security
Description:
Lines 81-84 parse 32-bit big-endian integer from network. Limit of 100KB (line 86) is reasonable but allocates new byte array per packet without pooling, which could cause memory pressure under high load.
Suggestion:
Consider using a reusable buffer pool for common packet sizes, or add rate limiting for allocation frequency.
Confidence: 60%
Rule: c_buffer_overflow
Review ID: 7dceb3ee-2843-4df2-91e1-e1a6f1855ffd
Rate it 👍 or 👎 to improve future reviews | Powered by diffray
| //private final FileDescriptor micFd; | ||
|
|
||
| private DesktopConnection(LocalSocket videoSocket, LocalSocket audioSocket, LocalSocket controlSocket, LocalSocket clientAudioSocket) throws IOException { | ||
| this.videoSocket = videoSocket; | ||
| this.audioSocket = audioSocket; | ||
| this.controlSocket = controlSocket; | ||
| this.clientAudioSocket = clientAudioSocket; | ||
|
|
||
| videoFd = videoSocket != null ? videoSocket.getFileDescriptor() : null; | ||
| audioFd = audioSocket != null ? audioSocket.getFileDescriptor() : null; | ||
| controlChannel = controlSocket != null ? new ControlChannel(controlSocket) : null; | ||
| //micFd = clientAudioSocket != null ? clientAudioSocket.getFileDescriptor() : null; |
There was a problem hiding this comment.
🔵 LOW - Commented-out code for microphone socket
Agent: refactoring
Category: quality
Description:
Lines 33 and 44 have commented-out code for micFd. This indicates incomplete implementation that should be cleaned up or completed.
Suggestion:
Remove commented-out code entirely, or add TODO comment explaining the planned implementation.
Confidence: 80%
Rule: quality_law_of_demeter
Review ID: 7dceb3ee-2843-4df2-91e1-e1a6f1855ffd
Rate it 👍 or 👎 to improve future reviews | Powered by diffray
Review Summary
Validated 40 issues: 22 kept, 18 filtered Issues Found: 22💬 See 21 individual line comment(s) for details. 📊 8 unique issue type(s) across 22 location(s) 📋 Full issue list (click to expand)🟠 HIGH - Array bounds violation on audio stream access (2 occurrences)Agent: security Category: security Why this matters: Buffer overflows allow arbitrary code execution. 📍 View all locations
Rule: 🟠 HIGH - Potential null dereference of outputBuffer (3 occurrences)Agent: bugs Category: bug Why this matters: NullPointerException is the most common runtime exception in Java. 📍 View all locations
Rule: 🟠 HIGH - Empty catch block without exception loggingAgent: spring Category: bug Why this matters: Swallowed errors hide bugs and make debugging difficult. File: Description: The catch block at line 130 is empty - it catches Exception but does nothing with it. This silently swallows critical decoder cleanup errors without any diagnostic information. Suggestion: Log the exception using Ln.e() before the cleanup logic: catch (Exception e) { Ln.e("Error stopping decoder", e); } Confidence: 85% Rule: 🟡 MEDIUM - PipedInputStream not closed in new thread (2 occurrences)Agent: bugs Category: bug Why this matters: Memory leaks degrade performance and can crash the application. 📍 View all locations
Rule: 🟡 MEDIUM - Timing control logic embedded in encoding loopAgent: architecture Category: quality Why this matters: Multi-purpose code is hard to test, modify, and reuse. File: Description: Lines 350-365 embed timing control (start_time tracking, frame duration calculation, sleep logic) directly in the transmission section. This mixes playback timing with encoding responsibility. Suggestion: Extract timing control into a separate function or helper. Let the main loop call a timing function to wait until the next frame. Confidence: 65% Rule: 🟡 MEDIUM - Overly broad exception handling in client audio setup (2 occurrences)Agent: spring Category: quality 📍 View all locations
Rule: 🟡 MEDIUM - Excessive nesting depth (4 levels) in main audio loop (6 occurrences)Agent: refactoring Category: quality 📍 View all locations
Rule: 🔵 LOW - Empty catch block for setTargetMixRole - intentional (5 occurrences)Agent: bugs Category: bug Why this matters: Swallowed errors hide bugs and make debugging difficult. 📍 View all locations
Rule: ℹ️ 1 issue(s) outside PR diff (click to expand)
🟠 HIGH - Missing break statement in switch caseAgent: bugs Category: bug File: Description: The case "audio_encoder" is missing a break statement, causing fall-through to the next case "power_off_on_close". Setting audio_encoder will incorrectly also set powerOffScreenOnClose. Suggestion: Add 'break;' after line 424 to prevent fall-through to the next case. Confidence: 98% Rule: Review ID: |
|
Why is this thread spammed with a bot review? Nobody asked for it AFAIK. |
|
Weird, I didn't ask for it either. |
Cabbache
force-pushed
the
forward-client-microphone
branch
from
fe310e2 to
28113de
Compare
|
Well, I don't really like these tools, but I must admit it actually found a real bug outside this PR, see #6439 (comment), at the bottom: "1 issue(s) outside PR diff (click to expand)". Commit f9960e9 is the culprit. Any static analyzer should give a warning, though, AI is not needed for that AFAIK. |
Refs #6439 comment <#6439 (comment)>
|
Thank you, fixed: cda4387 |
AI tools can be great for reviews, but I don't want them to be intrusive. I don't want an unsolicited bot posting a ton of AI-generated review comments. What could be useful is a tool or website where I can push a branch or a PR and get an explanation of possible issues (like a powerful static analyzer), without adding noise to the PR thread. |
|
@JunaidQrysh I can confirm that In reality, |
|
Hi Cabbache, I try to use scrcpy into an active phone call using the the main Samsung phone app which is: but doesn't work. ( I tried with android 14 15 16) into partial dumpsys audio I see this difference: active phone call, scrcpy plays mp3 on phone A and we do not hear the mp3 on the B phone side neither A side Other state:
|
Cabbache
force-pushed
the
forward-client-microphone
branch
from
b11bf8b to
cdc664b
Compare
|
Thanks for the report @pierl2000. This looks like a vendor specific issue related to Qualcomm PAL ( Can you test these changes? https://github.com/Cabbache/scrcpy/tree/qualcomm-pal-test |
|
Hi Cabbache, adb shell -n logcat | grep -ai 'audio|sound|snd|scrcpy|voice|mix' 01-06 19:52:07.805 1767 2003 I SDM : HWDeviceDRM::UpdateMixerAttributes: Mixer WxH 1080x2340-1 for Peripheral |
|
Hi @Cabbache I tried this code on "Device: [Xiaomi] Redmi 24116RACCG (Android 14)" (Redmi note 14 pro 4g) and it worked! I needed to re-run it after the call started but that's fine. Also the sound gets echoed in a disturbing way. Thanks! |
|
Thanks for the feedback @trombettafrank. A bit unrelated but I got my hands on |
Cabbache
force-pushed
the
forward-client-microphone
branch
2 times, most recently
from
cdc664b to
8b8582c
Compare
|
Tried on Mac x Android 16 (after commenting out HAVE_V4L2 conditional statements), getting these errors: Not sure if this information is of any help but I'd love to provide more feedback/output if you'd like me to test it further. |
|
@almog can you share the output of |
Hey @Cabbache, I forgot to mention that I tried that too and it failed: Running ffmpeg directly (as suggested in the previous output) produces a this list of devices: |
|
@almog did you try I havent tested with avfoundation. |
@Cabbache Yes, but now I noticed that you prefixed the zero with colon (
No error here but the device doesn't seem to register any audio input either.
Notice that in case 3 the error is different error than for the zero device (no colon). Not sure if any of the information I provide here is helpful in any way. |
Cabbache
force-pushed
the
forward-client-microphone
branch
from
8b8582c to
50734c9
Compare
|
I was able to get a Mac to test with and replicated some of the issues @almog reported. I got it to work with a Google Pixel device. |
|
Thank you @Cabbache! I can confirm it works now on my Mac, only issues I see right now (that were mentioned by others) are:
|
10 participants
This is a WIP based on the poc by @yume-chan to add the feature requested in #3880