Voice Broadcast - Fix playback stuck in buffering #7646
Conversation
Florian14
requested review from
a team and
onurays
and removed request for
a team
Florian14
force-pushed
the
bugfix/fre/fix_playback_stuck_in_buffering
branch
from
c302938 to
5330d9c
Compare
| @@ -73,7 +72,7 @@ class VoiceBroadcastPlayerImpl @Inject constructor( | |||
| @MainThread | |||
| set(value) { | |||
| if (field != value) { | |||
| Timber.w("isLiveListening: $field -> $value") | |||
| Timber.w("## Voice Broadcast | isLiveListening: $field -> $value") | |||
There was a problem hiding this comment.
This will produce many logs, right? Can we avoid if not necessary?
There was a problem hiding this comment.
Not so many, it will only log if the user is listening to a voice broadcast.
Then, if the VB is ended, it will log once (or not if the default value does not change). If the VB is live, it will log if the user scrolls in the playback or if the recorder pause/resume the recording, but in a quite acceptable frequency.
Btw, it doesn't need to warn, I changed it to debug
Same for the log about playingState, It can be a little more frequent but also only when listening to a VB. The frequency is still acceptable imo, and the log is useful for debugging.
Also changed to debug level
| @@ -373,6 +376,19 @@ class VoiceBroadcastPlayerImpl @Inject constructor( | |||
| // Notify live mode change to all the listeners attached to the current voice broadcast id | |||
| listeners[voiceBroadcastId]?.forEach { listener -> listener.onLiveModeChanged(isLiveListening) } | |||
| } | |||
|
|
|||
| // Live has ended and last chunk has been reached, we can stop the playback | |||
| if (!isLiveListening && playingState == State.BUFFERING && playlist.currentSequence == mostRecentVoiceBroadcastEvent?.content?.lastChunkSequence) { | |||
There was a problem hiding this comment.
To avoid possible bugs, we should encapsulate these logic in state data classes and create use case classes with unit test.
There was a problem hiding this comment.
I agree with you but I am not sure to follow exactly what you have in mind about the use cases. There are a lot of combinations with several parameters to take into account, For the moment, I don't know how many combinations there are and if I would be able to enumerate all of them.
I already refactored this class but I confirm it is still not perfect, it will require new refactoring and unit tests to prevent regression as every change can be dangerous. I created an issue to track the missing tests on this part. I'll try to improve this mechanism by introducing a state machine at this moment.
Florian14
force-pushed
the
feature/fre/voice_broadcast_handle_event_deletion
branch
2 times, most recently
from
f85f4cf to
620bebc
Compare
Florian14
force-pushed
the
bugfix/fre/fix_playback_stuck_in_buffering
branch
from
ca00a46 to
fe0bca7
Compare
|
SonarCloud Quality Gate failed. |
Type of change
Content
Improvement related to the voice broadcast playback, fix issues with buffering state
Also fixed an issue when the live broadcast is ended, the playback was not automatically reset after listening to the last chunk.
Motivation and context
Sometimes, the (live) playback was stuck in a buffering state after a pause/resume from the recorder or when receiving new chunks after reaching the current duration
Screenshots / GIFs
Tests
Verify that the playback is not stuck in buffering when starting listening to a live voice broadcast (from the beggining & after reaching the end)
The playback should be automatically reset when the listening is over and the live VB has ended.
Tested devices
Checklist