-
Notifications
You must be signed in to change notification settings - Fork 41
fix: crashes on audio message playback [WPB-16195] #3917
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
fix: crashes on audio message playback [WPB-16195] #3917
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## release/candidate #3917 +/- ##
=====================================================
+ Coverage 45.59% 45.74% +0.15%
=====================================================
Files 491 492 +1
Lines 17078 17097 +19
Branches 2845 2845
=====================================================
+ Hits 7786 7821 +35
+ Misses 8500 8483 -17
- Partials 792 793 +1
... and 2 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
Built wire-android-staging-compat-pr-3917.apk is available for download |
app/src/main/kotlin/com/wire/android/media/audiomessage/ConversationAudioMessagePlayer.kt
Show resolved
Hide resolved
app/src/main/kotlin/com/wire/android/media/audiomessage/ConversationAudioMessagePlayer.kt
Outdated
Show resolved
Hide resolved
…into fix/audio-message-playback-crashing
|
Built wire-android-staging-compat-pr-3917.apk is available for download |
PR Submission Checklist for internal contributors
The PR Title
SQPIT-764
The PR Description
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 ofDROP_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 theAudioMessage
but instead it uses state from injected scopedAudioMessageViewModel
.Replaced
DROP_OLDEST
withSUSPEND
inConversationAudioMessagePlayer
to not lose any state updates.Testing
Test Coverage (Optional)
How to Test
Receive audio messages and try to play them.
PR Post Submission Checklist for internal contributors (Optional)
PR Post Merge Checklist for internal contributors
References
feat(conversation-list): Sort conversations by most emojis in the title #SQPIT-764
.