Support audio isolation for virtual displays #6561
Conversation
Changes SummaryThis PR implements audio isolation for apps running on virtual displays via the Type: feature|mixed Components Affected: Audio subsystem (AudioPlaybackCapture, AudioDirectCapture), Virtual display capture (NewDisplayCapture, SurfaceCapture), CLI argument parsing (cli.c, options.h), Server-client communication (server.c, Server.java), System service wrappers (ActivityManager, PackageManager, ServiceManager), Event injection (Controller.java) Files Changed
Architecture Impact
Risk Areas: CRITICAL: screen.c file corruption - contains Java code instead of C code, will cause build failure, Reflection-heavy implementation in AudioPlaybackCapture and ActivityManager - relies on private Android APIs that may change across versions, Background AppMonitor thread creation without documented lifecycle guarantees - could lead to race conditions if not carefully managed, Audio UID switching logic may cause audio interruptions during app transitions, PackageManager wrapper uses reflection to access IPackageManager - potential compatibility issues across Android versions, AppMonitor thread does not have exception handling around System.currentTimeMillis() or Thread.sleep() edge cases, Interaction between audioIgnoreApps and audioFilterApps filtering logic could be unclear to users Suggestions
Full review in progress... | Powered by diffray |
| } catch (InterruptedException e) { | ||
| // ignore | ||
| } |
There was a problem hiding this comment.
🟡 MEDIUM - InterruptedException caught without restoring interrupt flag
Agent: spring
Category: quality
Description:
InterruptedException is caught but only contains a comment '// ignore'. The interrupt flag is not restored, which can cause loss of interruption signals.
Suggestion:
Call Thread.currentThread().interrupt() to restore the interrupt flag, or document explicitly why ignoring is safe here (e.g., short sleep for timing purposes)
Why this matters: Unhandled errors cause failures.
Confidence: 72%
Rule: java_do_not_ignore_caught_exceptions
Review ID: 4fa8a08a-4c88-4c8e-acb1-aaf6ba9eb3b8
Rate it 👍 or 👎 to improve future reviews | Powered by diffray
| if (r == 0) { | ||
| try { | ||
| Thread.sleep(10); | ||
| } catch (InterruptedException e) { | ||
| return -1; | ||
| } | ||
| return read(outDirectBuffer, outBufferInfo); |
There was a problem hiding this comment.
🟠 HIGH - Potential unbounded recursion in read() method
Agent: bugs
Category: bug
Description:
When reader.read() returns 0, the method recurses after a 10ms sleep. If the reader consistently returns 0, this will cause StackOverflowError.
Suggestion:
Replace recursive call with a while loop with a maximum retry count, or use tail recursion optimization if available
Why this matters: Missing base case causes infinite recursion and StackOverflowError.
Confidence: 78%
Rule: java_recursion_no_base_case
Review ID: 4fa8a08a-4c88-4c8e-acb1-aaf6ba9eb3b8
Rate it 👍 or 👎 to improve future reviews | Powered by diffray
| while (monitorRunning) { | ||
| try { | ||
| // Increase limit to 50 to ensure we find tasks even if many background tasks exist | ||
| java.util.List<android.app.ActivityManager.RunningTaskInfo> tasks = am.getRunningTasks(50); | ||
| boolean foundTopApp = false; | ||
| if (tasks != null) { | ||
| for (android.app.ActivityManager.RunningTaskInfo task : tasks) { | ||
| int taskDisplayId = getTaskDisplayId(task); | ||
| String paramPkg = task.topActivity != null ? task.topActivity.getPackageName() : "null"; | ||
|
|
||
| if (audioFilterApps != null && audioFilterApps.contains(paramPkg)) { | ||
| continue; | ||
| } | ||
|
|
||
| if (taskDisplayId == displayId && task.topActivity != null) { | ||
| if (!foundTopApp) { | ||
| // This is the FIRST task we found for this display -> It is the Top App | ||
| foundTopApp = true; | ||
| String packageName = paramPkg; | ||
| if (!packageName.equals(lastPackageName)) { | ||
| Ln.i("App changed on display " + displayId + ": " + packageName); | ||
| lastPackageName = packageName; | ||
|
|
||
| int newUid = -1; | ||
| if (audioIgnoreApps != null && audioIgnoreApps.contains(packageName)) { | ||
| Ln.i("App ignored (forcing global audio): " + packageName); | ||
| newUid = -1; | ||
| } else { | ||
| // Resolve UID | ||
| try { | ||
| android.content.pm.ApplicationInfo info = pm.getApplicationInfo(packageName, 0, 0); | ||
| if (info != null) { | ||
| newUid = info.uid; | ||
| } | ||
| } catch (Exception e) { | ||
| Ln.e("Could not get UID for " + packageName, e); | ||
| } | ||
| } | ||
|
|
||
| if (newUid != lastUid) { | ||
| Ln.i("Updating audio capture target UID: " + newUid + " for package: " + packageName); | ||
| audioCapture.setTargetUid(newUid); | ||
| lastUid = newUid; | ||
| } | ||
| } | ||
| } | ||
| break; | ||
| } | ||
| } | ||
|
|
||
| } | ||
|
|
||
| if (!foundTopApp) { | ||
| if (!"SYSTEM".equals(lastPackageName)) { | ||
| Ln.i("No app detected on display " + displayId + ", switching to Global Audio"); | ||
| lastPackageName = "SYSTEM"; | ||
| if (lastUid != -1) { | ||
| audioCapture.setTargetUid(-1); | ||
| lastUid = -1; | ||
| } | ||
| } | ||
| } | ||
| Thread.sleep(500); | ||
| } catch (InterruptedException e) { | ||
| break; | ||
| } catch (Exception e) { | ||
| Ln.e("App monitor error", e); | ||
| try { | ||
| Thread.sleep(1000); | ||
| } catch (InterruptedException ignored) { | ||
| // ignored | ||
| } | ||
| } | ||
| } | ||
| }, "AppMonitor"); | ||
| appMonitorThread.start(); | ||
| } | ||
|
|
||
| @android.annotation.SuppressLint("BlockedPrivateApi") | ||
| private static int getTaskDisplayId(android.app.ActivityManager.RunningTaskInfo task) { | ||
| try { | ||
| Class<?> taskInfoClass = Class.forName("android.app.TaskInfo"); | ||
| java.lang.reflect.Field displayIdField = taskInfoClass.getDeclaredField("displayId"); | ||
| displayIdField.setAccessible(true); | ||
| return displayIdField.getInt(task); | ||
| } catch (Exception e) { | ||
| Ln.w("Could not get displayId from task", e); | ||
| return -1; | ||
| } |
There was a problem hiding this comment.
🟠 HIGH - Inefficient reflection calls in polling loop
Agent: performance
Category: performance
Description:
The appMonitor thread performs reflection lookups (Class.forName, getDeclaredField, setAccessible) on every poll iteration for each task. The getTaskDisplayId() method at lines 278-287 does full reflection on every call instead of caching the Field reference.
Suggestion:
Cache the reflected Field reference (displayIdField) as a static class field at class initialization, then only call getInt() on each iteration. Example: private static final Field DISPLAY_ID_FIELD = initDisplayIdField();
Why this matters: N+1 queries cause severe performance degradation.
Confidence: 90%
Rule: perf_n_plus_one_queries
Review ID: 4fa8a08a-4c88-4c8e-acb1-aaf6ba9eb3b8
Rate it 👍 or 👎 to improve future reviews | Powered by diffray
| if (audioFilterApps != null && audioFilterApps.contains(paramPkg)) { | ||
| continue; | ||
| } | ||
|
|
||
| if (taskDisplayId == displayId && task.topActivity != null) { | ||
| if (!foundTopApp) { | ||
| // This is the FIRST task we found for this display -> It is the Top App | ||
| foundTopApp = true; | ||
| String packageName = paramPkg; | ||
| if (!packageName.equals(lastPackageName)) { | ||
| Ln.i("App changed on display " + displayId + ": " + packageName); | ||
| lastPackageName = packageName; | ||
|
|
||
| int newUid = -1; | ||
| if (audioIgnoreApps != null && audioIgnoreApps.contains(packageName)) { |
There was a problem hiding this comment.
🟡 MEDIUM - List.contains() in polling loop creates O(N*M) complexity
Agent: performance
Category: performance
Description:
audioFilterApps and audioIgnoreApps are declared as List (lines 62-63), causing O(N) lookup per contains() call. With up to 50 tasks checked every 500ms, this creates unnecessary overhead.
Suggestion:
Convert audioFilterApps and audioIgnoreApps from List to Set for O(1) lookup. Modify the constructor and Options class accordingly.
Why this matters: Poor performance degrades user experience.
Confidence: 68%
Rule: perf_quadratic_loops
Review ID: 4fa8a08a-4c88-4c8e-acb1-aaf6ba9eb3b8
Rate it 👍 or 👎 to improve future reviews | Powered by diffray
| try { | ||
| AudioAttributes attributes = new AudioAttributes.Builder().setUsage(usage).build(); | ||
| addMixRuleMethod.invoke(audioMixingRuleBuilder, ruleMatchAttributeUsageConstant, attributes); | ||
| } catch (Exception e) { | ||
| // Ignore invalid usages if any | ||
| } |
There was a problem hiding this comment.
🟡 MEDIUM - Empty catch block with only comment
Agent: quality
Category: quality
Description:
Catch block for Exception in loop (lines 80-87) silently ignores failures when adding audio usage rules. While intentional, lack of logging makes debugging difficult.
Suggestion:
Add debug logging: Ln.d("Skipping usage " + usage + ": " + e.getClass().getSimpleName());
Why this matters: Swallowed errors hide bugs and make debugging difficult.
Confidence: 65%
Rule: java_exception_ignored
Review ID: 4fa8a08a-4c88-4c8e-acb1-aaf6ba9eb3b8
Rate it 👍 or 👎 to improve future reviews | Powered by diffray
| // IContentProvider provider = providerHolder.provider; | ||
| Field providerField = providerHolder.getClass().getDeclaredField("provider"); | ||
| providerField.setAccessible(true); | ||
| return (IContentProvider) providerField.get(providerHolder); |
There was a problem hiding this comment.
🟡 MEDIUM - Reflection field.get() result cast without null check
Agent: security
Category: quality
Description:
providerField.get(providerHolder) is cast to IContentProvider without null checking. Field.get() can return null if the field value is null.
Suggestion:
Add null check before casting: Object provider = providerField.get(providerHolder); return provider != null ? (IContentProvider) provider : null;
Confidence: 65%
Rule: java_validate_parameter_types_before_casting
Review ID: 4fa8a08a-4c88-4c8e-acb1-aaf6ba9eb3b8
Rate it 👍 or 👎 to improve future reviews | Powered by diffray
| // IContentProvider provider = providerHolder.provider; | ||
| Field providerField = providerHolder.getClass().getDeclaredField("provider"); | ||
| providerField.setAccessible(true); | ||
| return (IContentProvider) providerField.get(providerHolder); |
There was a problem hiding this comment.
🟡 MEDIUM - Catch block swallows exception without propagation or error indication
Agent: spring
Category: quality
Description:
ReflectiveOperationException is caught and logged but then silently ignored with a void method. This prevents callers from knowing if the operation failed.
Suggestion:
Either throw a runtime exception to propagate the error, or return a boolean indicating success/failure instead of silently ignoring.
Confidence: 62%
Rule: java_do_not_ignore_caught_exceptions
Review ID: 4fa8a08a-4c88-4c8e-acb1-aaf6ba9eb3b8
Rate it 👍 or 👎 to improve future reviews | Powered by diffray
| public void forceStopPackage(String packageName) { | ||
| try { | ||
| Method method = getForceStopPackageMethod(); | ||
| method.invoke(manager, packageName, /* userId */ /* UserHandle.USER_CURRENT */ -2); | ||
| method.invoke(manager, packageName, -2); |
There was a problem hiding this comment.
🟡 MEDIUM - Catch block swallows exception and returns ambiguous status code
Agent: spring
Category: quality
Description:
Throwable is caught and logged, then 0 is returned. The caller cannot distinguish between a successful activity start and failure.
Suggestion:
Return a distinct error value (e.g., -1) or throw an exception to clearly indicate failure state.
Confidence: 65%
Rule: java_do_not_ignore_caught_exceptions
Review ID: 4fa8a08a-4c88-4c8e-acb1-aaf6ba9eb3b8
Rate it 👍 or 👎 to improve future reviews | Powered by diffray
| } catch (Throwable e) { | ||
| Ln.e("Could not invoke method", e); | ||
| } |
There was a problem hiding this comment.
🟡 MEDIUM - Catch block swallows exception without error indication to caller
Agent: spring
Category: quality
Description:
Throwable is caught and logged but the method returns void without any indication of failure to the caller.
Suggestion:
Return a boolean value indicating success/failure, or throw a runtime exception to notify callers of the failure.
Confidence: 62%
Rule: java_do_not_ignore_caught_exceptions
Review ID: 4fa8a08a-4c88-4c8e-acb1-aaf6ba9eb3b8
Rate it 👍 or 👎 to improve future reviews | Powered by diffray
| } catch (Throwable e) { | ||
| Ln.e("Could not invoke getTasks", e); | ||
| return null; | ||
| } |
There was a problem hiding this comment.
🟡 MEDIUM - Catch block silently returns null on error without clear failure indication
Agent: spring
Category: quality
Description:
Throwable is caught and logged but null is returned, making it impossible for callers to distinguish failure from a legitimate empty result.
Suggestion:
Return Optional or throw an exception to distinguish error state from empty result.
Confidence: 60%
Rule: java_do_not_ignore_caught_exceptions
Review ID: 4fa8a08a-4c88-4c8e-acb1-aaf6ba9eb3b8
Rate it 👍 or 👎 to improve future reviews | Powered by diffray
Review Summary
Validated 28 issues: 17 kept, 11 filtered Issues Found: 17💬 See 12 individual line comment(s) for details. 📊 7 unique issue type(s) across 17 location(s) 📋 Full issue list (click to expand)🟠 HIGH - Potential NullPointerException on displayInfoAgent: bugs Category: bug Why this matters: NullPointerException is the most common runtime exception in Java. File: Description: getDisplayInfo() result is used without null check at line 126-129, but the same method call at lines 95-101 has a null check. Suggestion: Add null check after line 126: if (displayInfo == null) { /* handle error or use defaults */ } before accessing displayInfo methods Confidence: 88% Rule: 🟠 HIGH - Inefficient reflection calls in polling loopAgent: performance Category: performance Why this matters: N+1 queries cause severe performance degradation. File: Description: The appMonitor thread performs reflection lookups (Class.forName, getDeclaredField, setAccessible) on every poll iteration for each task. The getTaskDisplayId() method at lines 278-287 does full reflection on every call instead of caching the Field reference. Suggestion: Cache the reflected Field reference (displayIdField) as a static class field at class initialization, then only call getInt() on each iteration. Example: private static final Field DISPLAY_ID_FIELD = initDisplayIdField(); Confidence: 90% Rule: 🟠 HIGH - Potential unbounded recursion in read() methodAgent: bugs Category: bug Why this matters: Missing base case causes infinite recursion and StackOverflowError. File: Description: When reader.read() returns 0, the method recurses after a 10ms sleep. If the reader consistently returns 0, this will cause StackOverflowError. Suggestion: Replace recursive call with a while loop with a maximum retry count, or use tail recursion optimization if available Confidence: 78% Rule: 🟡 MEDIUM - InterruptedException caught without restoring interrupt flag (8 occurrences)Agent: spring Category: quality Why this matters: Unhandled errors cause failures. 📍 View all locations
Rule: 🟡 MEDIUM - List.contains() in polling loop creates O(N*M) complexityAgent: performance Category: performance Why this matters: Poor performance degrades user experience. File: Description: audioFilterApps and audioIgnoreApps are declared as List (lines 62-63), causing O(N) lookup per contains() call. With up to 50 tasks checked every 500ms, this creates unnecessary overhead. Suggestion: Convert audioFilterApps and audioIgnoreApps from List to Set for O(1) lookup. Modify the constructor and Options class accordingly. Confidence: 68% Rule: 🟡 MEDIUM - Empty catch block with only comment (2 occurrences)Agent: quality Category: quality Why this matters: Swallowed errors hide bugs and make debugging difficult. 📍 View all locations
Rule: 🟡 MEDIUM - Reflection invoke results cast without null check (3 occurrences)Agent: security Category: quality 📍 View all locations
Rule: ℹ️ 5 issue(s) outside PR diff (click to expand)
🟠 HIGH - Potential NullPointerException on displayInfoAgent: bugs Category: bug Why this matters: NullPointerException is the most common runtime exception in Java. File: Description: getDisplayInfo() result is used without null check at line 126-129, but the same method call at lines 95-101 has a null check. Suggestion: Add null check after line 126: if (displayInfo == null) { /* handle error or use defaults */ } before accessing displayInfo methods Confidence: 88% Rule: 🟡 MEDIUM - Reflection invoke result cast without null validationAgent: security Category: quality File: Description: getDefaultMethod.invoke(null) result is cast to IInterface without null checking. If the underlying method returns null, this would cause issues. Suggestion: Add null check: Object result = getDefaultMethod.invoke(null); if (result == null) throw new AssertionError("getDefault() returned null"); IInterface am = (IInterface) result; Confidence: 60% Rule: 🔵 LOW - InterruptedException caught without restoring interrupt flag (3 occurrences)Agent: spring Category: quality Why this matters: Unhandled errors cause failures. 📍 View all locations
Rule: Review ID: |
2 participants
This PR implements audio isolation for apps running on a virtual display (
--new-display).When using
--audio-source=playbackflag, AppMonitor now detects the foreground app on the virtual display and restricts audio capture to its UID, isolating it from other device audio.New CLI options for compatibility:
--audio-ignore-apps: Force global audio capture for specific apps (fixes apps that crash on UID switch).--audio-filter-apps: Ignore specific apps (e.g., Display over other apps) to correctly target the underlying app.