Skip to content

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

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

Closed
wants to merge 21 commits into from

Conversation

saleniuk
Copy link
Contributor

@saleniuk saleniuk commented Mar 12, 2025

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.

@codecov-commenter
Copy link

codecov-commenter commented Mar 12, 2025

Codecov Report

Attention: Patch coverage is 85.88235% with 12 lines in your changes missing coverage. Please review.

Project coverage is 45.66%. Comparing base (0a20227) to head (4f72e67).

Files with missing lines Patch % Lines
...rc/main/kotlin/com/wire/android/WireApplication.kt 0.00% 4 Missing ⚠️
...ndroid/media/audiomessage/AudioMessageViewModel.kt 89.74% 4 Missing ⚠️
...dia/audiomessage/ConversationAudioMessagePlayer.kt 91.66% 2 Missing ⚠️
...analytics/ObserveCurrentSessionAnalyticsUseCase.kt 92.30% 0 Missing and 1 partial ⚠️
...d/ui/home/conversations/media/FileAssetsContent.kt 0.00% 1 Missing ⚠️
Additional details and impacted files
@@                  Coverage Diff                  @@
##           release/candidate    #3912      +/-   ##
=====================================================
+ Coverage              45.59%   45.66%   +0.07%     
=====================================================
  Files                    491      492       +1     
  Lines                  17078    17085       +7     
  Branches                2845     2845              
=====================================================
+ Hits                    7786     7802      +16     
+ Misses                  8500     8490      -10     
- 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% <ø> (ø)
...analytics/ObserveCurrentSessionAnalyticsUseCase.kt 83.33% <92.30%> (-5.56%) ⬇️
...d/ui/home/conversations/media/FileAssetsContent.kt 0.00% <0.00%> (ø)
...dia/audiomessage/ConversationAudioMessagePlayer.kt 63.35% <91.66%> (+2.18%) ⬆️
...rc/main/kotlin/com/wire/android/WireApplication.kt 0.00% <0.00%> (ø)
...ndroid/media/audiomessage/AudioMessageViewModel.kt 89.74% <89.74%> (ø)

... and 1 file 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 0a20227...4f72e67. Read the comment docs.

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

@saleniuk saleniuk requested review from a team, sbakhtiarov, yamilmedina, alexandreferris, Garzas and ohassine and removed request for a team March 12, 2025 16:11
Copy link
Contributor

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

Copy link
Contributor

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

Copy link
Member

@vitorhugods vitorhugods left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Really nice improvement 🚀

Copy link

Ups 🫰🟨

This PR is too big. Please try to break it up into smaller PRs.

Copy link
Contributor

@saleniuk looks like you are rolling back kalium to a previous commitish.

This means that the PR's target branch (develop) is using a newer version of Kalium, and the changes in this PR will rollback Kalium to an older version.

develop This PR
1fb0f78f3758056cbe74d8debd8ad17cef76d5f3 62b50d50bd060af4840de2030330c4e87c659b55

Is this intentional?

@saleniuk saleniuk changed the base branch from develop to release/candidate March 14, 2025 17:27
Copy link
Contributor

@saleniuk looks like you are rolling back kalium to a previous commitish.

This means that the PR's target branch (develop) is using a newer version of Kalium, and the changes in this PR will rollback Kalium to an older version.

develop This PR
1fb0f78f3758056cbe74d8debd8ad17cef76d5f3 62b50d50bd060af4840de2030330c4e87c659b55

Is this intentional?

Copy link

@saleniuk
Copy link
Contributor Author

closing in favour of the one that will be merged to RC first: #3917

@saleniuk saleniuk closed this Mar 14, 2025
@saleniuk saleniuk deleted the fix/crash-audio-message-playback branch March 14, 2025 18:11
Copy link
Contributor

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

Copy link
Contributor

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants