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 - live listening #7419

Merged
merged 12 commits into from Oct 20, 2022
Merged

Voice broadcast - live listening #7419

merged 12 commits into from Oct 20, 2022

Conversation

Florian14
Copy link
Contributor

Type of change

  • WIP Feature
  • Bugfix
  • Technical
  • Other :

Content

Be able to listen to an ongoing voice broadcast
When a voice broadcast is live, trigger the play action will start listening to the last received chunk and wait for the new ones with a "buffering" state in the meantime.
Also, when the recording is paused, the current chunk is now stopped and sent to the room. Resume the recording will start a new chunk file.

Note: the UI will be improved in a dedicated PR for a better rendering of the listening state

Motivation and context

Continue #7127

Screenshots / GIFs

Tests

  • Start playing a live voice broadcast
  • Verify that we are able to listen it until it is stopped or paused by the recorder
  • Verify that when the recorder resume the voice broadcast, you continue to listen to the new chunks
  • Verify that the listener is able to pause and resume the listening without problem

Tested devices

  • Physical
  • Emulator
  • OS version(s):

Checklist

@Florian14 Florian14 changed the title Feature/fre/voice broadcast live listening Voice broadcast - live listening Oct 20, 2022
@Florian14 Florian14 force-pushed the feature/fre/voice_broadcast_live_listening branch from 6aa494f to 0d7a36a Compare October 20, 2022 10:36
@Florian14 Florian14 requested review from a team, ericdecanini and ganfra and removed request for a team and ericdecanini October 20, 2022 10:41
@Florian14 Florian14 force-pushed the feature/fre/voice_broadcast_live_listening branch from 0d7a36a to dd54b76 Compare October 20, 2022 10:51
@Florian14 Florian14 marked this pull request as ready for review October 20, 2022 11:10
@bmarty
Copy link
Member

bmarty commented Oct 20, 2022

@Florian14 do you want to drop this commit? dd54b76

@Florian14
Copy link
Contributor Author

@Florian14 do you want to drop this commit? dd54b76

Yes I will remove it

bmarty
bmarty reviewed Oct 20, 2022
View reviewed changes
Copy link
Member

@bmarty bmarty left a comment

Choose a reason for hiding this comment

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

Amazing work!
I have made a few remarks but I have not tested the code.

vector/src/main/java/im/vector/app/features/voicebroadcast/VoiceBroadcastPlayer.kt Show resolved Hide resolved
vector/src/main/java/im/vector/app/features/voicebroadcast/VoiceBroadcastPlayer.kt Show resolved Hide resolved
vector/src/main/java/im/vector/app/features/voicebroadcast/VoiceBroadcastPlayer.kt Outdated Show resolved Hide resolved
...src/main/java/im/vector/app/features/voicebroadcast/usecase/GetVoiceBroadcastStateUseCase.kt Outdated
import javax.inject.Inject

class GetVoiceBroadcastStateUseCase @Inject constructor(
private val session: Session,
Copy link
Member

Choose a reason for hiding this comment

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

Please inject ActiveSessionHolder here instead of a Session, since an instance of this will be injected in a Singleton.

Copy link
Member

Choose a reason for hiding this comment

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

this will be considering in another PR - we create #7423 to track this point

mnaturel
mnaturel reviewed Oct 20, 2022
View reviewed changes
vector/src/main/java/im/vector/app/features/voicebroadcast/VoiceBroadcastPlayer.kt
) {
private val session
get() = sessionHolder.getActiveSession()
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we use getSafeActiveSession instead? The getActiveSession may throw an exception. If we keep, maybe we should try/catch in the call sites of this variable.

Copy link
Member

Choose a reason for hiding this comment

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

will be handled by #7423

mnaturel
mnaturel reviewed Oct 20, 2022
View reviewed changes
...src/main/java/im/vector/app/features/voicebroadcast/usecase/GetVoiceBroadcastStateUseCase.kt Outdated
import javax.inject.Inject

class GetVoiceBroadcastStateUseCase @Inject constructor(
private val session: Session,
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess it is usually preferable to use the ActiveSessionHolder instead of the Session component.

mnaturel
mnaturel reviewed Oct 20, 2022
View reviewed changes
vector/src/main/java/im/vector/app/features/voicebroadcast/VoiceBroadcastPlayer.kt

private val mediaPlayerScope = CoroutineScope(Dispatchers.IO)
private val coroutineScope = CoroutineScope(SupervisorJob() + Dispatchers.Default)
Copy link
Contributor

Choose a reason for hiding this comment

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

Just a thought: you will see but you may have a hard time when trying to add unit tests on this class due to the usage of this coroutineScope. Maybe in the future we may have to change the implementation of some methods.

@yostyle yostyle force-pushed the feature/fre/voice_broadcast_live_listening branch from 82ef8e6 to 1004496 Compare October 20, 2022 20:18
@giomfo giomfo mentioned this pull request Oct 20, 2022
Open
@giomfo giomfo requested a review from bmarty October 20, 2022 20:56
@Florian14 Florian14 force-pushed the feature/fre/voice_broadcast_live_listening branch from 1004496 to 0a9f2bf Compare October 20, 2022 21:03
giomfo
giomfo approved these changes Oct 20, 2022
View reviewed changes
Copy link
Member

@giomfo giomfo left a comment

Choose a reason for hiding this comment

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

Approved - #7423 has been created to improve session retrieving

@sonarcloud
Copy link

sonarcloud bot commented Oct 20, 2022

SonarCloud Quality Gate failed.    Quality Gate failed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 1 Code Smell

4.1% 4.1% Coverage
0.0% 0.0% Duplication

@Florian14 Florian14 merged commit d44d81e into develop Oct 20, 2022
@Florian14 Florian14 deleted the feature/fre/voice_broadcast_live_listening branch October 20, 2022 21:52
@danpe
Copy link

danpe commented Nov 6, 2022

Is this based on voice notes (files) or WebRTC?

@giomfo
Copy link
Member

giomfo commented Nov 7, 2022

Hi @danpe this implementation is based on voice messages (files), see details

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mnaturel mnaturel mnaturel left review comments

@giomfo giomfo giomfo approved these changes

@ganfra ganfra Awaiting requested review from ganfra ganfra was automatically assigned from element-hq/element-android-reviewers

@bmarty bmarty Awaiting requested review from bmarty

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

Successfully merging this pull request may close these issues.

None yet
6 participants
@Florian14 @bmarty @danpe @giomfo @mnaturel @yostyle