Repository navigation
Fix part of #6446: Fix lock contention ANR in AudioFragment and AudioPlayerController - #6464
Kishan8548 wants to merge 9 commits into
Conversation
Coverage ReportResultsNumber of files assessed: 25 Exempted coverageFiles exempted from coverage
|
There was a problem hiding this comment.
🟡 Changes recommended
There are correctness issues around cancellation/retry behavior (stale cancelled jobs can still start prepares, and URI caching can block retries after failures).
Get a fresh assessment by requesting another Copilot review.
Pull request overview
This PR addresses ANRs caused by MediaPlayer.reset()/release() blocking the UI thread during network-backed audio operations, and reduces redundant audio reloads triggered by repeated ephemeral state recomputation in the player UI.
Changes:
- Offloads blocking
MediaPlayerlifecycle operations to a background dispatcher and serializes them via a coroutineMutex. - Adds URI-level short-circuiting in
AudioViewModelto avoid reloading the same voiceover repeatedly. - Updates/extends Robolectric tests to synchronize coroutine execution and cover rapid load/teardown scenarios.
File summaries
| File | Description |
|---|---|
| domain/src/main/java/org/oppia/android/domain/audio/AudioPlayerController.kt | Moves reset/release/prepare sequencing off the main thread and adds coroutine-based serialization/cancellation. |
| app/src/main/java/org/oppia/android/app/player/audio/AudioViewModel.kt | Tracks the currently loaded voiceover URI to prevent redundant changeDataSource() calls. |
| domain/src/test/java/org/oppia/android/domain/audio/AudioPlayerControllerTest.kt | Adjusts tests for coroutine scheduling and adds concurrency-focused controller tests. |
| app/src/sharedTest/java/org/oppia/android/app/player/audio/AudioFragmentTest.kt | Adds a regression test to ensure repeated same-audio state updates don’t restart playback. |
Review details
- Files reviewed: 4/4 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
@Kishan8548, please be sure to assign a reviewer if the PR is ready. |
|
PTAL @Neer-rn |
|
Unassigning @Kishan8548 since a re-review was requested. @Kishan8548, please make sure you have addressed all review comments. Thanks! |
Neer-rn
left a comment
Neer-rn
left a comment
There was a problem hiding this comment.
Thanks for the work @Kishan8548. The PR mostly looks good, I have left few comments PTAL.
|
Hi @Kishan8548, it looks like some changes were requested on this pull request by @Neer-rn. PTAL. Thanks! |
|
@Neer-rn PTAL, addressed all requested changes and added corresponding tests. |
|
Unassigning @Kishan8548 since a re-review was requested. @Kishan8548, please make sure you have addressed all review comments. Thanks! |
Neer-rn
left a comment
Neer-rn
left a comment
There was a problem hiding this comment.
@Kishan8548 I again reviewed your PR but with the help of LLM this time and found a few more issues that need to be addressed. Left few comments PTAL.
|
Unassigning @Neer-rn since the review is done. |
|
Hi @Kishan8548, it looks like some changes were requested on this pull request by @Neer-rn. PTAL. Thanks! |
|
Hi @Kishan8548, it looks like some changes were requested on this pull request by @Neer-rn. PTAL. Thanks! |
Coverage ReportResultsNumber of files assessed: 43 Passing coverageFiles with passing code coverage
Exempted coverageFiles exempted from coverage
|
|
@Neer-rn PTAL |
Neer-rn
left a comment
Neer-rn
left a comment
There was a problem hiding this comment.
Thanks for the work @Kishan8548. Great work!!!
|
Unassigning @Neer-rn since they have already approved the PR. |
|
Assigning @adhiamboperes for code owner reviews. Thanks! |
adhiamboperes
left a comment
adhiamboperes
left a comment
There was a problem hiding this comment.
Thanks @Kishan8548!
This PR fixes a part of the ANR, but not all of it, so I would propose the title and headline to reflect that.
- Duplicate loading is partially fixed.
AudioViewModelskipped reloading the same URI once audio was prepared, playing, paused, or completed. It still restarted the same source while loading, which is the particularly risky case. - Some protection added against stale callbacks. Load IDs and player identity checks helped reject obsolete preparation callbacks.
- Main-thread blocking is not yet fixed. reset(), release(), and other media operations still run synchronously under audioLock. Both paths in the ANR reports remained.
- Progress polling moved in the wrong direction. The changes explicitly moved media position reads onto the main thread.
Do you think that you may be able to fix the outstanding issues in this PR? Alternatively, we can merge this PR as is, as "Fixes part of #".
What is left:
- give MediaPlayer a dedicated background thread with a Looper and serialize the player's lifecycle on that thread.
- avoid making the main thread wait on a lock held while MediaPlayer operations are running.
| nextUpdateJob = CoroutineScope(backgroundDispatcher).launch { | ||
| updateSeekBar() | ||
| nextUpdateJob = CoroutineScope(blockingDispatcher).launch { | ||
| withContext(Dispatchers.Main) { |
There was a problem hiding this comment.
This switches to Dispatchers.Main before reading isPlaying, currentPosition, and duration under audioLock. Previously these reads ran in the background. Keep media reads on the player’s owning thread and dispatch only the resulting UI state.
The ANR is likely caused by making synchronous calls on the main thread while an audio network operation is still in progress.
There was a problem hiding this comment.
Done. Removed withContext(Dispatchers.Main) from scheduleNextSeekBarUpdate so that isPlaying, currentPosition, and duration reads stay on the background blockingDispatcher thread under audioLock, and dispatched the resulting progress via postValue().
|
|
||
| if (languageCodeForDataSource != null) { | ||
| val targetUri = voiceOverToUri(voiceoverMap[languageCodeForDataSource]) | ||
| val currentStatus = (playProgressResultLiveData.value as? AsyncResult.Success)?.value?.type |
There was a problem hiding this comment.
Duplicate requests still restart audio while it is loading.
This only skips loading for successful prepared/playback states. While the source is AsyncResult.Pending, another request for the same URI calls changeDataSource() again. The setter followed by loadMainContentAudio() can therefore still reset an in-flight network request. Track and deduplicate the requested source during preparation too, while preserving explicit retry behavior.
There was a problem hiding this comment.
Done. Updated AudioViewModel to track the requested audio source identity (currentLoadedAudioUri, currentLoadedContentId, currentLoadedHasFeedback) and deduplicate in-flight requests while the source is in Pending/PREPARING. This prevents rapid duplicate calls (such as setting the language code followed by loadMainContentAudio()) from restarting an in-flight fetch, while preserving explicit retry behavior when reloadingMainContent == true or when recovering from AsyncResult.Failure.
|
On further thought, #6446 (comment) describes a much larger refactor, so I think it is best to keep the current PR scope as fixing part of the issue.
|
|
Hi @Kishan8548, I'm going to mark this PR as stale because it hasn't had any updates for 7 days. If no further activity occurs within 7 days, it will be automatically closed so that others can take up the issue. |
oppiabot
Bot
added
the
stale
|
@Kishan8548 PTAL at adhiambo's comments. |
oppiabot
Bot
removed
the
stale
|
@Neer-rn I am a little busy with my college exams, i'll work on the requested comments in some days. |
Kishan8548
changed the title
Kishan8548
changed the title
…und position polling
|
@Neer-rn PTAL, addressed all requested changes, one unit test timed out due to runner cancellation. |
|
Unassigning @Kishan8548 since a re-review was requested. @Kishan8548, please make sure you have addressed all review comments. Thanks! |
Explanation
Fixes part of #6446
Fixes input lock contention and ANR issues in
AudioFragmentandAudioPlayerController.Root Cause
MediaPlayer.reset()andMediaPlayer.release()were executed on the main UI thread. During network I/O, Android's internalMediaHTTPConnectionbinder thread holds internal locks, causing the main thread to block and trigger ANRs.AudioViewModel.loadAudio()was re-triggered on every ephemeral state compute even if the requested voiceover audio URI was already loaded, causing redundant resets and re-loads.Solution
currentLoadedAudioUrito short-circuit redundantloadAudio()invocations when the target URI is already active.@BackgroundDispatcherto offload blockingMediaPlayeroperations (reset(),prepareDataSource(), andrelease()) via Kotlin coroutines.Mutexto serializeMediaPlayerlifecycle operations in background jobs without blocking the main UI thread.audioLock(ReentrantLock) strictly to fast in-memory state mutations so UI thread calls (pause(),play(),seekTo()) are never blocked waiting on I/O.activeLoadJobto cancel stale in-flight loads upon subsequent operations.ShadowMediaPlayerevents with coroutines usingtestCoroutineDispatchers.runCurrent().AudioPlayerControllerTestfor rapid sequential loads and mid-load teardowns.AudioFragmentTestto verify that repeated state updates with the same audio source keep the audio playing without reloading.Essential Checklist
Disclosure of LLM Usage