[Voice Broadcast] Use internal playback timer to compute the playback position #8012

Florian14 · 2023-01-26T17:35:06Z

Type of change

Feature
Bugfix
Technical
Other :

Content

Rely on an internal playback timer to display the current playback position instead of relying on the position given by the android media player.

Note:
In my opinion, this is not an ideal solution, I reused and improved the existing CountUpTimer which is not based on real elapsed time. Under the hood, it's a tickerFlow which notifies at a fixed interval (50 ms in our case) except if we pause it. It can shift a bit for about +/- the intervalMs on each pause/resume depending on the timing.
Btw, I noticed that a too-low interval (eg. 10 ms) will break the timer. I also noticed perfs issues after enabling the debugger on android studio, the Flow takes too much time to notify. I don't know if a busy device can have the same kind of issue.

Motivation and context

Voice broadcasts sent by Element-Web were causing trouble in the playback position computation.
The chunks sent from the web are encoded in ogg format with aggregated metadata which is not the case for the other VB sent from iOS or Android.

Also, when the recorder is paused on the web, the aggregation is reset, so we cannot rely on the ogg file metadata to calculate the current playback position. The android media player also gives the aggregated position and duration of these files causing glitches in the playback rendering.

I did not find a way to get the relative position in the android MediaPlayer API. Moreover, there are some glitches when we switch to the next file, MediaPlayer.currentPosition starts returning the relative position (eg. 0ms/250ms/375ms) and then jumps to the aggregatedPosition (eg. 15_000ms...) but always returns the aggregated duration (eg. 45_000ms for a chunk of 15_000ms, supposing we are listening to the third chunk).

Screenshots / GIFs

Tests

Have several VB with several chunks (you can set the chunk duration to 15s on web in the /devtools options) recording from the web and iOS or Android
Verify that the playback position is correcty updated on each VB
Verify that the playback position is still correct after moving the cursor
Stress a bit the playback by switching to another one or playing with the controls (pause/resume)

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~~ main 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()

Florian14 · 2023-01-27T17:04:47Z

library/core-utils/src/main/java/im/vector/lib/core/utils/timer/Clock.kt

+
+package im.vector.lib.core.utils.timer
+
+interface Clock {


I did not find a better way instead of duplicating this file. I have issues with DI after moving the existing clock interface from the vector module into this one. Also, I cannot move the CountUpTimer to the vector module because another lib is using it.

…he media player position

library/core-utils/src/main/java/im/vector/lib/core/utils/timer/CountUpTimer.kt

jonnyandrew · 2023-01-30T10:25:00Z

...or/src/main/java/im/vector/app/features/voicebroadcast/listening/VoiceBroadcastPlayerImpl.kt

@@ -270,6 +282,8 @@ class VoiceBroadcastPlayerImpl @Inject constructor(
                playbackTracker.updatePausedAtPlaybackTime(voiceBroadcast.voiceBroadcastId, positionMillis, positionMillis.toFloat() / duration)
            }
            playingState == State.Playing || playingState == State.Buffering -> {
+                stopPlayer()


Is it right that when the playing state is State.Playing that we call stopPlayer()?

Yes, as the user requested to listen from another position (potentially in another chunk file), we stop the current player and will start a new one just two lines later. If we don't stop it, the playback position which is saved in the next line could be erased by the current player

... but you're right, this is not necessary, it is already done in startPlayback method. I'll remove it from this place

bmarty

Just one question, not blocking.

bmarty · 2023-01-30T11:51:18Z

library/core-utils/src/main/java/im/vector/lib/core/utils/timer/CountUpTimer.kt

    init {
        startCounter()
    }

    private fun startCounter() {
-        tickerFlow(coroutineScope, intervalInMs / 10)
+        tickerFlow(coroutineScope, intervalInMs)


/ 10 was used for this reason: c69bc12

Are you sure you want to revert this?

@bmarty The elapsed time and the current time are updated when the timer is paused & resumed (see https://github.com/vector-im/element-android/blob/941d0e5cf4c509fbc1ef6d3f29fa4242266dc74e/library/core-utils/src/main/java/im/vector/lib/core/utils/timer/CountUpTimer.kt#L58-L66), it should be more precise now and the previous trick should not be necessary anymore. Moreover, the timer does not work anymore if the interval is too low (eg. under 10 ms), the ticks are too close and it does not notify in the expected delay (it can take more than 2 seconds to notify 1 second...), so it is better to keep the real interval value and do not divide the value by ten to prevent this bad behaviour.

Reading the code again, I think user can observe an extra delay if we keep a tick per second.
The pb is that the tickerFlow ticks every second, and not when the UI has to be udpated.

As discussed together IRL, @Florian14 will make a dedicated PR to improve CountUpTimer implementation, and make it respect its expected contract. Ideally with some tests around it.

Florian14 · 2023-01-30T13:33:13Z

Some minor improvements since the last review: https://github.com/vector-im/element-android/pull/8012/files/3d87b796b46441e1c08fd8bd772565ecce587715..3ce757ee4bd52b6ab945c1bc532a34a67cca7b63

…layer

yostyle

SGTM. I did some tests.

sonarcloud · 2023-01-31T09:59:13Z

SonarCloud Quality Gate failed.

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

0.0% Coverage
0.0% Duplication

Florian14 commented Jan 27, 2023

View reviewed changes

Florian14 requested review from a team, jonnyandrew and bmarty and removed request for a team January 27, 2023 17:05

Florian14 marked this pull request as ready for review January 27, 2023 17:05

Florian Renaud added 8 commits January 30, 2023 11:03

Add optional initial time parameter in CountUpTimer

af67705

Voice Broadcast - use internal playback timer instead of relying on t…

a061045

…he media player position

CountUpTimer - compute elapsed time using real time

bdfebac

Fix playback position on live broadcast

05ffadb

changelog

003c2cd

Fix lint

3d87b79

Remove useless log

ff07cad

Increase touch area of the seek bar

44266bc

jonnyandrew approved these changes Jan 30, 2023

View reviewed changes

remove useless code

15c610f

bmarty approved these changes Jan 30, 2023

View reviewed changes

Florian Renaud added 3 commits January 30, 2023 14:12

Change variable to immutable

7bb7a62

CountUpTimer.elapsedTime now returns updated value

ecc0cb5

Remove useless code since CountUpTimer notify on pause and stop

3ce757e

Florian14 force-pushed the bugfix/fre/fix_vb_scrubbing branch from 941d0e5 to 3ce757e Compare January 30, 2023 13:29

Florian Renaud added 2 commits January 30, 2023 15:21

Do not enter in error state in case of cancellation exception

a0ab6de

Handle the potential offset on the chunk position used by the media p…

f74c7fa

…layer

Florian14 requested a review from yostyle January 31, 2023 09:17

Florian14 added the Z-NextRelease For issues and PRs which should be included in the NextRelease. label Jan 31, 2023

yostyle approved these changes Jan 31, 2023

View reviewed changes

Florian14 merged commit ca37cc5 into develop Jan 31, 2023

Florian14 deleted the bugfix/fre/fix_vb_scrubbing branch January 31, 2023 10:19

Florian14 mentioned this pull request Feb 1, 2023

Improve CountUpTimer ticks precision #8058

Merged

15 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Voice Broadcast] Use internal playback timer to compute the playback position #8012

[Voice Broadcast] Use internal playback timer to compute the playback position #8012

Florian14 commented Jan 26, 2023 •

edited

Loading

Florian14 Jan 27, 2023

jonnyandrew Jan 30, 2023

Florian14 Jan 30, 2023

Florian14 Jan 30, 2023

bmarty left a comment

bmarty Jan 30, 2023

Florian14 Jan 30, 2023

bmarty Jan 31, 2023

bmarty Jan 31, 2023

Florian14 commented Jan 30, 2023 •

edited

Loading

yostyle left a comment

sonarcloud bot commented Jan 31, 2023

[Voice Broadcast] Use internal playback timer to compute the playback position #8012

[Voice Broadcast] Use internal playback timer to compute the playback position #8012

Conversation

Florian14 commented Jan 26, 2023 • edited Loading

Type of change

Content

Motivation and context

Screenshots / GIFs

Tests

Tested devices

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bmarty left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Florian14 commented Jan 30, 2023 • edited Loading

yostyle left a comment

Choose a reason for hiding this comment

sonarcloud bot commented Jan 31, 2023

Florian14 commented Jan 26, 2023 •

edited

Loading

Florian14 commented Jan 30, 2023 •

edited

Loading