Refactor audio recording capabilities and enhance playback management - #176
Open
Dimowner wants to merge 13 commits into
Open
Refactor audio recording capabilities and enhance playback management#176Dimowner wants to merge 13 commits into
Dimowner wants to merge 13 commits into
Conversation
…itrate limits and enhance audio recording settings
…itrate limits and enhance audio recording settings
…rder. Refactor audio recording capabilities: remove DeviceRecordingCapabilities and related bitrate limit management.
…r smoother UI updates and resetting anchors on state changes. Update Android Gradle Plugin to version 9.4.0.
There was a problem hiding this comment.
🟡 Changes recommended
Critical recording lifecycle races and additional playback and codec-selection defects remain unresolved.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Introduces MediaCodec-based M4A recording with fallback support, smoother playback-position reporting, expanded tests, and dependency/version updates.
Changes:
- Adds AAC encoding, bitrate handling, fallback routing, and duration limits.
- Interpolates ExoPlayer playback progress.
- Adds recorder tests and updates build dependencies and app versioning.
File summaries
| File | Review |
|---|---|
gradle/wrapper/gradle-wrapper.properties |
Upgrades Gradle. |
gradle/libs.versions.toml |
Updates Android, Compose, KSP, and Media3 dependencies. |
app/src/test/java/com/dimowner/audiorecorder/v2/audio/M4aRecorderV2Test.kt |
Tests backend selection and fallback; contains a non-functional assertion (nit). |
app/src/test/java/com/dimowner/audiorecorder/v2/audio/AacCodecRecorderHelpersTest.kt |
Tests AAC timing, duration, and bitrate helpers. |
app/src/main/java/com/dimowner/audiorecorder/v2/data/model/FormatConfig.kt |
Formatting cleanup. |
app/src/main/java/com/dimowner/audiorecorder/v2/audio/MediaRecorderBase.kt |
Moves maximum-duration enforcement into recorder progress logic. |
app/src/main/java/com/dimowner/audiorecorder/v2/audio/M4aRecorderV2.kt |
Adds MediaRecorder fallback; a critical race can start fallback after recording was stopped. |
app/src/main/java/com/dimowner/audiorecorder/v2/audio/AudioRecordingService.kt |
Formatting cleanup. |
app/src/main/java/com/dimowner/audiorecorder/v2/audio/AudioRecorderDelegate.kt |
Routes M4A requests through the new recorder wrapper. |
app/src/main/java/com/dimowner/audiorecorder/v2/audio/AacCodecRecorderV2.kt |
Implements AAC recording; unresolved issues affect event ordering, startup fallback, and codec-specific bitrate selection, including one critical issue. |
app/src/main/java/com/dimowner/audiorecorder/v2/app/settings/SettingsExtensions.kt |
Formatting cleanup. |
app/src/main/java/com/dimowner/audiorecorder/audio/player/ExoAudioPlayer.kt |
Adds interpolated playback progress; speed changes can cause jumps or freezes. |
app/src/main/AndroidManifest.xml |
Removes an obsolete commented permission. |
app/src/androidTest/java/com/dimowner/audiorecorder/v2/audio/AacCodecRecorderInstrumentedTest.kt |
Adds device tests for the AAC recording pipeline. |
app/build.gradle.kts |
Bumps the app version to 2.4.1 (950). |
Review details
Suppressed comments (1)
app/src/test/java/com/dimowner/audiorecorder/v2/audio/M4aRecorderV2Test.kt:125
- This assertion cannot fail because
failedConfigis initialized to an empty string and is never assigned. It therefore does not verify recorder state or failure-cache clearing; remove it and its field, or replace it with an assertion against observable recorder behavior.
assertEquals("a successful start clears the remembered failure", "", failedConfig)
- Files reviewed: 15/15 changed files
- Comments generated: 5
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+555
to
+556
| emitEvent(RecorderEvent.OnStopRecording) | ||
| emitEvent(RecorderEvent.OnError(RecordingException())) |
Comment on lines
+153
to
+155
| coroutineScope.launch { | ||
| Timber.w("MediaCodec pipeline produced no audio ($reason), falling back to MediaRecorder") | ||
| startWithMediaRecorder(params) |
| anchorRealtimeMills = now | ||
| } | ||
| if (!isRendering) return raw | ||
| val interpolated = anchorPosMills + ((now - anchorRealtimeMills) * playbackSpeed).toLong() |
Comment on lines
+321
to
+325
| MediaCodecList(MediaCodecList.REGULAR_CODECS).codecInfos | ||
| .filter { info -> | ||
| info.isEncoder && info.supportedTypes.any { it.equals(MediaFormat.MIMETYPE_AUDIO_AAC, true) } | ||
| } | ||
| .mapNotNull { info -> |
Comment on lines
+547
to
+551
| session.muxedFrameCount == 0L -> { | ||
| // Nothing was ever written: the file is an unusable stub, so let the service drop | ||
| // the empty record along with it. | ||
| runCatching { outputFile.delete() } | ||
| emitEvent(RecorderEvent.OnError(RecorderInitException())) |
- Introduced `AudioInput` sealed interface to differentiate between microphone and system audio sources. - Updated recording methods across various recorder classes to accept `AudioInput` instead of raw audio source integers. - Implemented feature flags to manage system audio capture availability. - Enhanced UI to reflect system audio as a selectable option when supported. - Added necessary permissions and error handling for system audio recording.
…StopFailedException
…amplitude updates
…nt crashes and ensure proper resource release
…e resource management during recording stops
There was a problem hiding this comment.
🟡 Changes recommended
Critical recording-loop, finalization, and stop/start race defects remain unresolved.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (5)
app/src/main/java/com/dimowner/audiorecorder/v2/audio/AacCodecRecorderV2.kt:565
- These two events are launched from separate coroutines by
emitEvent, so their collection order is not guaranteed. IfOnErrorwins, the service clearsrecordedRecordIdbeforeOnStopRecordingcan persist the captured file, contradicting the required “stop first” ordering. Emit both sequentially from one coroutine.
emitEvent(RecorderEvent.OnStopRecording)
emitEvent(RecorderEvent.OnError(RecordingException()))
app/src/main/java/com/dimowner/audiorecorder/v2/audio/AacCodecRecorderV2.kt:336
- This computes the maximum across all AAC encoders, but
createEncoderByType()later opens the platform-preferred encoder, which may have a lower limit than a different encoder in this list. On devices with multiple AAC encoders, a valid high-bitrate codec can therefore be ignored: configuration of the preferred codec fails and the wrapper falls back to bitrate-clippingMediaRecorder. Select a compatible codec for the complete format and instantiate that codec by name, or clamp against the capabilities of the codec actually selected.
MediaCodecList(MediaCodecList.REGULAR_CODECS).codecInfos
.filter { info ->
info.isEncoder && info.supportedTypes.any { it.equals(MediaFormat.MIMETYPE_AUDIO_AAC, true) }
}
.mapNotNull { info ->
info.getCapabilitiesForType(MediaFormat.MIMETYPE_AUDIO_AAC).audioCapabilities?.bitrateRange?.upper
}
.maxOrNull() ?: Int.MAX_VALUE
app/src/main/java/com/dimowner/audiorecorder/v2/audio/AudioRecordingService.kt:487
- Missing projection consent is silently converted to the default microphone here. The static recording shortcut still calls
startServiceForeground()without projection data (shortcuts.xml:10-13→MainActivity.java:562-564), so users who selected System Audio will unknowingly record their microphone. Treat a missing/invalid projection as a failed system-audio start and route every entry point through the consent flow instead of changing capture sources.
if (projection == null) {
Timber.w("System audio was selected but no MediaProjection was granted; using the mic")
return AudioInput.Mic(DefaultValues.DefaultAudioSource.value)
app/src/main/res/values/strings.xml:295
- Every localized resource set defines its own
info_audio_source_html(for example,values-de/strings.xml:289-294andvalues-fr/strings.xml:267-272), so this new paragraph is never shown outside the default locale even though the System Audio option itself is translated. Add the corresponding explanation to those localized HTML resources as well.
app/src/test/java/com/dimowner/audiorecorder/v2/audio/M4aRecorderV2Test.kt:128 failedConfigis never written by the recorder or the test, so this assertion always compares the field's initial empty string with an empty string and cannot detect a regression. Remove it or assert observable recorder state/calls that actually prove the prior failure was cleared.
- Files reviewed: 55/55 changed files
- Comments generated: 6
- Review effort level: Balanced
Comment on lines
+381
to
+388
| read == AudioRecord.ERROR_INVALID_OPERATION -> { | ||
| Timber.e("AudioRecord read error: ERROR_INVALID_OPERATION") | ||
| break | ||
| } | ||
| read == AudioRecord.ERROR_BAD_VALUE -> { | ||
| Timber.e("AudioRecord read error: ERROR_BAD_VALUE") | ||
| break | ||
| } |
Comment on lines
+668
to
+670
| if (muxerStarted) activeMuxer?.stop() | ||
| } catch (e: IllegalStateException) { | ||
| Timber.e(e, "muxer.stop() problems") |
Comment on lines
+320
to
+323
| * Stops and releases the [recorder] this recording coroutine owns. Invoked from the | ||
| * coroutine's teardown. Releases the passed instance (not the field) so a rapid stop->start | ||
| * that has already swapped in a new [AudioRecord] is not torn down by the previous run; the | ||
| * field is only cleared if it still points at this recorder. |
Comment on lines
+126
to
+129
| } catch (e: ActivityNotFoundException) { | ||
| Timber.e(e, "No activity handles the screen capture request") | ||
| projectionDenied = true | ||
| } |
Comment on lines
+480
to
+484
| private fun resolveAudioInput(): AudioInput { | ||
| if (!isSystemAudioSelected()) { | ||
| return AudioInput.Mic(prefs.settingAudioSource.value) | ||
| } | ||
| val projection = mediaProjection |
| stopFailure = e | ||
| } finally { | ||
| // Always release resources | ||
| releaseRecorder(recorder) |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This pull request introduces support for recording system audio via MediaProjection on Android 14+ and adds comprehensive instrumentation tests for the new AAC recording pipeline. It also updates the app's permissions and foreground service declarations to support system audio capture, and improves the user consent flow for system audio recording. Additionally, analytics hooks are prepared for playback failure reporting, and the app version is incremented.
System Audio Recording Support:
AndroidManifest.xmlto declare theFOREGROUND_SERVICE_MEDIA_PROJECTIONpermission and to allowAudioRecordingServiceto use bothmicrophoneandmediaProjectionforeground service types. This enables recording of system audio on Android 14+ devices. [1] [2]TransparentRecordingActivityto handle user consent for system audio capture using MediaProjection, including managing the consent dialog, result handling, and passing the projection data to the recording service. [1] [2] [3]Testing Enhancements:
AacCodecRecorderInstrumentedTest.ktto verify that the new AAC recording pipeline records at the requested bitrate, handles edge cases (like pausing, max duration, metadata, and interrupted recordings), and gracefully manages unsupported bitrates.Analytics Preparation:
ExoAudioPlayerto accept anAnalyticsTrackerand imported analytics constants and failure types, preparing for more detailed playback failure reporting. [1] [2]Other Updates:
AndroidManifest.xml(removal of commented-outINTERNETpermission).