Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Voice Broadcast - Some internal improvements related to the player #7478

Merged
merged 4 commits into from
Oct 31, 2022
Merged

Voice Broadcast - Some internal improvements related to the player #7478

merged 4 commits into from
Oct 31, 2022

Conversation

Florian14
Copy link
Contributor

Type of change

  • Feature
  • Bugfix
  • Technical
  • Other : code improvement

Content

  • Added an interface upon the voice broadcast player implementation to clarify which methods are exposed to the application and move some code
  • Added a use case dedicated to the fetch of a voice broadcast playlist (live or ended)
  • Rework some code in the player implementation

Motivation and context

Consolidate the code base of the voice broadcast player

Screenshots / GIFs

Tests

Tested devices

  • Physical
  • Emulator
  • OS version(s):

Checklist

@Florian14 Florian14 force-pushed the feature/fre/voice_broadcast_player_interface branch from 19473b3 to 3fcac09 Compare October 27, 2022 21:50
@Florian14 Florian14 marked this pull request as ready for review October 28, 2022 06:56
@Florian14 Florian14 requested review from a team and jmartinesp and removed request for a team October 28, 2022 06:56
@sonarqubecloud
Copy link

SonarCloud Quality Gate failed.    Quality Gate failed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 5 Code Smells

25.5% 25.5% Coverage
2.7% 2.7% Duplication

@jmartinesp
Copy link
Member

Do the tests need to be run between 2 real devices? I tried with a physical one and an emulator and I never got it to work in real time.

@jmartinesp
Copy link
Member

jmartinesp commented Oct 28, 2022 •
edited
Loading

Sorry, I did several retries and I found 3 important issues:

  1. Infinite 'buffering': I started broadcasting on device A, the message appeared on device B. After a few seconds, I tapped on play on device B and I got an indeterminate progress bar, but after 30-40s is still was showing this progress bar and no audio came out.
  2. Broadcasting message won't play until a previous one is played: I don't know if this is 100% accurate, but it turns out that the only reliable way I found to listen to a stream is try to play an already finished one, pause it, then go back to the live one. Unless I did this, it didn't matter how much I waited, the live stream wouldn't play at all (the 1st issue might be a side-effect of this one).
  3. Same stream being played twice: I don't know how I exactly did this, I think I played and paused several times in a really short amount of time, but I ended up with the same broadcast being played twice with some delay. I could pause one of the players, but there was a 'background' one that kept playing the broadcast until I left the room.

@jmartinesp
Copy link
Member

It seems like issues 1 and 2 were related to long 'buffering' times (2min) before a chunk of voice broadcast is sent from the streaming device to the listeners, the UX could probably be improved, but at least we know it's working properly.

I can't reproduce the 3rd issue anymore, so I guess it's quite difficult to trigger.

jmartinesp
jmartinesp approved these changes Oct 31, 2022
View reviewed changes
Copy link
Member

@jmartinesp jmartinesp left a comment

Choose a reason for hiding this comment

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

LGTM. There are some UX issues around the buffering and there is a small duplication that could be addressed, but overall it works great.

It would be great if we could address the 3rd issue I mentioned about 2 streams being played at the same time, but I could only trigger it that time, so I think it's pretty difficult to reproduce.

...or/src/main/java/im/vector/app/features/voicebroadcast/listening/VoiceBroadcastPlayerImpl.kt
Comment on lines +180 to +189
State.PLAYING -> {
if (nextMediaPlayer == null) {
coroutineScope.launch { nextMediaPlayer = prepareNextMediaPlayer() }
}
}
State.PAUSED -> {
if (nextMediaPlayer == null) {
coroutineScope.launch { nextMediaPlayer = prepareNextMediaPlayer() }
}
}
Copy link
Member

Choose a reason for hiding this comment

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

These 2 cases could probably be merged.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch, I'll change it in a new PR

@Florian14
Copy link
Contributor Author

Thanks for your feedback @jmartinesp 🙏🏻 , I will keep in mind your issue about the stream played twice

@Florian14 Florian14 merged commit 01ab39e into resilience-rc Oct 31, 2022
@Florian14 Florian14 deleted the feature/fre/voice_broadcast_player_interface branch October 31, 2022 09:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jmartinesp jmartinesp jmartinesp approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@Florian14 @jmartinesp