Voice Broadcast - Start record #7363

Florian14 · 2022-10-13T21:47:08Z

Type of change

WIP Feature
Bugfix
Technical
Other :

Content

Handle the recording side of the voice broadcast with play/pause/resume/stop actions
Voice messages are recorded in mp4 for technical reasons (the Android MediaRecorder does not allow to record in multiple files in other formats).
For the moment, voice messages are sent to the room as separate events in relation to the voice broadcast state event.
The aggregation of the events and the conversion to ogg will be done in dedicated PRs.

Note: common voice messages are still recorded in ogg

Motivation and context

Continue #7127

Screenshots / GIFs

(there is transparency because of gif compression)

Tests

Enable the voice broadcast feature flag
Start a voice broadcast in a room
Verify that voice messages are sent to the room
Verify that the VB is correctly paused & resumed when playing with the buttons
Stop the VB and observe that a final voice message has been sent

Tested devices

Physical
Emulator
OS version(s):

Checklist

Changes has been tested on an Android device or Android emulator with API 21
UI change has been tested on both light and dark themes
Accessibility has been taken into account. See https://github.com/vector-im/element-android/blob/develop/CONTRIBUTING.md#accessibility
Pull request is based on the develop branch
Pull request includes a new file under ./changelog.d. See https://github.com/vector-im/element-android/blob/develop/CONTRIBUTING.md#changelog
Pull request includes screenshots or videos if containing UI changes
Pull request includes a sign off
You've made a self review of your PR
If you have modified the screen flow, or added new screens to the application, you have updated the test UiAllScreensSanityTest.allScreensTest()

mnaturel · 2022-10-17T09:34:18Z

...oid/src/main/java/org/matrix/android/sdk/internal/session/room/send/LocalEchoEventFactory.kt

            roomId: String,
            attachment: ContentAttachmentData,
-            rootThreadEventId: String?
+            rootThreadEventId: String?,


Not blocking but I am wondering if we should remove the rootThreadEventId param and build the relatesTo param in case of threads at an upper level?

I started considering this point but I am not sure it would make sense to build the relatesTo related to a thread in an upper level, IMO it makes sense to do it in the factory 😕

mnaturel · 2022-10-17T09:35:39Z

...oid/src/main/java/org/matrix/android/sdk/internal/session/room/send/LocalEchoEventFactory.kt

        return createMessageEvent(roomId, content)
    }

+    private fun generateThreadRelationContent(rootThreadEventId: String) =


Thanks for mutualizing this code!

bmarty

Nice work, a few remarks. Did not try to run the code.

bmarty · 2022-10-18T09:57:33Z

vector/src/main/java/im/vector/app/features/home/room/detail/composer/AudioMessageHelper.kt

-        voiceRecorder.stopRecord()
-        stopRecordingAmplitudes()
+        voiceRecorder.pauseRecord()
+        pauseRecordingAmplitudes()


I am OK with this change, but can it have a side effect on the voice message feature?

I did not observe any problem during my few tests with voice messages but I miss some bg about it, I revert the change for the moment and added a comment about it def9fc0

bmarty · 2022-10-18T10:03:23Z

vector/src/main/java/im/vector/app/features/voice/AbstractVoiceRecorderQ.kt

+    fun setNextOutputFile(roomId: String) {
+        val mediaRecorder = mediaRecorder ?: return
+        nextOutputFile = createOutputFile(roomId)
+        if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.O) {


We are always >= Q here, no? So this test will be always true. Do you agree?

bmarty · 2022-10-18T10:03:50Z

vector/src/main/java/im/vector/app/features/voice/AbstractVoiceRecorderQ.kt

+    private fun setOutputFile(roomId: String) {
+        val mediaRecorder = mediaRecorder ?: return
+        outputFile = outputFile ?: createOutputFile(roomId)
+        if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.O) {


Same remark

bmarty · 2022-10-18T10:05:31Z

vector/src/main/java/im/vector/app/features/voice/VoiceRecorderL.kt

+
+    override fun resumeRecord() {
+        Toast.makeText(context, "Not implemented for this Android version", Toast.LENGTH_SHORT).show()
+    }


Maybe we could add a fun canPauseAndResume(): Boolean in the interface to adapt the UI pro-actively. Or a more extensive fun getRecorderCapability(): RecorderCapability.

I will try to consider this point in another PR

bmarty · 2022-10-18T10:08:20Z

...or/src/main/java/im/vector/app/features/voicebroadcast/usecase/StartVoiceBroadcastUseCase.kt

+                "Voice message.${voiceMessageFile.extension}"
+        )
+        val audioType = outputFileUri.toMultiPickerAudioType(context) ?: return
+        if (audioType.duration > 1000) {


Do we want to keep this check on the duration? It was for voice message IIRC.

If we keep it, the else block should delete the file, maybe?

Yes, I was hesitating on that point, I think it makes sense to remove it, let's see what happens 🤞🏻 92bd8cd

bmarty · 2022-10-18T10:09:48Z

vector/src/main/java/im/vector/app/features/voicebroadcast/usecase/StopVoiceBroadcastUseCase.kt

+    }
+
+    private fun stopRecording() {
+        if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.Q) {


I think this check could be removed.

bmarty · 2022-10-18T10:11:01Z

vector/src/main/java/im/vector/app/features/voicebroadcast/VoiceBroadcastConstants.kt

+    const val STATE_ROOM_VOICE_BROADCAST_INFO = "io.element.voice_broadcast_info"
+
+    /** Default voice broadcast chunk duration, in seconds. */
+    const val DEFAULT_CHUNK_LENGTH = 30


Could be nice for clarity where it's used to have the unit in the name

Suggested change

const val DEFAULT_CHUNK_LENGTH = 30

const val DEFAULT_CHUNK_LENGTH_IN_SECONDS = 30

sonarqubecloud · 2022-10-18T12:25:44Z

SonarCloud Quality Gate failed.

0 Bugs
0 Vulnerabilities
0 Security Hotspots
5 Code Smells

16.2% Coverage
0.7% Duplication

bmarty

That's all good to me, thanks for the update!

Florian14 force-pushed the feature/fre/voice_broadcast_start_record branch 5 times, most recently from 0154ead to 82bd02c Compare October 14, 2022 14:41

Florian14 requested review from a team and bmarty and removed request for a team October 14, 2022 14:42

Florian14 marked this pull request as ready for review October 14, 2022 14:42

mnaturel reviewed Oct 17, 2022

View reviewed changes

Florian14 added the Z-NextRelease For issues and PRs which should be included in the NextRelease. label Oct 17, 2022

Florian Renaud added 15 commits October 17, 2022 22:33

Trigger play/pause/resume/stop actions on VoiceRecorder

a88a172

Reduce duplicated code

dbc6197

Add sdk entry to attach reference to the outgoing events

33a021c

Improve VoiceRecorder abstraction

e775404

Add VoiceBroadcastRecorder

ad2bf8d

Trigger VoiceBroadcast recording actions

3ad245d

Move Voice Broadcast constants into dedicated object

aecb660

Fix tests

ad730d5

Change VoiceBroadcastRecorder as Interface

c492fda

Add changelog

06bceef

Reformat and add trailing commas

fb9c747

Compute file size from chunk length

64e6a2b

Compute max file size from chunk length

9d35e81

Pause recording when the composer is not visible anymore

62596b3

Add punctuation to kdoc

3a951f2

Florian14 force-pushed the feature/fre/voice_broadcast_start_record branch from 82bd02c to 3a951f2 Compare October 17, 2022 22:41

Change chunk_length type in unit test

1db3d69

bmarty reviewed Oct 18, 2022

View reviewed changes

Florian Renaud added 4 commits October 18, 2022 13:33

Remove useless Android API checks

400118e

Revert AudioMessageHelper.pauseRecording

def9fc0

Voice Broadcast - Remove check on voice message minimum duration

92bd8cd

Rename const DEFAULT_CHUNK_LENGTH_IN_SECONDS

b9335c6

bmarty approved these changes Oct 18, 2022

View reviewed changes

Florian14 merged commit b675005 into develop Oct 18, 2022

Florian14 deleted the feature/fre/voice_broadcast_start_record branch October 18, 2022 14:43

	const val DEFAULT_CHUNK_LENGTH = 30
	const val DEFAULT_CHUNK_LENGTH_IN_SECONDS = 30

Voice Broadcast - Start record #7363

Voice Broadcast - Start record #7363

Uh oh!

Conversation

Florian14 commented Oct 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Type of change

Content

Motivation and context

Screenshots / GIFs

Tests

Tested devices

Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bmarty left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Florian14 Oct 18, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sonarqubecloud bot commented Oct 18, 2022

Uh oh!

bmarty left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Florian14 commented Oct 13, 2022 •

edited

Loading

Florian14 Oct 18, 2022 •

edited

Loading