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.

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

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

Merged
merged 14 commits into from
Jan 31, 2023
Merged
1 change: 1 addition & 0 deletions changelog.d/8012.bugfix
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
[Voice Broadcast] Use internal playback timer to compute the current playback position
Original file line number Diff line number Diff line change
Expand Up @@ -102,7 +102,7 @@ class VideoViewHolder constructor(itemView: View) :

views.videoView.setOnPreparedListener {
stopTimer()
countUpTimer = CountUpTimer(100).also {
countUpTimer = CountUpTimer(intervalInMs = 100).also {
it.tickListener = CountUpTimer.TickListener {
val duration = views.videoView.duration
val progress = views.videoView.currentPosition
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
/*
* Copyright (c) 2023 New Vector Ltd
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

package im.vector.lib.core.utils.timer

interface Clock {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

fun epochMillis(): Long
}

class DefaultClock : Clock {

/**
* Provides a UTC epoch in milliseconds
*
* This value is not guaranteed to be correct with reality
* as a User can override the system time and date to any values.
*/
override fun epochMillis(): Long {
return System.currentTimeMillis()
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -28,41 +28,50 @@ import java.util.concurrent.atomic.AtomicBoolean
import java.util.concurrent.atomic.AtomicLong

@OptIn(kotlinx.coroutines.ExperimentalCoroutinesApi::class)
class CountUpTimer(private val intervalInMs: Long = 1_000) {
class CountUpTimer(initialTime: Long = 0L, private val intervalInMs: Long = 1_000) {

private val coroutineScope = CoroutineScope(Dispatchers.Main)
private val elapsedTime: AtomicLong = AtomicLong()
private val resumed: AtomicBoolean = AtomicBoolean(false)

private val clock: Clock = DefaultClock()
private val lastTime: AtomicLong = AtomicLong()
private val elapsedTime: AtomicLong = AtomicLong(initialTime)

init {
startCounter()
}

private fun startCounter() {
tickerFlow(coroutineScope, intervalInMs / 10)
tickerFlow(coroutineScope, intervalInMs)
Copy link
Member

Choose a reason for hiding this comment

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

/ 10 was used for this reason: c69bc12

Are you sure you want to revert this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

.filter { resumed.get() }
.map { elapsedTime.addAndGet(intervalInMs / 10) }
.filter { it % intervalInMs == 0L }
.onEach {
tickListener?.onTick(it)
}.launchIn(coroutineScope)
.map { elapsedTime() }
.onEach { tickListener?.onTick(it) }
.launchIn(coroutineScope)
}

var tickListener: TickListener? = null

fun elapsedTime(): Long {
return elapsedTime.get()
return if (resumed.get()) {
val now = clock.epochMillis()
elapsedTime.addAndGet(now - lastTime.getAndSet(now))
} else {
elapsedTime.get()
}
}

fun pause() {
tickListener?.onTick(elapsedTime())
resumed.set(false)
}

fun resume() {
lastTime.set(clock.epochMillis())
resumed.set(true)
}

fun stop() {
tickListener?.onTick(elapsedTime())
coroutineScope.cancel()
}

Expand Down
2 changes: 1 addition & 1 deletion tools/check/forbidden_strings_in_code.txt
Original file line number Diff line number Diff line change
Expand Up @@ -176,7 +176,7 @@ PreferenceManager\.getDefaultSharedPreferences==2
R\.string\.template_

### Use the Clock interface, or use `measureTimeMillis`
System\.currentTimeMillis\(\)===2
System\.currentTimeMillis\(\)===3

### Remove extra space between the name and the description
\* @\w+ \w+ +
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -166,7 +166,7 @@ class WebRtcCall(
private var videoSender: RtpSender? = null
private var screenSender: RtpSender? = null

private val timer = CountUpTimer(1000L).apply {
private val timer = CountUpTimer(intervalInMs = 1000L).apply {
tickListener = CountUpTimer.TickListener { milliseconds ->
val formattedDuration = formatDuration(Duration.ofMillis(milliseconds))
listeners.forEach {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -198,7 +198,7 @@ class AudioMessageHelper @Inject constructor(

private fun startRecordingAmplitudes() {
amplitudeTicker?.stop()
amplitudeTicker = CountUpTimer(50).apply {
amplitudeTicker = CountUpTimer(intervalInMs = 50).apply {
tickListener = CountUpTimer.TickListener { onAmplitudeTick() }
resume()
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -105,7 +105,7 @@ abstract class LiveLocationUserItem : VectorEpoxyModel<LiveLocationUserItem.Hold
}

class Holder : VectorEpoxyHolder() {
val timer: CountUpTimer = CountUpTimer(1000)
val timer: CountUpTimer = CountUpTimer(intervalInMs = 1000)
val itemUserAvatarImageView by bind<ImageView>(R.id.itemUserAvatarImageView)
val itemUserDisplayNameTextView by bind<TextView>(R.id.itemUserDisplayNameTextView)
val itemRemainingTimeTextView by bind<TextView>(R.id.itemRemainingTimeTextView)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -206,7 +206,7 @@ class VoiceBroadcastPlayerImpl @Inject constructor(
}
}
State.Buffering -> {
val savedPosition = currentVoiceBroadcast?.voiceBroadcastId?.let { playbackTracker.getPlaybackTime(it) }
val savedPosition = currentVoiceBroadcast?.let { playbackTracker.getPlaybackTime(it.voiceBroadcastId) }
when {
// resume playback from the next sequence item
playlist.currentSequence != null -> playlist.getNextItem()?.let { startPlayback(it.startTime) }
Expand All @@ -223,24 +223,42 @@ class VoiceBroadcastPlayerImpl @Inject constructor(
}
}

private fun startPlayback(position: Int) {
private fun startPlayback(playbackPosition: Int) {
stopPlayer()
playingState = State.Buffering

val playlistItem = playlist.findByPosition(playbackPosition) ?: run {
Timber.w("## Voice Broadcast | No content to play at position $playbackPosition"); stop(); return
}
val sequence = playlistItem.sequence ?: run {
Timber.w("## Voice Broadcast | Playlist item has no sequence"); stop(); return
}

currentVoiceBroadcast?.let {
val percentage = tryOrNull { playbackPosition.toFloat() / playlist.duration } ?: 0f
playbackTracker.updatePausedAtPlaybackTime(it.voiceBroadcastId, playbackPosition, percentage)
}

val playlistItem = playlist.findByPosition(position)
val content = playlistItem?.audioEvent?.content ?: run { Timber.w("## Voice Broadcast | No content to play at position $position"); return }
val sequence = playlistItem.sequence ?: run { Timber.w("## Voice Broadcast | Playlist item has no sequence"); return }
val sequencePosition = position - playlistItem.startTime
prepareCurrentPlayerJob = sessionScope.launch {
try {
val mp = prepareMediaPlayer(content)
val mp = prepareMediaPlayer(playlistItem.audioEvent.content)

// Take the difference between the duration given from the media player and the duration given from the chunk event
// If the offset is smaller than 500ms, we consider there is no offset to keep the normal behaviour
val offset = (mp.duration - playlistItem.duration).takeUnless { it < 500 }?.coerceAtLeast(0) ?: 0
val sequencePosition = offset + (playbackPosition - playlistItem.startTime)

playlist.currentSequence = sequence - 1 // will be incremented in onNextMediaPlayerStarted
mp.start()
if (sequencePosition > 0) {
mp.seekTo(sequencePosition)
}

onNextMediaPlayerStarted(mp)
} catch (failure: VoiceBroadcastFailure.ListeningError) {
playingState = State.Error(failure)
if (failure.cause !is CancellationException) {
playingState = State.Error(failure)
}
}
}
}
Expand All @@ -259,7 +277,7 @@ class VoiceBroadcastPlayerImpl @Inject constructor(
playingState = State.Playing
currentMediaPlayer?.start()
} else {
val savedPosition = currentVoiceBroadcast?.voiceBroadcastId?.let { playbackTracker.getPlaybackTime(it) } ?: 0
val savedPosition = currentVoiceBroadcast?.let { playbackTracker.getPlaybackTime(it.voiceBroadcastId) } ?: 0
startPlayback(savedPosition)
}
}
Expand Down Expand Up @@ -301,7 +319,7 @@ class VoiceBroadcastPlayerImpl @Inject constructor(
} catch (failure: VoiceBroadcastFailure.ListeningError) {
// Do not change the playingState if the current player is still valid,
// the error will be thrown again when switching to the next player
if (playingState == State.Buffering || tryOrNull { currentMediaPlayer?.isPlaying } != true) {
if (failure.cause !is CancellationException && (playingState == State.Buffering || tryOrNull { currentMediaPlayer?.isPlaying } != true)) {
playingState = State.Error(failure)
}
}
Expand Down Expand Up @@ -355,6 +373,8 @@ class VoiceBroadcastPlayerImpl @Inject constructor(

private fun stopPlayer() {
tryOrNull { currentMediaPlayer?.stop() }
playbackTicker.stopPlaybackTicker()

currentMediaPlayer?.release()
currentMediaPlayer = null

Expand All @@ -376,7 +396,7 @@ class VoiceBroadcastPlayerImpl @Inject constructor(
State.Paused,
State.Buffering,
is State.Error,
State.Idle -> playbackTicker.stopPlaybackTicker(voiceBroadcastId)
State.Idle -> playbackTicker.stopPlaybackTicker()
}

// Notify playback tracker about error
Expand Down Expand Up @@ -416,22 +436,6 @@ class VoiceBroadcastPlayerImpl @Inject constructor(
prepareNextMediaPlayer()
}

private fun getCurrentPlaybackPosition(): Int? {
val voiceBroadcastId = currentVoiceBroadcast?.voiceBroadcastId ?: return null
val computedPosition = tryOrNull { currentMediaPlayer?.currentPosition }?.let { playlist.currentItem?.startTime?.plus(it) }
val savedPosition = playbackTracker.getPlaybackTime(voiceBroadcastId)
return computedPosition ?: savedPosition
}

private fun getCurrentPlaybackPercentage(): Float? {
val playlistPosition = playlist.currentItem?.startTime
val computedPosition = tryOrNull { currentMediaPlayer?.currentPosition }?.let { playlistPosition?.plus(it) } ?: playlistPosition
val duration = playlist.duration
val computedPercentage = if (computedPosition != null && duration > 0) computedPosition.toFloat() / duration else null
val savedPercentage = currentVoiceBroadcast?.voiceBroadcastId?.let { playbackTracker.getPercentage(it) }
return computedPercentage ?: savedPercentage
}

private inner class MediaPlayerListener :
MediaPlayer.OnInfoListener,
MediaPlayer.OnCompletionListener,
Expand Down Expand Up @@ -488,40 +492,41 @@ class VoiceBroadcastPlayerImpl @Inject constructor(

fun startPlaybackTicker(id: String) {
playbackTicker?.stop()
playbackTicker = CountUpTimer(50L).apply {
tickListener = CountUpTimer.TickListener { onPlaybackTick(id) }
playbackTicker = CountUpTimer(
initialTime = playbackTracker.getPlaybackTime(id)?.toLong() ?: 0L,
intervalInMs = 50L
).apply {
tickListener = CountUpTimer.TickListener { onPlaybackTick(id, it.toInt()) }
resume()
}
onPlaybackTick(id)
}

fun stopPlaybackTicker(id: String) {
fun stopPlaybackTicker() {
playbackTicker?.stop()
playbackTicker?.tickListener = null
playbackTicker = null
onPlaybackTick(id)
}

private fun onPlaybackTick(id: String) {
val playbackTime = getCurrentPlaybackPosition()
val percentage = getCurrentPlaybackPercentage()
private fun onPlaybackTick(id: String, position: Int) {
val percentage = tryOrNull { position.toFloat() / playlist.duration }
when (playingState) {
State.Playing -> {
if (playbackTime != null && percentage != null) {
playbackTracker.updatePlayingAtPlaybackTime(id, playbackTime, percentage)
if (percentage != null) {
playbackTracker.updatePlayingAtPlaybackTime(id, position, percentage)
}
}
State.Paused,
State.Buffering -> {
if (playbackTime != null && percentage != null) {
playbackTracker.updatePausedAtPlaybackTime(id, playbackTime, percentage)
if (percentage != null) {
playbackTracker.updatePausedAtPlaybackTime(id, position, percentage)
}
}
State.Idle -> {
// restart the playback time if player completed with less than 250 ms remaining time
if (playbackTime == null || percentage == null || (playlist.duration - playbackTime) < 250) {
// restart the playback time if player completed with less than 1s remaining time
if (percentage == null || (playlist.duration - position) < 1000) {
playbackTracker.stopPlayback(id)
} else {
playbackTracker.updatePausedAtPlaybackTime(id, playbackTime, percentage)
playbackTracker.updatePausedAtPlaybackTime(id, position, percentage)
}
}
is State.Error -> Unit
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,6 @@ class VoiceBroadcastPlaylist(
}

data class PlaylistItem(val audioEvent: MessageAudioEvent, val startTime: Int) {
val sequence: Int?
get() = audioEvent.sequence
val sequence: Int? = audioEvent.sequence
val duration: Int = audioEvent.duration
}
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,6 @@ import org.matrix.android.sdk.api.session.events.model.Content
import org.matrix.android.sdk.api.session.room.model.message.MessageContent
import org.matrix.android.sdk.api.session.room.model.message.MessageType.MSGTYPE_VOICE_BROADCAST_INFO
import org.matrix.android.sdk.api.session.room.model.relation.RelationDefaultContent
import timber.log.Timber

/**
* Content of the state event of type [VoiceBroadcastConstants.STATE_ROOM_VOICE_BROADCAST_INFO].
Expand All @@ -50,8 +49,4 @@ data class MessageVoiceBroadcastInfoContent(

val voiceBroadcastState: VoiceBroadcastState? = VoiceBroadcastState.values()
.find { it.value == voiceBroadcastStateStr }
?: run {
Timber.w("Invalid value for state: `$voiceBroadcastStateStr`")
null
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -248,30 +248,20 @@ class VoiceBroadcastRecorderQ(
recordingTicker = CountUpTimer().apply {
tickListener = CountUpTimer.TickListener { onTick(elapsedTime()) }
resume()
onTick(elapsedTime())
}
}

fun pause() {
recordingTicker?.apply {
pause()
onTick(elapsedTime())
}
recordingTicker?.pause()
}

fun resume() {
recordingTicker?.apply {
resume()
onTick(elapsedTime())
}
recordingTicker?.resume()
}

fun stop() {
recordingTicker?.apply {
stop()
onTick(elapsedTime())
recordingTicker = null
}
recordingTicker?.stop()
recordingTicker = null
}

private fun onTick(elapsedTimeMillis: Long) {
Expand Down
Loading