Add background audio playback for voice messages#104
Add background audio playback for voice messages#104
Conversation
Make notification clickable
…ges; Prevent unrelated activities from changing the pending intent
… avoid overaapping with system bar
Stop playback when message containing audio is removed
…annel Make device messages and subscribed channels edge to edge
…into adb/test-bg-audio
|
To test the changes in this pull request, install this apk: |
There was a problem hiding this comment.
Pull request overview
This PR introduces app-wide/background-capable audio playback (voice messages and other audio attachments) by migrating from the old per-view AudioSlidePlayer approach to a Media3 MediaSessionService + shared ViewModel architecture, and wires it through conversation and media gallery UIs.
Changes:
- Add
AudioPlaybackService(Media3MediaSessionService) andAudioPlaybackViewModel/AudioPlaybackState, plus a newcomponents.audioplay.AudioView. - Replace layout/component references from the old
components.AudioViewto the newcomponents.audioplay.AudioViewacross conversation items, attachment editor, and media gallery. - Update window-insets handling helpers and conversation UI padding/divider behavior; update call-type strings (audio vs video).
Reviewed changes
Copilot reviewed 32 out of 33 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
src/main/java/org/thoughtcrime/securesms/service/AudioPlaybackService.java |
New Media3 session service backing background playback and notification/session integration. |
src/main/java/org/thoughtcrime/securesms/components/audioplay/AudioView.java |
New audio message UI component wired to the playback ViewModel. |
src/main/java/org/thoughtcrime/securesms/components/audioplay/AudioPlaybackViewModel.java |
Shared playback state + MediaController interaction and progress polling. |
src/main/java/org/thoughtcrime/securesms/components/audioplay/AudioPlaybackState.java |
Playback state model communicated to views via LiveData. |
src/main/java/org/thoughtcrime/securesms/ConversationActivity.java |
Connects to AudioPlaybackService, owns controller lifecycle, forwards “current activity context” on play/pause. |
src/main/java/org/thoughtcrime/securesms/ConversationFragment.java |
Injects playback ViewModel into adapter; adds bottom divider + force-inset paths. |
src/main/java/org/thoughtcrime/securesms/ConversationAdapter.java |
Plumbs playback ViewModel + audio action listener into bind calls. |
src/main/java/org/thoughtcrime/securesms/ConversationItem.java |
Uses new AudioView + ViewModel instead of requesting duration via old player. |
src/main/java/org/thoughtcrime/securesms/mms/AttachmentManager.java |
Wires draft/attachment audio playback to the shared ViewModel. |
src/main/java/org/thoughtcrime/securesms/components/RemovableEditableMediaView.java |
Changes remove button handling from a single listener to a listener list. |
src/main/java/org/thoughtcrime/securesms/AllMediaActivity.java |
Adds MediaController hookup for global gallery audio playback. |
src/main/java/org/thoughtcrime/securesms/AllMediaDocumentsFragment.java / AllMediaDocumentsAdapter.java |
Inject playback ViewModel and bind audio items to new AudioView. |
src/main/java/org/thoughtcrime/securesms/util/ViewUtil.java |
Adds “force dispatch” variants for insets application and new overloads. |
src/main/AndroidManifest.xml |
Adds foreground-service permission and registers AudioPlaybackService. |
build.gradle |
Enables core library desugaring; adds Media3 dependencies. |
src/main/res/layout/*.xml |
Switches XML widgets to new AudioView class; adds bottom divider id; updates audio_view layout. |
src/main/res/values/strings.xml / DcHelper.java / CallItemView.java |
Adds audio/video call strings and uses them in call UI / stock translations. |
CHANGELOG-upstream.md |
Notes background playback feature. |
Deleted: components/AudioView.java, audio/AudioSlidePlayer.java |
Removes old audio playback implementation. |
| if (isInitialization) { | ||
| ViewUtil.forceApplyWindowInsets(findViewById(R.id.root_layout), true, false, true, false); | ||
| fragment.handleAddBottomInsets(); | ||
| } |
There was a problem hiding this comment.
setComposePanelVisibility(false) (used after chat state changes) does not adjust window insets or call fragment.handleAddBottomInsets() when dcChat.canSend() becomes false, because that logic is gated behind isInitialization. This can leave the list padding/divider in the wrong state after runtime changes. Consider always updating insets/divider when the sendability state changes, not only during initialization.
| if (isInitialization) { | |
| ViewUtil.forceApplyWindowInsets(findViewById(R.id.root_layout), true, false, true, false); | |
| fragment.handleAddBottomInsets(); | |
| } | |
| ViewUtil.forceApplyWindowInsets(findViewById(R.id.root_layout), true, false, true, false); | |
| fragment.handleAddBottomInsets(); |
| playPauseButton.setOnClickListener(v -> { | ||
| Log.w(TAG, "playPauseButton onClick"); | ||
|
|
||
| if (viewModel == null || audioUri == null) return; |
There was a problem hiding this comment.
playPauseButton click logs at WARN level on every tap. This is likely too noisy for production logs and may expose usage patterns. Consider removing it or lowering to DEBUG behind a build flag.
| public void onStartTrackingTouch(SeekBar seekBar) { | ||
| viewModel.setUserSeeking(true); | ||
| } | ||
|
|
||
| @Override | ||
| public void onStopTrackingTouch(SeekBar seekBar) { |
There was a problem hiding this comment.
onStartTrackingTouch() / onStopTrackingTouch() call viewModel.* without a null check, while the play/pause handler explicitly guards viewModel == null. If this view is ever used without calling setPlaybackViewModel() first (or after it’s cleared), dragging the seek bar will crash. Add the same null/URI guard in the seek bar callbacks or disable the seek bar until a ViewModel is set.
| public void onStartTrackingTouch(SeekBar seekBar) { | |
| viewModel.setUserSeeking(true); | |
| } | |
| @Override | |
| public void onStopTrackingTouch(SeekBar seekBar) { | |
| public void onStartTrackingTouch(SeekBar seekBar) { | |
| if (viewModel == null || audioUri == null) { | |
| return; | |
| } | |
| viewModel.setUserSeeking(true); | |
| } | |
| @Override | |
| public void onStopTrackingTouch(SeekBar seekBar) { | |
| if (viewModel == null || audioUri == null) { | |
| return; | |
| } |
| <service | ||
| android:name=".service.AudioPlaybackService" | ||
| android:foregroundServiceType="mediaPlayback" | ||
| android:exported="true"> | ||
| <intent-filter> | ||
| <action android:name="androidx.media3.session.MediaSessionService"/> | ||
| </intent-filter> | ||
| </service> |
There was a problem hiding this comment.
AudioPlaybackService is declared android:exported="true", which allows any other app to connect as a Media3 controller. Given the current onConnect implementation accepts all controllers and exposes a custom command, this becomes a security boundary issue. If external control isn't required, set exported="false"; otherwise explicitly restrict allowed controllers (e.g., same package UID / system UI) and only expose custom commands to trusted callers.
| public MediaSession.ConnectionResult onConnect( | ||
| MediaSession session, | ||
| MediaSession.ControllerInfo controller | ||
| ) { | ||
| SessionCommands sessionCommands = MediaSession | ||
| .ConnectionResult.DEFAULT_SESSION_COMMANDS.buildUpon() | ||
| .add(new SessionCommand("UPDATE_ACTIVITY_CONTEXT", new Bundle())) | ||
| .build(); | ||
|
|
||
| return new MediaSession.ConnectionResult.AcceptedResultBuilder(session) | ||
| .setAvailableSessionCommands(sessionCommands) | ||
| .build(); | ||
| } |
There was a problem hiding this comment.
onConnect() currently accepts all controllers and adds a custom command that can mutate the session activity. If the service stays exported, please validate controller (package/UID/signature) and only accept connections from trusted callers; otherwise untrusted apps can control playback and invoke custom commands.
| private void updateSessionActivity(Bundle args) { | ||
| try { | ||
| // Put all the original extras back into the intent | ||
| if (args != null && !args.isEmpty()) { | ||
| String activityClassName = args.getString("activity_class"); | ||
| args.remove("activity_class"); | ||
|
|
||
| if (activityClassName != null) { | ||
| Class<?> activityClass = Class.forName(activityClassName); | ||
| Intent intent = new Intent(this, activityClass); | ||
| intent.setFlags(Intent.FLAG_ACTIVITY_CLEAR_TOP | Intent.FLAG_ACTIVITY_SINGLE_TOP); | ||
| intent.putExtras(args); | ||
|
|
||
| PendingIntent pendingIntent = PendingIntent.getActivity( | ||
| this, | ||
| 0, | ||
| intent, | ||
| PendingIntent.FLAG_UPDATE_CURRENT | PendingIntent.FLAG_IMMUTABLE | ||
| ); | ||
|
|
||
| session.setSessionActivity(pendingIntent); | ||
| } |
There was a problem hiding this comment.
updateSessionActivity() reflects an arbitrary class name from a Bundle sent via a custom command and then uses it to build a PendingIntent. With an exported service, this can be abused to launch internal activities with attacker-controlled extras (or to DoS via bad bundles). Prefer an allowlist of known activities (or ignore the class name entirely) and reject the custom command from untrusted controllers.
|
|
||
| audioView.setPlaybackViewModel(playbackViewModel); | ||
| audioView.setAudio((AudioSlide) slide, 0); | ||
| removableMediaView.display(audioView, false); |
There was a problem hiding this comment.
removableMediaView.addRemoveClickListener(...) is called every time an audio attachment is set, but these listeners are never removed/cleared. If the user changes attachments multiple times, the list will grow and the remove button will trigger multiple callbacks (and keep references alive). Consider replacing the listener (single listener API) or clearing previous listeners before adding a new one.
| removableMediaView.display(audioView, false); | |
| removableMediaView.display(audioView, false); | |
| removableMediaView.clearRemoveClickListeners(); |
| public void setMediaController(@Nullable MediaController controller) { | ||
| this.mediaController = controller; | ||
| if (mediaController != null && mediaController.isPlaying()) { | ||
| startUpdateProgress(); | ||
| } | ||
| updateCurrentState(true); | ||
| setupPlayerListener(); | ||
| } | ||
|
|
||
| // Public methods | ||
| public void loadAudioAndPlay(int msgId, Uri audioUri) { | ||
| if (mediaController == null) return; | ||
|
|
||
| // Set media item if we have a different audio. | ||
| if (isDifferentAudio(msgId, audioUri)) { | ||
| updateState(msgId, audioUri, AudioPlaybackState.PlaybackStatus.LOADING, 0, 0); | ||
|
|
||
| MediaItem mediaItem = new MediaItem.Builder() | ||
| .setMediaId(String.valueOf(msgId)) | ||
| .setUri(audioUri) | ||
| .build(); | ||
| mediaController.setMediaItem(mediaItem); | ||
| mediaController.prepare(); | ||
| } | ||
|
|
||
| play(msgId, audioUri); | ||
| } | ||
| private boolean isSameAudio(int msgId, Uri audioUri) { | ||
| return !isDifferentAudio(msgId, audioUri); | ||
| } | ||
|
|
||
| private boolean isDifferentAudio(int msgId, Uri audioUri) { | ||
| AudioPlaybackState currentState = playbackState.getValue(); | ||
|
|
||
| return currentState != null && ( | ||
| msgId != currentState.getMsgId() || | ||
| currentState.getAudioUri() == null || | ||
| currentState.getAudioUri() != null && !currentState.getAudioUri().equals(audioUri)); | ||
| } | ||
|
|
||
| public void pause(int msgId, Uri audioUri) { | ||
| if (mediaController != null && isSameAudio(msgId, audioUri)) { | ||
| mediaController.pause(); | ||
| } | ||
| } | ||
|
|
||
| public void play(int msgId, Uri audioUri) { | ||
| if (mediaController != null && isSameAudio(msgId, audioUri)) { | ||
| mediaController.play(); | ||
| } | ||
| } | ||
|
|
||
| public void seekTo(long position, int msgId, Uri audioUri) { | ||
| if (mediaController != null && isSameAudio(msgId, audioUri)) { | ||
| mediaController.seekTo(position); | ||
| } | ||
| } | ||
|
|
||
| public void stop(int msgId, Uri audioUri) { | ||
| if (mediaController != null && isSameAudio(msgId, audioUri)) { | ||
| mediaController.stop(); | ||
| stopUpdateProgress(); | ||
| playbackState.setValue(AudioPlaybackState.idle()); | ||
| } | ||
| } | ||
|
|
||
| public void stopNonMessageAudioPlayback() { | ||
| stopByIds(NON_MESSAGE_AUDIO_MSG_ID); | ||
| } | ||
|
|
||
| // A special method for deleting message, where we only use message Ids | ||
| public void stopByIds(int... msgIds) { | ||
| AudioPlaybackState currentState = playbackState.getValue(); | ||
|
|
||
| if (mediaController != null && currentState != null) { | ||
| for (int msgId : msgIds) { | ||
| if (msgId == currentState.getMsgId()) { | ||
| mediaController.stop(); | ||
| stopUpdateProgress(); | ||
| playbackState.setValue(AudioPlaybackState.idle()); | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| public void setUserSeeking(boolean isUserSeeking) { | ||
| this.isUserSeeking = isUserSeeking; | ||
| } | ||
|
|
||
| // Private methods | ||
| private void setupPlayerListener() { | ||
| if (mediaController == null) return; | ||
|
|
||
| mediaController.addListener(new Player.Listener() { | ||
| @Override | ||
| public void onEvents(Player player, Player.Events events) { | ||
| if (events.containsAny(Player.EVENT_IS_PLAYING_CHANGED)) { | ||
| if (player.isPlaying()) { | ||
| startUpdateProgress(); | ||
| } else { | ||
| stopUpdateProgress(); | ||
| } | ||
| updateCurrentState(false); | ||
| } | ||
| if (events.containsAny(Player.EVENT_PLAYBACK_STATE_CHANGED)) { | ||
| if (player.getPlaybackState() == Player.STATE_READY) { | ||
| updateCurrentState(false); | ||
| } else if (player.getPlaybackState() == Player.STATE_ENDED) { | ||
| // This is to prevent automatically playing after the audio | ||
| // has been play to the end once, then user dragged the seek bar again | ||
| mediaController.setPlayWhenReady(false); | ||
| } | ||
| } | ||
| if (events.containsAny(Player.EVENT_PLAYER_ERROR)) { | ||
| updateCurrentAudioState(AudioPlaybackState.PlaybackStatus.ERROR, 0, 0); | ||
| } | ||
| } | ||
| }); | ||
| } |
There was a problem hiding this comment.
setMediaController() calls setupPlayerListener() every time but never removes the previously registered Player.Listener when replacing/clearing the controller. This can lead to duplicate callbacks and leaks if the controller is recreated. Keep a reference to the listener and remove it from the old controller in setMediaController() (and/or when setting controller to null).
| private void updateRemoveClickListener() { | ||
| this.remove.setOnClickListener(v -> { | ||
| for (OnClickListener listener : removeClickListeners) { | ||
| listener.onClick(v); | ||
| } | ||
| }); |
There was a problem hiding this comment.
This new multi-listener implementation has no way to clear/reset listeners, so call sites that add per-item listeners (e.g., attachment audio remove handling) can easily leak/duplicate callbacks. Consider adding a clearRemoveClickListeners() (or reintroduce a setRemoveClickListener() that replaces existing listeners) to prevent unbounded listener growth.
| private void updateRemoveClickListener() { | |
| this.remove.setOnClickListener(v -> { | |
| for (OnClickListener listener : removeClickListeners) { | |
| listener.onClick(v); | |
| } | |
| }); | |
| public void clearRemoveClickListeners() { | |
| removeClickListeners.clear(); | |
| updateRemoveClickListener(); | |
| } | |
| private void updateRemoveClickListener() { | |
| if (removeClickListeners.isEmpty()) { | |
| this.remove.setOnClickListener(null); | |
| } else { | |
| this.remove.setOnClickListener(v -> { | |
| for (OnClickListener listener : removeClickListeners) { | |
| listener.onClick(v); | |
| } | |
| }); | |
| } |
| ViewUtil.forceApplyWindowInsets(list, false, true, false, true); | ||
| ViewUtil.forceApplyWindowInsetsAsMargin(scrollToBottomButton, true, true, true, true); | ||
| pendingAddBottomInsets = false; | ||
| } | ||
|
|
||
| if (pendingRemoveBottomInsets) { | ||
| bottomDivider.setVisibility(View.VISIBLE); | ||
| ViewUtil.forceApplyWindowInsets(list, false, true, false, false); | ||
| ViewUtil.forceApplyWindowInsetsAsMargin(scrollToBottomButton, true, true, true, false); |
There was a problem hiding this comment.
The pendingAddBottomInsets / pendingRemoveBottomInsets blocks apply insets/margins with different side flags than handleAddBottomInsets() / handleRemoveBottomInsets() (e.g., forceApplyWindowInsetsAsMargin(scrollToBottomButton, true, true, true, true) vs only bottom). This can lead to inconsistent layout depending on whether the call happens before or after view inflation. Align the flags so both code paths apply the same insets.
| ViewUtil.forceApplyWindowInsets(list, false, true, false, true); | |
| ViewUtil.forceApplyWindowInsetsAsMargin(scrollToBottomButton, true, true, true, true); | |
| pendingAddBottomInsets = false; | |
| } | |
| if (pendingRemoveBottomInsets) { | |
| bottomDivider.setVisibility(View.VISIBLE); | |
| ViewUtil.forceApplyWindowInsets(list, false, true, false, false); | |
| ViewUtil.forceApplyWindowInsetsAsMargin(scrollToBottomButton, true, true, true, false); | |
| ViewUtil.forceApplyWindowInsets(list, false, false, false, true); | |
| ViewUtil.forceApplyWindowInsetsAsMargin(scrollToBottomButton, false, false, false, true); | |
| pendingAddBottomInsets = false; | |
| } | |
| if (pendingRemoveBottomInsets) { | |
| bottomDivider.setVisibility(View.VISIBLE); | |
| ViewUtil.forceApplyWindowInsets(list, false, false, false, false); | |
| ViewUtil.forceApplyWindowInsetsAsMargin(scrollToBottomButton, false, false, false, false); |
No description provided.