Pause / Resume audio to match activity life cycle #376
Conversation
|
Warning Rate limit exceeded@joshuatam has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 7 minutes and 33 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughReplaces XEnvironment's inline AudioManager/AudioDeviceCallback logic with a new AudioMonitor class that detects output-device changes, debounces and coordinates ALSA/PulseAudio restart behavior, and exposes lifecycle hooks; XEnvironment now constructs AudioMonitor and delegates onPause/onResume. Changes
Sequence Diagram(s)sequenceDiagram
participant AL as Android Lifecycle
participant XE as XEnvironment
participant AM as AudioMonitor
participant ALSA as ALSAServerComponent
participant PA as PulseAudioComponent
rect rgb(235,245,255)
note right of XE: App resume
AL->>XE: onResume()
XE->>AM: onResume()
AM->>AM: register AudioDeviceCallback
end
rect rgb(255,245,235)
note right of AM: Device change detected
AM->>AM: onAudioDevicesAdded/Removed(devices)
AM->>AM: filter sinks & apply skip-list
alt relevant sink change & not Android15+
AM->>AM: schedule debounced restart (500ms, guarded)
AM->>ALSA: stop
AM->>ALSA: start
AM->>PA: start
end
end
rect rgb(245,255,235)
note right of XE: App pause
AL->>XE: onPause()
XE->>AM: onPause()
AM->>AM: unregister AudioDeviceCallback
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/src/main/java/com/winlator/xenvironment/XEnvironment.java (1)
113-136: Consider marking audioCallbackRegistered as volatile for consistency.The
audioCallbackRegisteredfield is read and modified without synchronization inonPause()(lines 113, 115) andonResume()(lines 130, 132). While Android's lifecycle callbacks execute on the main thread, the class already demonstrates synchronization awareness with theisWinetricksRunning()andsetWinetricksRunning()synchronized methods for similar state management. For consistency and defensive coding, consider marking the field as volatile:- private boolean audioCallbackRegistered = false; + private volatile boolean audioCallbackRegistered = false;This is a minor improvement; the current code has low practical risk since all accesses occur on the main thread and the
audioDeviceCallbackdoes not read this field.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/src/main/java/com/winlator/xenvironment/XEnvironment.java
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (3)
app/src/main/java/com/winlator/xenvironment/XEnvironment.java (3)
146-168: Well-structured audio control helpers.The
pauseAudio()andresumeAudio()methods are properly encapsulated with appropriate null checks and clear naming. The symmetry between stopping and starting components is correct and improves code readability compared to inline logic.
112-144: Audio lifecycle integration is well-ordered.The pause/resume sequence is logically sound: unregister the audio device callback before pausing components, and register the callback before resuming. This ordering prevents spurious audio device change notifications during the paused state and ensures audio is properly initialized before programs resume.
170-182: Existing restartAudioComponent() remains consistent with new pause/resume logic.The asymmetric PulseAudio handling in
restartAudioComponent()(onlystart()called because stop occurs internally) is compatible with the newpauseAudio()andresumeAudio()methods, which directly callstop()andstart()respectively. The inline comment adequately explains the design.
|
This doesn't work on my device - audio stays paused after screen is turned off. |
joshuatam
force-pushed
the
fix/audio-pause-resume
branch
from
69506ad to
2d28505
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Fix all issues with AI Agents 🤖
In @app/src/main/java/com/winlator/xenvironment/XEnvironment.java:
- Around line 113-116: audioCallbackRegistered is read/modified from multiple
threads causing a race; mirror the pattern used for winetricksRunning by making
all access synchronized: add synchronized accessor methods (e.g.,
isAudioCallbackRegistered() and setAudioCallbackRegistered(boolean)) and replace
direct reads/writes of the audioCallbackRegistered field in onPause, onResume,
the constructor, and in the audioDeviceCallback/restartAudioComponent() code to
use those methods; ensure the unregisterAudioDeviceCallback(audioDeviceCallback)
call is guarded by the synchronized check so the flag and registration state
cannot race.
- Around line 146-156: pauseAudio stops ALSAServerComponent and
PulseAudioComponent without recording whether they were running, so resumeAudio
cannot restore prior state; add boolean fields (e.g., wasAlsaRunning,
wasPulseRunning) to XEnvironment, set each flag in pauseAudio by checking the
component's running state (e.g., call ALSAServerComponent.isRunning() and
PulseAudioComponent.isRunning() if available) before calling stop(), and in
resumeAudio only call start() on ALSAServerComponent and PulseAudioComponent
when their corresponding flag is true; after resuming clear or reset the flags
and guard all checks against null components.
🧹 Nitpick comments (1)
app/src/main/java/com/winlator/xenvironment/XEnvironment.java (1)
170-182: Code duplication and inconsistent component behavior.The
restartAudioComponent()method reveals important implementation details:
Inconsistency:
ALSAServerComponentrequires explicitstop()+start(), butPulseAudioComponent.start()internally callsstop()(line 179 comment).Duplication: This method essentially duplicates
pauseAudio()+resumeAudio()logic but handles the PulseAudio inconsistency differently. InpauseAudio()(line 154), PulseAudio is explicitly stopped, but this may be redundant sincestart()will stop it anyway.Maintenance risk: The behavioral difference between components isn't documented in
pauseAudio()/resumeAudio(), making the code harder to maintain.Consider one of these approaches:
Option 1: Extract common logic
private void restartAudioComponent() { - final ALSAServerComponent alsaServerComponent = getComponent(ALSAServerComponent.class); - if (alsaServerComponent != null) { - alsaServerComponent.stop(); - alsaServerComponent.start(); - } - - final PulseAudioComponent pulseAudioComponent = getComponent(PulseAudioComponent.class); - if (pulseAudioComponent != null) { - //pulseAudioComponent.stop(); stop is already called inside start function - pulseAudioComponent.start(); - } + pauseAudio(); + resumeAudio(); }Option 2: Document the inconsistency
private void pauseAudio() { final ALSAServerComponent alsaServerComponent = getComponent(ALSAServerComponent.class); if (alsaServerComponent != null) { alsaServerComponent.stop(); } + // Note: PulseAudioComponent.start() internally calls stop(), so this stop() is + // technically redundant. However, it's included for consistency with ALSA and + // to ensure audio is stopped even if start() is never called in resumeAudio(). final PulseAudioComponent pulseAudioComponent = getComponent(PulseAudioComponent.class); if (pulseAudioComponent != null) { pulseAudioComponent.stop(); } }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/src/main/java/com/winlator/xenvironment/XEnvironment.java
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (2)
app/src/main/java/com/winlator/xenvironment/XEnvironment.java (2)
158-168: No issues found. BothALSAServerComponent.start()andPulseAudioComponent.start()are safely designed to handle multiple invocations: ALSAServerComponent checks if the connector is already initialized and returns early, while PulseAudioComponent explicitly stops any existing process before starting a new one.
135-143: Ordering inonResume()is correct; audio components do not depend on guest processes.The audio components (ALSA and PulseAudio) are independent server/daemon processes. ALSAServerComponent is a Unix socket connector that handles audio requests, and PulseAudioComponent is a standalone PulseAudio daemon. Neither has dependencies on guest processes running.
The current ordering—resuming audio before processes—is appropriate: audio servers should be ready before guest processes attempt to use them.
This is not the cause of the reported "audio remains paused" bug.
joshuatam
force-pushed
the
fix/audio-pause-resume
branch
from
2d28505 to
6178d4d
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Fix all issues with AI Agents 🤖
In @app/src/main/java/com/winlator/xenvironment/XEnvironment.java:
- Around line 131-134: The resumeAudio() method is never invoked, so the audio
lifecycle never restarts; call resumeAudio() from the activity resume path where
audioManager.registerAudioDeviceCallback is handled (i.e., after
audioDeviceCallback is registered and audioCallbackRegistered is set, or
directly inside the onResume handler). Update the code around
audioManager.registerAudioDeviceCallback / audioCallbackRegistered to invoke
resumeAudio() (or ensure onResume calls resumeAudio()), referencing the
resumeAudio() method, audioManager.registerAudioDeviceCallback(...),
audioDeviceCallback, and audioCallbackRegistered to locate the change.
- Around line 116-120: onPause() never calls pauseAudio() and accesses
audioCallbackRegistered unsafely, so update onPause() to call pauseAudio() and
protect all accesses/modifications to audioCallbackRegistered (and the
unregister/register of audioDeviceCallback) using a synchronized accessor
pattern: add synchronized
isAudioCallbackRegistered()/setAudioCallbackRegistered(boolean) or synchronize
on a dedicated lock, then inside pauseAudio() and onPause() perform
unregisterAudioDeviceCallback(audioDeviceCallback) and
setAudioCallbackRegistered(false) within the same synchronized block to prevent
the race; ensure any other places that read/write audioCallbackRegistered use
the same synchronized accessors.
♻️ Duplicate comments (3)
app/src/main/java/com/winlator/xenvironment/XEnvironment.java (3)
50-54: Same API level concern as onAudioDevicesAdded.This segment uses the same stream-based approach as
onAudioDevicesAdded(lines 39-44), so the same API level verification applies.
144-154: Method is never invoked; missing state tracking.This method is never called (see lines 116-120). Even if it were called, the past review comment remains valid: stopping components without tracking their running state means
resumeAudio()cannot correctly restore the previous state.
156-166: Method is never invoked; unconditional start without state tracking.This method is never called (see lines 131-134). Even if it were called, it would unconditionally start components that may not have been running before pause, as flagged in past review comments.
🧹 Nitpick comments (1)
app/src/main/java/com/winlator/xenvironment/XEnvironment.java (1)
29-33: Consider using a Set for better lookup performance.The device types chosen are appropriate for filtering. However, since this array is checked repeatedly in the callback (lines 41, 52), consider using a
Set<Integer>for O(1) lookups instead of streaming the array each time.🔎 Proposed refactor using Set
- private final int[] skippedDeviceTypes = { - AudioDeviceInfo.TYPE_BUILTIN_EARPIECE, - AudioDeviceInfo.TYPE_BUILTIN_SPEAKER, - AudioDeviceInfo.TYPE_TELEPHONY - }; + private final Set<Integer> skippedDeviceTypes = new HashSet<>(Arrays.asList( + AudioDeviceInfo.TYPE_BUILTIN_EARPIECE, + AudioDeviceInfo.TYPE_BUILTIN_SPEAKER, + AudioDeviceInfo.TYPE_TELEPHONY + ));Then update the callback checks:
- if (Arrays.stream(addedDevices).allMatch(device -> - device.isSink() && - Arrays.stream(skippedDeviceTypes).allMatch(type -> type != device.getType()) - )) { + if (Arrays.stream(addedDevices).allMatch(device -> + device.isSink() && + !skippedDeviceTypes.contains(device.getType()) + )) {Add the import:
+import java.util.HashSet; +import java.util.Set;
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/src/main/java/com/winlator/xenvironment/XEnvironment.java
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (2)
app/src/main/java/com/winlator/xenvironment/XEnvironment.java (2)
17-17: LGTM!The import is correctly added to support the stream operations in the audio device callbacks.
39-44: No action needed. The app'sminSdkVersionis 26, which exceeds the API level 24 requirement forArrays.stream(). The Stream API usage is fully compatible with the project's configuration.
joshuatam
force-pushed
the
fix/audio-pause-resume
branch
from
6178d4d to
75c1a39
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
Fix all issues with AI Agents 🤖
In @app/src/main/java/com/winlator/audio/AudioMonitor.java:
- Line 36: The audioDevices field in AudioMonitor is accessed from both the main
thread (constructor) and the audio callback thread
(onAudioDevicesAdded/onAudioDevicesRemoved) and must be made thread-safe; update
the declaration of audioDevices to either use the volatile modifier or replace
it with an AtomicReference<AudioDeviceInfo[]> and then change all reads/writes
in the constructor, onAudioDevicesAdded, onAudioDevicesRemoved, and any other
access sites to use atomic get/set/compareAndSet operations so updates are
visible to the callback thread without races.
- Line 30: The boolean field audioCallbackRegistered is accessed from multiple
threads and must be protected; introduce a final lock object (e.g.,
callbackLock) and wrap every read/write of audioCallbackRegistered inside
synchronized(callbackLock) blocks (including in the constructor if it
manipulates registration state), and update onPause() and onResume() to
synchronize around callbackLock when checking/updating audioCallbackRegistered
and calling audioManager.unregisterAudioDeviceCallback(audioDeviceCallback) /
audioManager.registerAudioDeviceCallback(audioDeviceCallback, null) to prevent
the race.
- Around line 100-104: The version check in restartAudioComponent uses
Build.VERSION_CODES.UPSIDE_DOWN_CAKE but the comment says "android 15 or above";
update the constant to Build.VERSION_CODES.VANILLA_ICE_CREAM so the condition
(Build.VERSION.SDK_INT >= Build.VERSION_CODES.VANILLA_ICE_CREAM) matches Android
15 as intended and keep the rest of the restartAudioComponent method unchanged.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/src/main/java/com/winlator/xenvironment/XEnvironment.java (1)
81-94: Critical: Audio components are not paused/resumed during lifecycle transitions.Based on the PR objectives ("Pause / Resume audio to match activity life cycle") and the user bug report ("audio remains paused after the screen is turned off"), the implementation is incomplete. Looking at
AudioMonitor:
onPause()only unregisters the callback—it does not stopALSAServerComponentorPulseAudioComponentonResume()only registers the callback—it does not restart the audio componentsThe past review comments flagged missing
pauseAudio()/resumeAudio()calls, but the fix only moved callback management toAudioMonitorwithout adding actual audio stop/start logic.🔎 Proposed fix: Add pause/resume audio logic to AudioMonitor
Add methods to
AudioMonitorthat actually stop/start the audio components:// In AudioMonitor.java public void onPause() { if (audioCallbackRegistered) { audioManager.unregisterAudioDeviceCallback(audioDeviceCallback); audioCallbackRegistered = false; } // Actually pause audio components pauseAudioComponents(); } public void onResume() { if (!audioCallbackRegistered) { audioManager.registerAudioDeviceCallback(audioDeviceCallback, null); audioCallbackRegistered = true; } // Actually resume audio components resumeAudioComponents(); } private void pauseAudioComponents() { final ALSAServerComponent alsa = xEnvironment.getComponent(ALSAServerComponent.class); if (alsa != null) alsa.stop(); final PulseAudioComponent pulse = xEnvironment.getComponent(PulseAudioComponent.class); if (pulse != null) pulse.stop(); } private void resumeAudioComponents() { final ALSAServerComponent alsa = xEnvironment.getComponent(ALSAServerComponent.class); if (alsa != null) alsa.start(); final PulseAudioComponent pulse = xEnvironment.getComponent(PulseAudioComponent.class); if (pulse != null) pulse.start(); }
♻️ Duplicate comments (1)
app/src/main/java/com/winlator/audio/AudioMonitor.java (1)
129-141: Missing audio pause/resume functionality.As noted in the
XEnvironment.javareview, these lifecycle methods only manage callback registration. They do not stop or start the actual audio components (ALSAServerComponent,PulseAudioComponent), which is the stated objective of this PR.See the proposed fix in the
XEnvironment.javareview comment.
🧹 Nitpick comments (1)
app/src/main/java/com/winlator/audio/AudioMonitor.java (1)
20-20: Remove unused import.
ExecutorServiceis imported but never used in this file.🔎 Proposed fix
-import java.util.concurrent.ExecutorService;
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/src/main/java/com/winlator/audio/AudioMonitor.javaapp/src/main/java/com/winlator/xenvironment/XEnvironment.java
🧰 Additional context used
🧬 Code graph analysis (2)
app/src/main/java/com/winlator/xenvironment/XEnvironment.java (1)
app/src/main/java/com/winlator/audio/AudioMonitor.java (1)
AudioMonitor(25-142)
app/src/main/java/com/winlator/audio/AudioMonitor.java (3)
app/src/main/java/com/winlator/xenvironment/XEnvironment.java (1)
XEnvironment(15-102)app/src/main/java/com/winlator/xenvironment/components/ALSAServerComponent.java (1)
ALSAServerComponent(14-46)app/src/main/java/com/winlator/xenvironment/components/PulseAudioComponent.java (1)
PulseAudioComponent(19-96)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (2)
app/src/main/java/com/winlator/xenvironment/XEnvironment.java (1)
22-35: LGTM on the refactoring to AudioMonitor.The extraction of audio monitoring logic into a dedicated
AudioMonitorclass improves separation of concerns. The initialization in the constructor is appropriate.app/src/main/java/com/winlator/audio/AudioMonitor.java (1)
46-98: LGTM on device change detection logic.The filtering logic to detect new/removed sink devices while excluding built-in speakers is well-structured. The use of streams for filtering and the caching of
audioDevicesfor comparison is appropriate.
| private final XEnvironment xEnvironment; | ||
| private final AudioManager audioManager; | ||
| private final AtomicBoolean isRestartingAudio = new AtomicBoolean(false); | ||
| private boolean audioCallbackRegistered = false; |
There was a problem hiding this comment.
Race condition on audioCallbackRegistered flag.
The audioCallbackRegistered field is accessed from multiple threads without synchronization:
- Main thread:
onPause(),onResume(), constructor - Audio thread: callback methods (registered with
nullhandler, so uses system thread)
This is the same issue flagged in past reviews on XEnvironment.java—it was moved here but not fixed.
🔎 Proposed fix using synchronized accessors
- private boolean audioCallbackRegistered = false;
+ private volatile boolean audioCallbackRegistered = false;
+ private final Object callbackLock = new Object();Then update accessors:
public void onPause() {
synchronized (callbackLock) {
if (audioCallbackRegistered) {
audioManager.unregisterAudioDeviceCallback(audioDeviceCallback);
audioCallbackRegistered = false;
}
}
}
public void onResume() {
synchronized (callbackLock) {
if (!audioCallbackRegistered) {
audioManager.registerAudioDeviceCallback(audioDeviceCallback, null);
audioCallbackRegistered = true;
}
}
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In @app/src/main/java/com/winlator/audio/AudioMonitor.java around line 30, The
boolean field audioCallbackRegistered is accessed from multiple threads and must
be protected; introduce a final lock object (e.g., callbackLock) and wrap every
read/write of audioCallbackRegistered inside synchronized(callbackLock) blocks
(including in the constructor if it manipulates registration state), and update
onPause() and onResume() to synchronize around callbackLock when
checking/updating audioCallbackRegistered and calling
audioManager.unregisterAudioDeviceCallback(audioDeviceCallback) /
audioManager.registerAudioDeviceCallback(audioDeviceCallback, null) to prevent
the race.
| AudioDeviceInfo.TYPE_BUILTIN_SPEAKER, | ||
| AudioDeviceInfo.TYPE_TELEPHONY | ||
| }; | ||
| private AudioDeviceInfo[] audioDevices; |
There was a problem hiding this comment.
Thread safety issue with audioDevices field.
The audioDevices array is read and written from both the main thread (constructor) and the audio callback thread (in onAudioDevicesAdded/onAudioDevicesRemoved) without synchronization. This can cause stale reads or visibility issues.
🔎 Proposed fix
Make the field volatile or use synchronization:
- private AudioDeviceInfo[] audioDevices;
+ private volatile AudioDeviceInfo[] audioDevices;Alternatively, use AtomicReference<AudioDeviceInfo[]> for atomic updates.
Also applies to: 53-70, 79-96
🤖 Prompt for AI Agents
In @app/src/main/java/com/winlator/audio/AudioMonitor.java around line 36, The
audioDevices field in AudioMonitor is accessed from both the main thread
(constructor) and the audio callback thread
(onAudioDevicesAdded/onAudioDevicesRemoved) and must be made thread-safe; update
the declaration of audioDevices to either use the volatile modifier or replace
it with an AtomicReference<AudioDeviceInfo[]> and then change all reads/writes
in the constructor, onAudioDevicesAdded, onAudioDevicesRemoved, and any other
access sites to use atomic get/set/compareAndSet operations so updates are
visible to the callback thread without races.
75c1a39 to
d2ebd9e
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
app/src/main/java/com/winlator/audio/AudioMonitor.java (2)
30-30: Race condition onaudioCallbackRegisteredflag.The
audioCallbackRegisteredfield is accessed from multiple threads without synchronization: the main thread (constructor,onPause(),onResume()) and the audio callback thread (registered withnullhandler). This creates a race where callback registration state and flag updates can become inconsistent.This issue was flagged in previous reviews and remains unresolved. Based on learnings, mirror the synchronized accessor pattern used for
winetricksRunningin XEnvironment.🔎 Proposed fix using synchronized access
+private final Object callbackLock = new Object(); private boolean audioCallbackRegistered = false;Update
onPause():public void onPause() { - if (audioCallbackRegistered) { - audioManager.unregisterAudioDeviceCallback(audioDeviceCallback); - audioCallbackRegistered = false; + synchronized (callbackLock) { + if (audioCallbackRegistered) { + audioManager.unregisterAudioDeviceCallback(audioDeviceCallback); + audioCallbackRegistered = false; + } } }Update
onResume():public void onResume() { - if (!audioCallbackRegistered) { - audioManager.registerAudioDeviceCallback(audioDeviceCallback, null); - audioCallbackRegistered = true; + synchronized (callbackLock) { + if (!audioCallbackRegistered) { + audioManager.registerAudioDeviceCallback(audioDeviceCallback, null); + audioCallbackRegistered = true; + } } }Update constructor:
public AudioMonitor(XEnvironment xEnvironment, Context context) { this.xEnvironment = xEnvironment; this.audioManager = (AudioManager) context.getSystemService(Context.AUDIO_SERVICE); this.audioDevices = this.audioManager.getDevices(GET_DEVICES_OUTPUTS); - this.audioCallbackRegistered = true; - this.audioManager.registerAudioDeviceCallback(audioDeviceCallback, null); + synchronized (callbackLock) { + this.audioCallbackRegistered = true; + this.audioManager.registerAudioDeviceCallback(audioDeviceCallback, null); + } }
36-36: Thread safety issue withaudioDevicesfield.The
audioDevicesarray is read in the constructor on the main thread and written from the audio callback thread (lines 70, 96) without synchronization. This can cause visibility issues where callback methods see stale device lists.This issue was flagged in previous reviews and remains unresolved.
🔎 Proposed fix using volatile
-private AudioDeviceInfo[] audioDevices; +private volatile AudioDeviceInfo[] audioDevices;Alternatively, use
AtomicReference<AudioDeviceInfo[]>for atomic updates:-private AudioDeviceInfo[] audioDevices; +private final AtomicReference<AudioDeviceInfo[]> audioDevices = new AtomicReference<>();Then update all access points to use
audioDevices.get()andaudioDevices.set().
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/src/main/java/com/winlator/audio/AudioMonitor.javaapp/src/main/java/com/winlator/xenvironment/XEnvironment.java
🧰 Additional context used
🧬 Code graph analysis (2)
app/src/main/java/com/winlator/audio/AudioMonitor.java (3)
app/src/main/java/com/winlator/xenvironment/XEnvironment.java (1)
XEnvironment(15-102)app/src/main/java/com/winlator/xenvironment/components/ALSAServerComponent.java (1)
ALSAServerComponent(14-46)app/src/main/java/com/winlator/xenvironment/components/PulseAudioComponent.java (1)
PulseAudioComponent(19-96)
app/src/main/java/com/winlator/xenvironment/XEnvironment.java (1)
app/src/main/java/com/winlator/audio/AudioMonitor.java (1)
AudioMonitor(25-142)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (6)
app/src/main/java/com/winlator/xenvironment/XEnvironment.java (2)
5-5: Clean refactoring to centralize audio monitoring.The introduction of AudioMonitor as a dedicated class improves separation of concerns and encapsulates audio device monitoring logic.
Also applies to: 22-22
35-35: Correct AudioMonitor instantiation.The constructor properly initializes AudioMonitor with the required dependencies.
app/src/main/java/com/winlator/audio/AudioMonitor.java (4)
38-44: Constructor initialization is correct.The constructor properly initializes the AudioManager, captures the initial device list, and registers the audio device callback.
48-71: Device addition handling logic is correct.The filtering logic properly identifies new sink devices that should trigger an audio restart, excluding built-in speakers and earpieces. The stream-based filtering is clear and correct.
73-97: Device removal handling logic is correct.The removal handling mirrors the addition logic appropriately, ensuring audio is restarted when meaningful devices (e.g., headphones) are disconnected.
100-127: Audio restart logic is well-implemented.The restart sequence correctly:
- Skips on Android 15+ where it's unnecessary (using the correct VANILLA_ICE_CREAM constant)
- Uses AtomicBoolean to prevent concurrent restarts
- Delays 500ms to allow device changes to stabilize
- Stops and starts ALSA server
- Restarts PulseAudio (which internally stops before starting)
joshuatam
force-pushed
the
fix/audio-pause-resume
branch
from
d2ebd9e to
2ca3219
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Fix all issues with AI Agents 🤖
In @app/src/main/java/com/winlator/audio/AudioMonitor.java:
- Around line 100-125: There is a race between restartAudioComponent() and
lifecycle methods; add a shared final Object audioComponentLock field and
synchronize all audio lifecycle operations on it: in restartAudioComponent()
wrap the version check, isRestartingAudio check/set/clear and the
ALSAServerComponent/PulseAudioComponent stop/start calls inside
synchronized(audioComponentLock) so the method atomically checks/sets
isRestartingAudio and performs component restarts; also ensure onPause() and
onResume() use the same synchronized(audioComponentLock) block around their
component stop/start logic so lifecycle and device-change callbacks cannot
interleave.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/src/main/java/com/winlator/xenvironment/XEnvironment.java (1)
82-94: Critical: Audio components still not stopped/started during lifecycle events.Despite past reviews flagging this issue as critical and marking it "✅ Addressed", the current implementation only unregisters/registers the audio device callback—it does not stop or start the actual audio components (ALSAServerComponent and PulseAudioComponent).
This is the root cause of the reported bug where "audio remains paused after the screen is turned off." When the activity pauses, audio continues playing because the components are never stopped. When it resumes, the user expects audio to work but experiences issues because the lifecycle state is incorrect.
The
restartAudioComponent()method in AudioMonitor demonstrates the correct component lifecycle management (stop/start), but it's only invoked by device callbacks, not byonPause()/onResume().🔎 Proposed fix to add component lifecycle management
In AudioMonitor.java, add state tracking fields:
private boolean alsaWasRunning = false; private boolean pulseAudioWasRunning = false;Update AudioMonitor.onPause() to stop components:
public void onPause() { if (audioCallbackRegistered) { audioManager.unregisterAudioDeviceCallback(audioDeviceCallback); audioCallbackRegistered = false; } + + // Stop audio components and track their running state + final ALSAServerComponent alsaServerComponent = xEnvironment.getComponent(ALSAServerComponent.class); + if (alsaServerComponent != null) { + // Check if component has isRunning() method; if not, assume it was running if non-null + alsaWasRunning = true; // or use alsaServerComponent.isRunning() if available + alsaServerComponent.stop(); + } + + final PulseAudioComponent pulseAudioComponent = xEnvironment.getComponent(PulseAudioComponent.class); + if (pulseAudioComponent != null) { + pulseAudioWasRunning = true; // or use pulseAudioComponent.isRunning() if available + pulseAudioComponent.stop(); + } }Update AudioMonitor.onResume() to restart components:
public void onResume() { if (!audioCallbackRegistered) { audioManager.registerAudioDeviceCallback(audioDeviceCallback, null); audioCallbackRegistered = true; } + + // Restart audio components if they were running before pause + final ALSAServerComponent alsaServerComponent = xEnvironment.getComponent(ALSAServerComponent.class); + if (alsaServerComponent != null && alsaWasRunning) { + alsaServerComponent.start(); + } + + final PulseAudioComponent pulseAudioComponent = xEnvironment.getComponent(PulseAudioComponent.class); + if (pulseAudioComponent != null && pulseAudioWasRunning) { + pulseAudioComponent.start(); + } }Note: Verify whether ALSAServerComponent and PulseAudioComponent have
isRunning()methods. Based on the snippets provided, they havestart()andstop()methods butisRunning()is not shown. You may need to add state tracking or check ifconnector != nullfor ALSA andpid != -1for PulseAudio.
♻️ Duplicate comments (2)
app/src/main/java/com/winlator/audio/AudioMonitor.java (2)
30-30: Race condition on callback registration flag.
audioCallbackRegisteredis accessed from the main thread (constructor,onPause(),onResume()) without synchronization. While the callbacks themselves run on the system thread, there's a window where:
onPause()checks the flag on the main thread- Callback fires on system thread during unregistration
onPause()unregisters and clears the flagThis can lead to callbacks being invoked after the flag indicates they're unregistered. The pattern used for
winetricksRunningin XEnvironment (lines 24-30) shows the correct synchronized accessor approach.🔎 Proposed fix using synchronized access
- private boolean audioCallbackRegistered = false; + private final Object callbackLock = new Object(); + private boolean audioCallbackRegistered = false;Update the constructor:
public AudioMonitor(XEnvironment xEnvironment, Context context) { this.xEnvironment = xEnvironment; this.audioManager = (AudioManager) context.getSystemService(Context.AUDIO_SERVICE); this.audioDevices = this.audioManager.getDevices(GET_DEVICES_OUTPUTS); - this.audioCallbackRegistered = true; - this.audioManager.registerAudioDeviceCallback(audioDeviceCallback, null); + synchronized (callbackLock) { + this.audioManager.registerAudioDeviceCallback(audioDeviceCallback, null); + this.audioCallbackRegistered = true; + } }Update
onPause()andonResume():public void onPause() { - if (audioCallbackRegistered) { - audioManager.unregisterAudioDeviceCallback(audioDeviceCallback); - audioCallbackRegistered = false; + synchronized (callbackLock) { + if (audioCallbackRegistered) { + audioManager.unregisterAudioDeviceCallback(audioDeviceCallback); + audioCallbackRegistered = false; + } } } public void onResume() { - if (!audioCallbackRegistered) { - audioManager.registerAudioDeviceCallback(audioDeviceCallback, null); - audioCallbackRegistered = true; + synchronized (callbackLock) { + if (!audioCallbackRegistered) { + audioManager.registerAudioDeviceCallback(audioDeviceCallback, null); + audioCallbackRegistered = true; + } } }
36-36: Thread safety issue withaudioDevicesarray.The
audioDevicesfield is read and written from multiple threads without synchronization:
- Constructor (main thread): initializes the array
onAudioDevicesAdded(callback thread): reads at lines 53-67, writes at line 70onAudioDevicesRemoved(callback thread): reads at lines 79-93, writes at line 96This can cause visibility issues where one thread sees a stale array reference, or torn reads/writes during concurrent access.
🔎 Proposed fix using volatile or AtomicReference
Option 1: Use volatile (simpler for array reference updates):
- private AudioDeviceInfo[] audioDevices; + private volatile AudioDeviceInfo[] audioDevices;Option 2: Use AtomicReference (more explicit atomicity):
+ import java.util.concurrent.atomic.AtomicReference; ... - private AudioDeviceInfo[] audioDevices; + private final AtomicReference<AudioDeviceInfo[]> audioDevices = new AtomicReference<>();Then update all accesses:
- Constructor:
audioDevices.set(this.audioManager.getDevices(GET_DEVICES_OUTPUTS))- Reads:
audioDevices.get().length,Arrays.stream(audioDevices.get())- Writes:
audioDevices.set(audioManager.getDevices(GET_DEVICES_OUTPUTS))The volatile approach is simpler and sufficient since you're replacing the entire array reference each time.
🧹 Nitpick comments (2)
app/src/main/java/com/winlator/audio/AudioMonitor.java (2)
10-11: Remove unused imports.The
HandlerandLooperimports are not used anywhere in the code.🔎 Proposed cleanup
import android.media.AudioManager; import android.os.Build; -import android.os.Handler; -import android.os.Looper;
20-20: Remove unused import.The
ExecutorServiceimport is not used anywhere in the code.🔎 Proposed cleanup
import java.util.Arrays; -import java.util.concurrent.ExecutorService; import java.util.concurrent.atomic.AtomicBoolean;
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/src/main/java/com/winlator/audio/AudioMonitor.javaapp/src/main/java/com/winlator/xenvironment/XEnvironment.java
🧰 Additional context used
🧬 Code graph analysis (2)
app/src/main/java/com/winlator/xenvironment/XEnvironment.java (1)
app/src/main/java/com/winlator/audio/AudioMonitor.java (1)
AudioMonitor(25-140)
app/src/main/java/com/winlator/audio/AudioMonitor.java (4)
app/src/main/java/com/winlator/xenvironment/XEnvironment.java (1)
XEnvironment(15-102)app/src/main/java/com/winlator/xenvironment/components/ALSAServerComponent.java (1)
ALSAServerComponent(14-46)app/src/main/java/com/winlator/xenvironment/components/PulseAudioComponent.java (1)
PulseAudioComponent(19-96)app/src/main/java/com/winlator/core/ArrayUtils.java (1)
ArrayUtils(10-44)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
| private void restartAudioComponent() { | ||
| // if it is android 15 or above skip restart | ||
| if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.VANILLA_ICE_CREAM) { | ||
| return; | ||
| } | ||
|
|
||
| if (isRestartingAudio.get()) { | ||
| return; | ||
| } | ||
|
|
||
| isRestartingAudio.set(true); | ||
|
|
||
| final ALSAServerComponent alsaServerComponent = xEnvironment.getComponent(ALSAServerComponent.class); | ||
| if (alsaServerComponent != null) { | ||
| alsaServerComponent.stop(); | ||
| alsaServerComponent.start(); | ||
| } | ||
|
|
||
| final PulseAudioComponent pulseAudioComponent = xEnvironment.getComponent(PulseAudioComponent.class); | ||
| if (pulseAudioComponent != null) { | ||
| //pulseAudioComponent.stop(); stop is already called inside start function | ||
| pulseAudioComponent.start(); | ||
| } | ||
|
|
||
| isRestartingAudio.set(false); | ||
| } |
There was a problem hiding this comment.
Potential race condition between device-change restarts and lifecycle management.
The restartAudioComponent() method is invoked from the audio callback thread when devices change, but if onPause()/onResume() were to stop/start components (as they should per the critical issue flagged above), there would be a race:
- User pauses app →
onPause()stops ALSA/PulseAudio on main thread - Device callback fires →
restartAudioComponent()starts components on callback thread - Components are now running despite the app being paused
The isRestartingAudio flag prevents concurrent restartAudioComponent() calls but doesn't coordinate with lifecycle management.
🔎 Proposed fix using shared synchronization
Add a lock to coordinate all audio component lifecycle operations:
private final XEnvironment xEnvironment;
private final AudioManager audioManager;
+ private final Object audioComponentLock = new Object();
private final AtomicBoolean isRestartingAudio = new AtomicBoolean(false);Update restartAudioComponent():
private void restartAudioComponent() {
if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.VANILLA_ICE_CREAM) {
return;
}
if (isRestartingAudio.get()) {
return;
}
isRestartingAudio.set(true);
- final ALSAServerComponent alsaServerComponent = xEnvironment.getComponent(ALSAServerComponent.class);
- if (alsaServerComponent != null) {
- alsaServerComponent.stop();
- alsaServerComponent.start();
- }
-
- final PulseAudioComponent pulseAudioComponent = xEnvironment.getComponent(PulseAudioComponent.class);
- if (pulseAudioComponent != null) {
- //pulseAudioComponent.stop(); stop is already called inside start function
- pulseAudioComponent.start();
- }
+ synchronized (audioComponentLock) {
+ final ALSAServerComponent alsaServerComponent = xEnvironment.getComponent(ALSAServerComponent.class);
+ if (alsaServerComponent != null) {
+ alsaServerComponent.stop();
+ alsaServerComponent.start();
+ }
+
+ final PulseAudioComponent pulseAudioComponent = xEnvironment.getComponent(PulseAudioComponent.class);
+ if (pulseAudioComponent != null) {
+ //pulseAudioComponent.stop(); stop is already called inside start function
+ pulseAudioComponent.start();
+ }
+ }
isRestartingAudio.set(false);
}Apply the same synchronized (audioComponentLock) block around component stop/start operations in onPause() and onResume() once those are added (per the critical issue fix).
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In @app/src/main/java/com/winlator/audio/AudioMonitor.java around lines 100-125,
There is a race between restartAudioComponent() and lifecycle methods; add a
shared final Object audioComponentLock field and synchronize all audio lifecycle
operations on it: in restartAudioComponent() wrap the version check,
isRestartingAudio check/set/clear and the
ALSAServerComponent/PulseAudioComponent stop/start calls inside
synchronized(audioComponentLock) so the method atomically checks/sets
isRestartingAudio and performs component restarts; also ensure onPause() and
onResume() use the same synchronized(audioComponentLock) block around their
component stop/start logic so lifecycle and device-change callbacks cannot
interleave.
2ca3219 to
e8b5c75
Compare
Moves audio device monitoring to a dedicated AudioMonitor class, centralizing audio management and simplifying the XEnvironment class. Introduces a delay before restarting audio components on device changes, preventing unnecessary restarts and improving audio stability. Skips audio restart on Android 15 and above. Fixes audio issues during pause/resume.
joshuatam
force-pushed
the
fix/audio-pause-resume
branch
from
e8b5c75 to
03bb1e3
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Fix all issues with AI Agents 🤖
In @app/src/main/java/com/winlator/audio/AudioMonitor.java:
- Around line 108-113: onResume() currently only re-registers the device
callback and fails to restart ALSA/PulseAudio; modify onResume() to restart
audio components under the same synchronization used in onPause() (e.g.,
synchronized(audioLock) or the lock used around lines 82-99), calling the
component start methods (e.g., startAlsa(), startPulseAudio() or
restartAudioComponents()) if they are not running, then re-register
audioDeviceCallback and set audioCallbackRegistered = true; ensure you mirror
the stop/start ordering and state checks from onPause() to avoid races.
♻️ Duplicate comments (2)
app/src/main/java/com/winlator/audio/AudioMonitor.java (2)
29-29: Race condition onaudioCallbackRegisteredflag.The
audioCallbackRegisteredfield is accessed from multiple threads without synchronization:
- Main thread: constructor (line 42),
onPause()(line 102),onResume()(line 109)- Audio callback thread: implicitly accessed when callbacks fire (registered with
nullhandler at line 43)This is the same critical concurrency issue flagged in past reviews.
🔎 Proposed fix using volatile and synchronized blocks
- private boolean audioCallbackRegistered = false; + private volatile boolean audioCallbackRegistered = false; + private final Object callbackLock = new Object();Then update the lifecycle methods:
public void onPause() { synchronized (callbackLock) { if (audioCallbackRegistered) { audioManager.unregisterAudioDeviceCallback(audioDeviceCallback); audioCallbackRegistered = false; } } } public void onResume() { synchronized (callbackLock) { if (!audioCallbackRegistered) { audioManager.registerAudioDeviceCallback(audioDeviceCallback, null); audioCallbackRegistered = true; } } }Based on past review feedback.
82-99: Race condition between device-change restarts and lifecycle management.The
restartAudioComponent()method can be invoked from a delayed callback on the main thread, but there's no synchronization withonPause()/onResume(). This creates a race:
- User pauses app →
onPause()unregisters callback (but doesn't stop components)- Pending delayed restart fires →
restartAudioComponent()starts components- Components are now running despite the app being paused
This is the same issue flagged in past reviews and remains unaddressed.
🔎 Proposed fix using shared synchronization
Add a lock to coordinate all audio component lifecycle operations:
private final XEnvironment xEnvironment; private final AudioManager audioManager; + private final Object audioComponentLock = new Object(); private boolean audioCallbackRegistered = false;Update
restartAudioComponent():private void restartAudioComponent() { if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.VANILLA_ICE_CREAM) { return; } + synchronized (audioComponentLock) { final ALSAServerComponent alsaServerComponent = xEnvironment.getComponent(ALSAServerComponent.class); if (alsaServerComponent != null) { alsaServerComponent.stop(); alsaServerComponent.start(); } final PulseAudioComponent pulseAudioComponent = xEnvironment.getComponent(PulseAudioComponent.class); if (pulseAudioComponent != null) { //pulseAudioComponent.stop(); stop is already called inside start function pulseAudioComponent.start(); } + } }Apply the same
synchronized (audioComponentLock)block inonPause()andonResume()once those methods are updated to stop/start components (see critical issue below).Based on past review feedback.
🧹 Nitpick comments (3)
app/src/main/java/com/winlator/audio/AudioMonitor.java (3)
20-21: Remove unused imports.
ExecutorServiceandAtomicBooleanare imported but never used in this class.🔎 Proposed fix
import org.apache.commons.lang3.ArrayUtils; import java.util.Arrays; -import java.util.concurrent.ExecutorService; -import java.util.concurrent.atomic.AtomicBoolean; import timber.log.Timber;
30-34: Remove unusedskippedDeviceTypesfield.The
skippedDeviceTypesarray is declared but never used. The device-change logic at lines 59 and 76 checks onlyisSink()without filtering against these types.🔎 Proposed fix
If these device types should truly be skipped, update the filtering logic:
// Check if any new device is sink and all new devices are not in skippedDeviceTypes - if (Arrays.stream(addedDevices).anyMatch(AudioDeviceInfo::isSink)) { + if (Arrays.stream(addedDevices) + .filter(AudioDeviceInfo::isSink) + .anyMatch(device -> !ArrayUtils.contains(skippedDeviceTypes, device.getType()))) { restartHandler.postDelayed(restartRunnable, restartDelay); }Apply the same pattern at line 76. Otherwise, remove the unused field.
48-62: Overly broad exception handling.The
try-catchblock at lines 51-53 catchesException, butHandler.removeCallbacks()should not throw exceptions. This catch block can be removed.🔎 Proposed fix
- try { - restartHandler.removeCallbacks(restartRunnable); - } catch (Exception ignored) { - } + restartHandler.removeCallbacks(restartRunnable);Apply the same change at lines 67-70 in
onAudioDevicesRemoved.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/src/main/java/com/winlator/audio/AudioMonitor.javaapp/src/main/java/com/winlator/xenvironment/XEnvironment.java
🚧 Files skipped from review as they are similar to previous changes (1)
- app/src/main/java/com/winlator/xenvironment/XEnvironment.java
🧰 Additional context used
🧬 Code graph analysis (1)
app/src/main/java/com/winlator/audio/AudioMonitor.java (4)
app/src/main/java/com/winlator/xenvironment/XEnvironment.java (1)
XEnvironment(15-102)app/src/main/java/com/winlator/xenvironment/components/ALSAServerComponent.java (1)
ALSAServerComponent(14-46)app/src/main/java/com/winlator/xenvironment/components/PulseAudioComponent.java (1)
PulseAudioComponent(19-96)app/src/main/java/app/gamenative/MainActivity.kt (2)
onPause(268-275)onResume(255-266)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (1)
app/src/main/java/com/winlator/audio/AudioMonitor.java (1)
39-44: Constructor logic is sound.The callback registration in the constructor is appropriate. This ensures audio device monitoring starts immediately when
AudioMonitoris instantiated byXEnvironment.
| public void onPause() { | ||
| if (audioCallbackRegistered) { | ||
| audioManager.unregisterAudioDeviceCallback(audioDeviceCallback); | ||
| audioCallbackRegistered = false; | ||
| } | ||
| } |
There was a problem hiding this comment.
CRITICAL: onPause() must stop audio components to match lifecycle.
The current implementation only unregisters the device callback but does not stop the audio components (ALSA and PulseAudio). This is inconsistent with the PR objective ("Pause / Resume audio to match activity life cycle") and likely the root cause of the bug reported by utkarshdalal: "audio remains paused after the screen is turned off."
When the activity is paused (e.g., screen off, app backgrounded), the audio components should be stopped. When resumed, they should be restarted.
🔎 Proposed fix to stop audio components in onPause
public void onPause() {
+ // Cancel any pending restarts
+ restartHandler.removeCallbacks(restartRunnable);
+
+ // Unregister device callback
if (audioCallbackRegistered) {
audioManager.unregisterAudioDeviceCallback(audioDeviceCallback);
audioCallbackRegistered = false;
}
+
+ // Stop audio components to match activity lifecycle
+ final ALSAServerComponent alsaServerComponent = xEnvironment.getComponent(ALSAServerComponent.class);
+ if (alsaServerComponent != null) {
+ alsaServerComponent.stop();
+ }
+
+ final PulseAudioComponent pulseAudioComponent = xEnvironment.getComponent(PulseAudioComponent.class);
+ if (pulseAudioComponent != null) {
+ pulseAudioComponent.stop();
+ }
}Important: Add synchronization around the component stop/start logic as suggested in the race-condition fix above (lines 82-99).
Committable suggestion skipped: line range outside the PR's diff.
| public void onResume() { | ||
| if (!audioCallbackRegistered) { | ||
| audioManager.registerAudioDeviceCallback(audioDeviceCallback, null); | ||
| audioCallbackRegistered = true; | ||
| } | ||
| } |
There was a problem hiding this comment.
CRITICAL: onResume() must restart audio components to match lifecycle.
The current implementation only re-registers the device callback but does not restart the audio components (ALSA and PulseAudio). To properly resume audio playback when the activity resumes (e.g., screen on, app foregrounded), the components must be restarted.
This is the counterpart to the critical issue in onPause().
🔎 Proposed fix to restart audio components in onResume
public void onResume() {
+ // Re-register device callback
if (!audioCallbackRegistered) {
audioManager.registerAudioDeviceCallback(audioDeviceCallback, null);
audioCallbackRegistered = true;
}
+
+ // Restart audio components to match activity lifecycle
+ final ALSAServerComponent alsaServerComponent = xEnvironment.getComponent(ALSAServerComponent.class);
+ if (alsaServerComponent != null) {
+ alsaServerComponent.stop();
+ alsaServerComponent.start();
+ }
+
+ final PulseAudioComponent pulseAudioComponent = xEnvironment.getComponent(PulseAudioComponent.class);
+ if (pulseAudioComponent != null) {
+ pulseAudioComponent.start();
+ }
}Important: Add synchronization around the component stop/start logic as suggested in the race-condition fix above (lines 82-99).
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In @app/src/main/java/com/winlator/audio/AudioMonitor.java around lines 108-113,
onResume() currently only re-registers the device callback and fails to restart
ALSA/PulseAudio; modify onResume() to restart audio components under the same
synchronization used in onPause() (e.g., synchronized(audioLock) or the lock
used around lines 82-99), calling the component start methods (e.g.,
startAlsa(), startPulseAudio() or restartAudioComponents()) if they are not
running, then re-register audioDeviceCallback and set audioCallbackRegistered =
true; ensure you mirror the stop/start ordering and state checks from onPause()
to avoid races.
2 participants
Summary by CodeRabbit
Bug Fixes
Refactor
New Features
✏️ Tip: You can customize this high-level summary in your review settings.