Skip to content

fix: crashes on audio message playback [WPB-16195] 🍒 #3923

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Mar 20, 2025

Conversation

github-actions[bot]
Copy link
Contributor

This PR was automatically cherry-picked based on the following PR:

Original PR description:


TaskWPB-16195 [Android]Audio not available or app crashes on playback


PR Submission Checklist for internal contributors

  • The PR Title

    • conforms to the style of semantic commits messages¹ supported in Wire's Github Workflow²
    • contains a reference JIRA issue number like SQPIT-764
    • answers the question: If merged, this PR will: ... ³
  • The PR Description

    • is free of optional paragraphs and you have filled the relevant parts to the best of your ability

What's new in this PR?

Issues

The app crashes as soon as the user taps the play button on audio message.

Causes (Optional)

Race condition when fetching audio asset file - it can be executed multiple times at once as it's also called to fetch waves mask and then asset file can be overwritten and broken (created but empty for instance) resulting in a crash when trying to play empty file.
The "fetch waves mask" action can also be called multiple times for each message as it's called every time paging data is updated. The whole map of audio states is propagated through all composables all way down from the screen to the AudioMessage item and it's not stable so it can be also prone to more unneeded recompositions.
ConversationAudioMessagePlayer dropping and losing some of the crucial states emitted because of DROP_OLDEST and for instance making the audio message be played without any indication on the screen.

Solutions

Secured getAssetMessage by keeping the deferred until it's completed so that each execution during that time will reuse the same deferred and it won't be called multiple times at the same time - no issues with overwriting the file and no unnecessary requests.
Created dedicated scoped AudioMessageViewModel which handles the state for the given message and executes calls to play the audio associated with that message, change the audio speed or fetch waves mask for that audio so that it happens only once and is kept in the view model. Thanks to that, a map with all audio states doesn't need to be passed all the way to the AudioMessage but instead it uses state from injected scoped AudioMessageViewModel.
Replaced DROP_OLDEST with SUSPEND in ConversationAudioMessagePlayer to not lose any state updates.

Testing

Test Coverage (Optional)

  • I have added automated test to this contribution

How to Test

Receive audio messages and try to play them.


PR Post Submission Checklist for internal contributors (Optional)

  • Wire's Github Workflow has automatically linked the PR to a JIRA issue

PR Post Merge Checklist for internal contributors

  • If any soft of configuration variable was introduced by this PR, it has been added to the relevant documents and the CI jobs have been updated.

References
  1. https://sparkbox.com/foundry/semantic_commit_messages
  2. https://github.com/wireapp/.github#usage
  3. E.g. feat(conversation-list): Sort conversations by most emojis in the title #SQPIT-764.

@github-actions github-actions bot added cherry-pick PR is cherry-picking changes from another banch size/L labels Mar 19, 2025
Copy link

@codecov-commenter
Copy link

Codecov Report

Attention: Patch coverage is 88.88889% with 8 lines in your changes missing coverage. Please review.

Project coverage is 45.75%. Comparing base (c8b67c8) to head (b1de80c).
Report is 1 commits behind head on develop.

Files with missing lines Patch % Lines
...ndroid/media/audiomessage/AudioMessageViewModel.kt 89.74% 4 Missing ⚠️
...dia/audiomessage/ConversationAudioMessagePlayer.kt 88.88% 2 Missing and 1 partial ⚠️
...d/ui/home/conversations/media/FileAssetsContent.kt 0.00% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #3923      +/-   ##
===========================================
+ Coverage    45.59%   45.75%   +0.15%     
===========================================
  Files          492      493       +1     
  Lines        17091    17110      +19     
  Branches      2852     2852              
===========================================
+ Hits          7793     7828      +35     
+ Misses        8506     8489      -17     
- Partials       792      793       +1     
Files with missing lines Coverage Δ
...rsations/messages/ConversationMessagesViewModel.kt 68.94% <100.00%> (+0.19%) ⬆️
...rsations/messages/ConversationMessagesViewState.kt 100.00% <100.00%> (ø)
...conversations/messages/item/MessageClickActions.kt 0.00% <ø> (ø)
.../conversations/messages/item/RegularMessageItem.kt 0.00% <ø> (ø)
...tions/model/messagetypes/audio/AudioMessageType.kt 0.00% <ø> (ø)
...d/ui/home/conversations/media/FileAssetsContent.kt 0.00% <0.00%> (ø)
...dia/audiomessage/ConversationAudioMessagePlayer.kt 65.66% <88.88%> (+4.48%) ⬆️
...ndroid/media/audiomessage/AudioMessageViewModel.kt 89.74% <89.74%> (ø)

... and 2 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c8b67c8...b1de80c. Read the comment docs.

🚀 New features to boost your workflow:
  • Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Contributor Author

Built wire-android-staging-compat-pr-3923.apk is available for download

Copy link
Contributor Author

Built wire-android-dev-debug-pr-3923.apk is available for download

@saleniuk saleniuk requested review from a team, sbakhtiarov, yamilmedina, MohamadJaara, vitorhugods and saleniuk and removed request for a team March 19, 2025 16:28
@saleniuk saleniuk added this pull request to the merge queue Mar 20, 2025
Merged via the queue into develop with commit 48277d7 Mar 20, 2025
11 of 12 checks passed
@saleniuk saleniuk deleted the fix/audio-message-playback-crashing-cherry-pick branch March 20, 2025 12:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cherry-pick PR is cherry-picking changes from another banch size/L
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants